From 2ff78b7fdaa31440928bf3ec0348ac98705f47d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mois=C3=A9s=20Guimar=C3=A3es?= Date: Tue, 11 Feb 2014 11:24:11 -0300 Subject: [PATCH] Boundaries check for client hello parsing. -- INCOMPLETE DATA checked only once with hello size against buffer size -- BUFFER_ERROR returned in case of message overflow (piece larger than the hello size) -- OPAQUE16_LEN used whenever 2 bytes are needed. -- Session id checking improved. TLS extensions return codes fixed. --- src/internal.c | 129 +++++++++++++++++++++++++++++++++---------------- src/tls.c | 62 +++++++++++------------- 2 files changed, 114 insertions(+), 77 deletions(-) diff --git a/src/internal.c b/src/internal.c index 04c516389..e15249829 100644 --- a/src/internal.c +++ b/src/internal.c @@ -9988,30 +9988,39 @@ static void PickHashSigAlgo(CYASSL* ssl, static int DoClientHello(CYASSL* ssl, const byte* input, word32* inOutIdx, word32 totalSz, word32 helloSz) { - byte b; + byte b; ProtocolVersion pv; Suites clSuites; - word32 i = *inOutIdx; - word32 begin = i; + word32 i = *inOutIdx; + word32 begin = i; #ifdef CYASSL_CALLBACKS if (ssl->hsInfoOn) AddPacketName("ClientHello", &ssl->handShakeInfo); if (ssl->toInfoOn) AddLateName("ClientHello", &ssl->timeoutInfo); #endif - /* make sure can read up to session */ - if (i + sizeof(pv) + RAN_LEN + ENUM_LEN > totalSz) + + /* make sure can read the client hello */ + if (begin + helloSz > totalSz) return INCOMPLETE_DATA; - XMEMCPY(&pv, input + i, sizeof(pv)); + /* protocol version, random and session id length check */ + if ((i - begin) + OPAQUE16_LEN + RAN_LEN + ENUM_LEN > helloSz) + return BUFFER_ERROR; + + /* protocol version */ + XMEMCPY(&pv, input + i, OPAQUE16_LEN); ssl->chVersion = pv; /* store */ - i += (word32)sizeof(pv); + i += OPAQUE16_LEN; + if (ssl->version.minor > pv.minor) { byte haveRSA = 0; byte havePSK = 0; + if (!ssl->options.downgrade) { CYASSL_MSG("Client trying to connect with lesser version"); return VERSION_ERROR; } + if (pv.minor == SSLv3_MINOR) { /* turn off tls */ CYASSL_MSG(" downgrading to SSLv3"); @@ -10040,6 +10049,7 @@ static void PickHashSigAlgo(CYASSL* ssl, ssl->options.haveECDSAsig, ssl->options.haveStaticECC, ssl->options.side); } + /* random */ XMEMCPY(ssl->arrays->clientRandom, input + i, RAN_LEN); i += RAN_LEN; @@ -10053,79 +10063,103 @@ static void PickHashSigAlgo(CYASSL* ssl, printf("\n"); } #endif + /* session id */ b = input[i++]; - if (b) { - if (i + ID_LEN > totalSz) - return INCOMPLETE_DATA; + + if (b == ID_LEN) { + if ((i - begin) + ID_LEN > helloSz) + return BUFFER_ERROR; + XMEMCPY(ssl->arrays->sessionID, input + i, ID_LEN); - i += b; - ssl->options.resuming= 1; /* client wants to resume */ + i += ID_LEN; + ssl->options.resuming = 1; /* client wants to resume */ CYASSL_MSG("Client wants to resume session"); } + else if (b) + return BUFFER_ERROR; /* session ID nor 0 neither 32 bytes long */ #ifdef CYASSL_DTLS /* cookie */ if (ssl->options.dtls) { + + if ((i - begin) + ENUM_LEN > helloSz) + return BUFFER_ERROR; + b = input[i++]; + if (b) { byte cookie[MAX_COOKIE_LEN]; if (b > MAX_COOKIE_LEN) return BUFFER_ERROR; - if (i + b > totalSz) - return INCOMPLETE_DATA; + + if ((i - begin) + b > helloSz) + return BUFFER_ERROR; + if (ssl->ctx->CBIOCookie == NULL) { CYASSL_MSG("Your Cookie callback is null, please set"); return COOKIE_ERROR; } + if ((ssl->ctx->CBIOCookie(ssl, cookie, COOKIE_SZ, ssl->IOCB_CookieCtx) != COOKIE_SZ) || (b != COOKIE_SZ) || (XMEMCMP(cookie, input + i, b) != 0)) { return COOKIE_ERROR; } + i += b; } } #endif - if (i + LENGTH_SZ > totalSz) - return INCOMPLETE_DATA; /* suites */ - ato16(&input[i], &clSuites.suiteSz); - i += 2; + if ((i - begin) + OPAQUE16_LEN > helloSz) + return BUFFER_ERROR; + + ato16(&input[i], &clSuites.suiteSz); + i += OPAQUE16_LEN; + + /* suites and compression length check */ + if ((i - begin) + clSuites.suiteSz + ENUM_LEN > helloSz) + return BUFFER_ERROR; - /* suites and comp len */ - if (i + clSuites.suiteSz + ENUM_LEN > totalSz) - return INCOMPLETE_DATA; if (clSuites.suiteSz > MAX_SUITE_SZ) return BUFFER_ERROR; + XMEMCPY(clSuites.suites, input + i, clSuites.suiteSz); i += clSuites.suiteSz; clSuites.hashSigAlgoSz = 0; - b = input[i++]; /* comp len */ - if (i + b > totalSz) - return INCOMPLETE_DATA; + /* compression length */ + b = input[i++]; + + if ((i - begin) + b > helloSz) + return BUFFER_ERROR; if (ssl->options.usingCompression) { int match = 0; + while (b--) { byte comp = input[i++]; + if (comp == ZLIB_COMPRESSION) match = 1; } + if (!match) { CYASSL_MSG("Not matching compression, turning off"); ssl->options.usingCompression = 0; /* turn off */ } } else - i += b; /* ignore, since we're not on */ + i += b; /* ignore, since we're not on */ *inOutIdx = i; - if ( (i - begin) < helloSz) { + + /* tls extensions */ + if ((i - begin) < helloSz) { #ifdef HAVE_TLS_EXTENSIONS if (IsTLS(ssl)) { int ret = 0; @@ -10135,10 +10169,14 @@ static void PickHashSigAlgo(CYASSL* ssl, /* Process the hello extension. Skip unsupported. */ word16 totalExtSz; + if ((i - begin) + OPAQUE16_LEN > helloSz) + return BUFFER_ERROR; + ato16(&input[i], &totalExtSz); - i += LENGTH_SZ; - if (totalExtSz > helloSz + begin - i) - return INCOMPLETE_DATA; + i += OPAQUE16_LEN; + + if ((i - begin) + totalExtSz > helloSz) + return BUFFER_ERROR; #ifdef HAVE_TLS_EXTENSIONS if ((ret = TLSX_Parse(ssl, (byte *) input + i, @@ -10149,19 +10187,24 @@ static void PickHashSigAlgo(CYASSL* ssl, #else while (totalExtSz) { word16 extId, extSz; + + if (OPAQUE16_LEN + OPAQUE16_LEN > totalExtSz) + return BUFFER_ERROR; ato16(&input[i], &extId); - i += LENGTH_SZ; + i += OPAQUE16_LEN; ato16(&input[i], &extSz); - i += EXT_ID_SZ; - if (extSz > totalExtSz - LENGTH_SZ - EXT_ID_SZ) - return INCOMPLETE_DATA; + i += OPAQUE16_LEN; + + if (OPAQUE16_LEN + OPAQUE16_LEN + extSz > totalExtSz) + return BUFFER_ERROR; if (extId == HELLO_EXT_SIG_ALGO) { ato16(&input[i], &clSuites.hashSigAlgoSz); - i += LENGTH_SZ; - if (clSuites.hashSigAlgoSz > extSz - LENGTH_SZ) - return INCOMPLETE_DATA; + i += OPAQUE16_LEN; + + if (OPAQUE16_LEN + clSuites.hashSigAlgoSz > extSz) + return BUFFER_ERROR; XMEMCPY(clSuites.hashSigAlgo, &input[i], min(clSuites.hashSigAlgoSz, HELLO_EXT_SIGALGO_MAX)); @@ -10170,27 +10213,29 @@ static void PickHashSigAlgo(CYASSL* ssl, else i += extSz; - totalExtSz -= LENGTH_SZ + EXT_ID_SZ + extSz; + totalExtSz -= OPAQUE16_LEN + OPAQUE16_LEN + extSz; } #endif *inOutIdx = i; } else - *inOutIdx = begin + helloSz; /* skip extensions */ + *inOutIdx = begin + helloSz; /* skip extensions */ } - ssl->options.clientState = CLIENT_HELLO_COMPLETE; - + ssl->options.clientState = CLIENT_HELLO_COMPLETE; ssl->options.haveSessionId = 1; + /* ProcessOld uses same resume code */ if (ssl->options.resuming && (!ssl->options.dtls || - ssl->options.acceptState == HELLO_VERIFY_SENT)) { /* let's try */ + ssl->options.acceptState == HELLO_VERIFY_SENT)) { /* let's try */ int ret = -1; CYASSL_SESSION* session = GetSession(ssl,ssl->arrays->masterSecret); + if (!session) { CYASSL_MSG("Session lookup for resume failed"); ssl->options.resuming = 0; - } else { + } + else { if (MatchSuite(ssl, &clSuites) < 0) { CYASSL_MSG("Unsupported cipher suite, ClientHello"); return UNSUPPORTED_SUITE; diff --git a/src/tls.c b/src/tls.c index 6f4d4e6cb..02d69a57d 100644 --- a/src/tls.c +++ b/src/tls.c @@ -721,46 +721,38 @@ static int TLSX_SNI_Parse(CYASSL* ssl, byte* input, word16 length, if (!extension) extension = TLSX_Find(ssl->ctx->extensions, SERVER_NAME_INDICATION); - if (!extension || !extension->data) { - if (!isRequest) { - CYASSL_MSG("Unexpected SNI response from server"); - } + if (!extension || !extension->data) + return isRequest ? 0 : BUFFER_ERROR; /* not using SNI OR unexpected + SNI response from server. */ - return 0; /* not using SNI */ - } - - if (!isRequest) { - if (length) { - CYASSL_MSG("SNI response should be empty!"); - } - - return 0; /* nothing to do */ - } + if (!isRequest) + return length ? BUFFER_ERROR : 0; /* SNI response must be empty! + Nothing else to do. */ #ifndef NO_CYASSL_SERVER if (OPAQUE16_LEN > length) - return INCOMPLETE_DATA; + return BUFFER_ERROR; ato16(input, &size); offset += OPAQUE16_LEN; /* validating sni list length */ if (length != OPAQUE16_LEN + size) - return INCOMPLETE_DATA; + return BUFFER_ERROR; for (size = 0; offset < length; offset += size) { SNI *sni; byte type = input[offset++]; if (offset + OPAQUE16_LEN > length) - return INCOMPLETE_DATA; + return BUFFER_ERROR; ato16(input + offset, &size); offset += OPAQUE16_LEN; if (offset + size > length) - return INCOMPLETE_DATA; + return BUFFER_ERROR; if (!(sni = TLSX_SNI_Find((SNI *) extension->data, type))) { continue; /* not using this SNI type */ @@ -905,34 +897,34 @@ int TLSX_SNI_GetFromBuffer(const byte* clientHello, word32 helloSz, offset += HANDSHAKE_HEADER_SZ; if (offset + len32 > helloSz) - return INCOMPLETE_DATA; + return BUFFER_ERROR; /* client hello */ offset += VERSION_SZ + RAN_LEN; /* version, random */ if (helloSz < offset + clientHello[offset]) - return INCOMPLETE_DATA; + return BUFFER_ERROR; offset += ENUM_LEN + clientHello[offset]; /* skip session id */ /* cypher suites */ if (helloSz < offset + OPAQUE16_LEN) - return INCOMPLETE_DATA; + return BUFFER_ERROR; ato16(clientHello + offset, &len16); offset += OPAQUE16_LEN; if (helloSz < offset + len16) - return INCOMPLETE_DATA; + return BUFFER_ERROR; offset += len16; /* skip cypher suites */ /* compression methods */ if (helloSz < offset + 1) - return INCOMPLETE_DATA; + return BUFFER_ERROR; if (helloSz < offset + clientHello[offset]) - return INCOMPLETE_DATA; + return BUFFER_ERROR; offset += ENUM_LEN + clientHello[offset]; /* skip compression methods */ @@ -944,7 +936,7 @@ int TLSX_SNI_GetFromBuffer(const byte* clientHello, word32 helloSz, offset += OPAQUE16_LEN; if (helloSz < offset + len16) - return INCOMPLETE_DATA; + return BUFFER_ERROR; while (len16 > OPAQUE16_LEN + OPAQUE16_LEN) { word16 extType; @@ -957,7 +949,7 @@ int TLSX_SNI_GetFromBuffer(const byte* clientHello, word32 helloSz, offset += OPAQUE16_LEN; if (helloSz < offset + extLen) - return INCOMPLETE_DATA; + return BUFFER_ERROR; if (extType != SERVER_NAME_INDICATION) { offset += extLen; /* skip extension */ @@ -968,7 +960,7 @@ int TLSX_SNI_GetFromBuffer(const byte* clientHello, word32 helloSz, offset += OPAQUE16_LEN; if (helloSz < offset + listLen) - return INCOMPLETE_DATA; + return BUFFER_ERROR; while (listLen > ENUM_LEN + OPAQUE16_LEN) { byte sniType = clientHello[offset++]; @@ -978,7 +970,7 @@ int TLSX_SNI_GetFromBuffer(const byte* clientHello, word32 helloSz, offset += OPAQUE16_LEN; if (helloSz < offset + sniLen) - return INCOMPLETE_DATA; + return BUFFER_ERROR; if (sniType != type) { offset += sniLen; @@ -1028,7 +1020,7 @@ static int TLSX_MFL_Parse(CYASSL* ssl, byte* input, word16 length, byte isRequest) { if (length != ENUM_LEN) - return INCOMPLETE_DATA; + return BUFFER_ERROR; switch (*input) { case CYASSL_MFL_2_9 : ssl->max_fragment = 512; break; @@ -1135,7 +1127,7 @@ static int TLSX_THM_Parse(CYASSL* ssl, byte* input, word16 length, byte isRequest) { if (length != 0 || input == NULL) - return INCOMPLETE_DATA; + return BUFFER_ERROR; #ifndef NO_CYASSL_SERVER if (isRequest) { @@ -1258,13 +1250,13 @@ static int TLSX_EllipticCurve_Parse(CYASSL* ssl, byte* input, word16 length, (void) isRequest; /* shut up compiler! */ if (OPAQUE16_LEN > length || length % OPAQUE16_LEN) - return INCOMPLETE_DATA; + return BUFFER_ERROR; ato16(input, &offset); /* validating curve list length */ if (length != OPAQUE16_LEN + offset) - return INCOMPLETE_DATA; + return BUFFER_ERROR; while (offset) { ato16(input + offset, &name); @@ -1705,7 +1697,7 @@ int TLSX_Parse(CYASSL* ssl, byte* input, word16 length, byte isRequest, word16 size; if (length - offset < HELLO_EXT_TYPE_SZ + OPAQUE16_LEN) - return INCOMPLETE_DATA; + return BUFFER_ERROR; ato16(input + offset, &type); offset += HELLO_EXT_TYPE_SZ; @@ -1714,7 +1706,7 @@ int TLSX_Parse(CYASSL* ssl, byte* input, word16 length, byte isRequest, offset += OPAQUE16_LEN; if (offset + size > length) - return INCOMPLETE_DATA; + return BUFFER_ERROR; switch (type) { case SERVER_NAME_INDICATION: @@ -1748,7 +1740,7 @@ int TLSX_Parse(CYASSL* ssl, byte* input, word16 length, byte isRequest, ato16(input + offset, &suites->hashSigAlgoSz); if (suites->hashSigAlgoSz > size - OPAQUE16_LEN) - return INCOMPLETE_DATA; + return BUFFER_ERROR; XMEMCPY(suites->hashSigAlgo, input + offset + OPAQUE16_LEN,