From 5285132db93658ce182122444cb26322032a51ea Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 7 Oct 2020 16:21:22 -0700 Subject: [PATCH 1/2] Get Size 1. Add a function GetSize() that calls GetUint32() then checks that the value read in plus the data index is still less than the data length. 2. Replaced a few checks of the size of some data with calls to GetSize(). Included are public key type length, public key length, and the signature length in DoUserAuthPublicKey(). --- src/internal.c | 38 ++++++++++++++++++-------------------- wolfssh/internal.h | 1 + 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/internal.c b/src/internal.c index f6f73d1..438572e 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1770,6 +1770,21 @@ int GetUint32(word32* v, const byte* buf, word32 len, word32* idx) } +int GetSize(word32* v, const byte* buf, word32 len, word32* idx) +{ + int result; + + result = GetUint32(v, buf, len, idx); + if (result == WS_SUCCESS) { + if (*v + *idx > len) { + result = WS_BUFFER_E; + } + } + + return result; +} + + /* Gets the size of the mpint, and puts the pointer to the start of * buf's number into *mpint. This function does not copy. */ int GetMpint(word32* mpintSz, byte** mpint, byte* buf, word32 len, word32* idx) @@ -4140,24 +4155,12 @@ static int DoUserAuthRequestPublicKey(WOLFSSH* ssh, WS_UserAuthData* authData, } if (ret == WS_SUCCESS) - ret = GetUint32(&pk->publicKeyTypeSz, buf, len, &begin); - - if (ret == WS_SUCCESS) { - if (pk->publicKeyTypeSz > len - begin) { - ret = WS_BUFFER_E; - } - } + ret = GetSize(&pk->publicKeyTypeSz, buf, len, &begin); if (ret == WS_SUCCESS) { pk->publicKeyType = buf + begin; begin += pk->publicKeyTypeSz; - ret = GetUint32(&pk->publicKeySz, buf, len, &begin); - } - - if (ret == WS_SUCCESS) { - if (pk->publicKeySz > len - begin) { - ret = WS_BUFFER_E; - } + ret = GetSize(&pk->publicKeySz, buf, len, &begin); } if (ret == WS_SUCCESS) { @@ -4165,12 +4168,7 @@ static int DoUserAuthRequestPublicKey(WOLFSSH* ssh, WS_UserAuthData* authData, begin += pk->publicKeySz; if (pk->hasSignature) { - ret = GetUint32(&pk->signatureSz, buf, len, &begin); - if (ret == WS_SUCCESS) { - if (pk->signatureSz > len - begin) { - ret = WS_BUFFER_E; - } - } + ret = GetSize(&pk->signatureSz, buf, len, &begin); if (ret == WS_SUCCESS) { pk->signature = buf + begin; begin += pk->signatureSz; diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 683a440..2889c56 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -706,6 +706,7 @@ WOLFSSH_LOCAL int wolfSSH_ProcessBuffer(WOLFSSH_CTX*, /* Parsing functions */ WOLFSSH_LOCAL int GetBoolean(byte*, byte*, word32, word32*); WOLFSSH_LOCAL int GetUint32(word32*, const byte*, word32, word32*); +WOLFSSH_LOCAL int GetSize(word32*, const byte*, word32, word32*); WOLFSSH_LOCAL int GetMpint(word32*, byte**, byte*, word32, word32*); WOLFSSH_LOCAL int GetString(char*, word32*, byte*, word32, word32*); WOLFSSH_LOCAL int GetStringAlloc(void*, char**, byte*, word32, word32*); From 4a518018e09f11e2460661325f83b37f8cf4c768 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 3 Feb 2021 11:44:12 -0800 Subject: [PATCH 2/2] Get Size 1. Revise the bounds check in GetString() to match the old bounds checks. 2. Replace the last few instances of getting the length of a SSH string and checking it by hand with calls to GetString(). --- src/internal.c | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-) diff --git a/src/internal.c b/src/internal.c index 438572e..2772594 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1776,7 +1776,7 @@ int GetSize(word32* v, const byte* buf, word32 len, word32* idx) result = GetUint32(v, buf, len, idx); if (result == WS_SUCCESS) { - if (*v + *idx > len) { + if (*v > len - *idx) { result = WS_BUFFER_E; } } @@ -4315,13 +4315,7 @@ static int DoUserAuthRequest(WOLFSSH* ssh, if (ret == WS_SUCCESS) { begin = *idx; WMEMSET(&authData, 0, sizeof(authData)); - ret = GetUint32(&authData.usernameSz, buf, len, &begin); - } - - if (ret == WS_SUCCESS) { - if (authData.usernameSz > len - begin) { - ret = WS_BUFFER_E; - } + ret = GetSize(&authData.usernameSz, buf, len, &begin); } if (ret == WS_SUCCESS) { @@ -4341,13 +4335,7 @@ static int DoUserAuthRequest(WOLFSSH* ssh, authData.serviceName = buf + begin; begin += authData.serviceNameSz; - ret = GetUint32(&authData.authNameSz, buf, len, &begin); - } - - if (ret == WS_SUCCESS) { - if (authData.authNameSz > len - begin) { - ret = WS_BUFFER_E; - } + ret = GetSize(&authData.authNameSz, buf, len, &begin); } if (ret == WS_SUCCESS) { @@ -4483,7 +4471,7 @@ static int DoUserAuthBanner(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) ret = GetString(banner, &bannerSz, buf, len, idx); if (ret == WS_SUCCESS) - ret = GetUint32(&bannerSz, buf, len, idx); + ret = GetSize(&bannerSz, buf, len, idx); if (ret == WS_SUCCESS) { if (ssh->ctx->showBanner) { @@ -4772,7 +4760,7 @@ static int DoChannelOpenFail(WOLFSSH* ssh, } if (ret == WS_SUCCESS) - ret = GetUint32(&langSz, buf, len, &begin); + ret = GetSize(&langSz, buf, len, &begin); if (ret == WS_SUCCESS) { *idx = begin + langSz; @@ -5075,11 +5063,11 @@ static int DoChannelData(WOLFSSH* ssh, ret = GetUint32(&channelId, buf, len, &begin); if (ret == WS_SUCCESS) - ret = GetUint32(&dataSz, buf, len, &begin); + ret = GetSize(&dataSz, buf, len, &begin); /* Validate dataSz */ if (ret == WS_SUCCESS) { - if ((len < begin) || (dataSz > len - begin)) { + if (len < begin) { ret = WS_RECV_OVERFLOW_E; } } @@ -5145,12 +5133,7 @@ static int DoChannelExtendedData(WOLFSSH* ssh, ret = (dataTypeCode == CHANNEL_EXTENDED_DATA_STDERR) ? WS_SUCCESS : WS_INVALID_EXTDATA; if (ret == WS_SUCCESS) - ret = GetUint32(&dataSz, buf, len, &begin); - if (ret == WS_SUCCESS) { - if (dataSz > (len - begin)) { - ret = WS_BUFFER_E; - } - } + ret = GetSize(&dataSz, buf, len, &begin); if (ret == WS_SUCCESS) { channel = ChannelFind(ssh, channelId, WS_CHANNEL_ID_SELF);