From 8e7e3c59448013be44b8cba2b7f89a894ee34113 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 1 Apr 2023 16:07:29 +0100 Subject: [PATCH] Improve time-safety of XDM-AUTHORIZATION-1 validation. While writing the previous patch, I realise that walking along a decrypted string and stopping to complain about the first mismatch you find is an anti-pattern. If we're going to deliberately give the same error message for various mismatches, so as not to give away which part failed first, then we should also avoid giving away the same information via a timing leak! I don't think this is serious enough to warrant the full-on advisory protocol, because XDM-AUTHORIZATION-1 is rarely used these days and also DES-based, so there are bigger problems with it. (Plus, why on earth is it based on encryption anyway, not a MAC?) But since I spotted it in passing, might as well fix it. --- ssh/x11fwd.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/ssh/x11fwd.c b/ssh/x11fwd.c index dcd298ab..c0ae59f1 100644 --- a/ssh/x11fwd.c +++ b/ssh/x11fwd.c @@ -231,22 +231,24 @@ static char *x11_verify(unsigned long peer_ip, int peer_port, return dupstr("cannot do XDM-AUTHORIZATION-1 without remote " "address data"); des_decrypt_xdmauth(auth->data+9, data, 24); - if (memcmp(auth->data, data, 8) != 0) { - /* cookie wrong */ - return dupstr("XDM-AUTHORIZATION-1 data failed check"); - } - if (GET_32BIT_MSB_FIRST(data+8) != peer_ip) { - /* IP wrong */ - return dupstr("XDM-AUTHORIZATION-1 data failed check"); - } - if ((int)GET_16BIT_MSB_FIRST(data+12) != peer_port) { - /* port wrong */ - return dupstr("XDM-AUTHORIZATION-1 data failed check"); - } - t = GET_32BIT_MSB_FIRST(data+14); + + /* Bitwise-OR together any mismatches in the fixed parts of + * the data, to allow checking it all at once */ + uint32_t mismatches = 0; + /* Check non-key half of auth cookie */ + for (i = 0; i < 8; i++) + mismatches |= auth->data[i] ^ data[i]; + /* Check IP address and port */ + mismatches |= GET_32BIT_MSB_FIRST(data+8) ^ peer_ip; + mismatches |= (unsigned short)(GET_16BIT_MSB_FIRST(data+12) ^ + peer_port); + /* Check zero padding */ for (i = 18; i < 24; i++) - if (data[i] != 0) /* zero padding wrong */ - return dupstr("XDM-AUTHORIZATION-1 data failed check"); + mismatches |= data[i]; + if (mismatches) + return dupstr("XDM-AUTHORIZATION-1 data failed check"); + + t = GET_32BIT_MSB_FIRST(data+14); tim = time(NULL); if (((unsigned long)t - (unsigned long)tim + XDM_MAXSKEW) > 2*XDM_MAXSKEW)