From 54cc0c5b296ee7c27b48a3c8e7aead6e74f2abf1 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 23 Jan 2017 20:08:18 +0000 Subject: [PATCH] Tweak bounds checks in pageant_add_keyfile. When we're going through the response from an SSH agent we asked for a list of keys, and processing the string lengths in the SSH-2 sequence of (public blob, comment) pairs, we were adding 4 to each string length, and although we checked if the result came out to a negative value (if interpreted as a signed integer) or a positive one going beyond the end of the response buffer, we didn't check if it wrapped round to a positive value less than 4. As a result, if an agent returned malformed data sent a length field of 0xFFFFFFFC, the pointer would advance no distance at all through the buffer, and the next iteration of the loop would check the same length field again. (However, this would only consume CPU pointlessly for a limited time, because the outer loop up to 'nkeys' would still terminate sooner or later. Also, I don't think this can sensibly be classed as a serious security hazard - it's arguably a borderline DoS, but it requires a hostile SSH _agent_ if data of that type is to be sent on purpose, and an untrusted SSH agent is not part of the normal security model!) --- pageant.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/pageant.c b/pageant.c index 68bfab53..31a5540c 100644 --- a/pageant.c +++ b/pageant.c @@ -1344,8 +1344,11 @@ int pageant_add_keyfile(Filename *filename, const char *passphrase, *retstr = dupstr("Received broken key list from agent"); return PAGEANT_ACTION_FAILURE; } - n = toint(4 + GET_32BIT(p)); - if (n < 0 || keylistlen < n) { + n = GET_32BIT(p); + p += 4; + keylistlen -= 4; + + if (n < 0 || n > keylistlen) { *retstr = dupstr("Received broken key list from agent"); return PAGEANT_ACTION_FAILURE; } @@ -1359,8 +1362,11 @@ int pageant_add_keyfile(Filename *filename, const char *passphrase, *retstr = dupstr("Received broken key list from agent"); return PAGEANT_ACTION_FAILURE; } - n = toint(4 + GET_32BIT(p)); - if (n < 0 || keylistlen < n) { + n = GET_32BIT(p); + p += 4; + keylistlen -= 4; + + if (n < 0 || n > keylistlen) { *retstr = dupstr("Received broken key list from agent"); return PAGEANT_ACTION_FAILURE; }