From 8045316e70900a1584f0c3af2749cc831f00c1c7 Mon Sep 17 00:00:00 2001 From: Tim Taubert Date: Mon, 28 Nov 2016 10:36:35 +0100 Subject: [PATCH] Bug 1320326 - Make ssl3_ConsumeHandshakeNumber() return a SECStatus and take a pointer argument r=mt Differential Revision: https://nss-review.dev.mozaws.net/D97 --- lib/ssl/ssl3con.c | 165 ++++++++++++++++++++------------------- lib/ssl/ssl3ext.c | 20 ++--- lib/ssl/ssl3ext.h | 9 ++- lib/ssl/ssl3exthandle.c | 120 ++++++++++++++-------------- lib/ssl/sslimpl.h | 9 ++- lib/ssl/tls13con.c | 25 +++--- lib/ssl/tls13exthandle.c | 27 ++++--- 7 files changed, 192 insertions(+), 183 deletions(-) diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c index b9a5d41e2c..80a37068f4 100644 --- a/lib/ssl/ssl3con.c +++ b/lib/ssl/ssl3con.c @@ -1088,10 +1088,11 @@ ssl_ClientReadVersion(sslSocket *ss, SSL3Opaque **b, unsigned int *len, SSL3ProtocolVersion *version) { SSL3ProtocolVersion v; - PRInt32 temp; + PRUint32 temp; + SECStatus rv; - temp = ssl3_ConsumeHandshakeNumber(ss, 2, b, len); - if (temp < 0) { + rv = ssl3_ConsumeHandshakeNumber(ss, &temp, 2, b, len); + if (rv != SECSuccess) { return SECFailure; /* alert has been sent */ } @@ -4311,7 +4312,7 @@ ssl3_AppendHandshakeHeader(sslSocket *ss, SSL3HandshakeType t, PRUint32 length) * override the generic error code by setting another. */ SECStatus -ssl3_ConsumeHandshake(sslSocket *ss, void *v, PRInt32 bytes, SSL3Opaque **b, +ssl3_ConsumeHandshake(sslSocket *ss, void *v, PRUint32 bytes, SSL3Opaque **b, PRUint32 *length) { PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss)); @@ -4329,37 +4330,33 @@ ssl3_ConsumeHandshake(sslSocket *ss, void *v, PRInt32 bytes, SSL3Opaque **b, /* Read up the next "bytes" number of bytes from the (decrypted) input * stream "b" (which is *length bytes long), and interpret them as an - * integer in network byte order. Returns the received value. + * integer in network byte order. Sets *num to the received value. * Reduces *length by bytes. Advances *b by bytes. * - * Returns SECFailure (-1) on failure. - * This value is indistinguishable from the equivalent received value. - * Only positive numbers are to be received this way. - * Thus, the largest value that may be sent this way is 0x7fffffff. * On error, an alert has been sent, and a generic error code has been set. */ -PRInt32 -ssl3_ConsumeHandshakeNumber(sslSocket *ss, PRInt32 bytes, SSL3Opaque **b, - PRUint32 *length) +SECStatus +ssl3_ConsumeHandshakeNumber(sslSocket *ss, PRUint32 *num, PRUint32 bytes, + SSL3Opaque **b, PRUint32 *length) { PRUint8 *buf = *b; int i; - PRInt32 num = 0; PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss)); PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss)); - PORT_Assert(bytes <= sizeof num); - if ((PRUint32)bytes > *length) { + *num = 0; + if (bytes > *length || bytes > sizeof(*num)) { return ssl3_DecodeError(ss); } PRINT_BUF(60, (ss, "consume bytes:", *b, bytes)); - for (i = 0; i < bytes; i++) - num = (num << 8) + buf[i]; + for (i = 0; i < bytes; i++) { + *num = (*num << 8) + buf[i]; + } *b += bytes; *length -= bytes; - return num; + return SECSuccess; } /* Read in two values from the incoming decrypted byte stream "b", which is @@ -4377,21 +4374,22 @@ ssl3_ConsumeHandshakeNumber(sslSocket *ss, PRInt32 bytes, SSL3Opaque **b, * point to the values in the buffer **b. */ SECStatus -ssl3_ConsumeHandshakeVariable(sslSocket *ss, SECItem *i, PRInt32 bytes, +ssl3_ConsumeHandshakeVariable(sslSocket *ss, SECItem *i, PRUint32 bytes, SSL3Opaque **b, PRUint32 *length) { - PRInt32 count; + PRUint32 count; + SECStatus rv; PORT_Assert(bytes <= 3); i->len = 0; i->data = NULL; i->type = siBuffer; - count = ssl3_ConsumeHandshakeNumber(ss, bytes, b, length); - if (count < 0) { /* Can't test for SECSuccess here. */ + rv = ssl3_ConsumeHandshakeNumber(ss, &count, bytes, b, length); + if (rv != SECSuccess) { return SECFailure; } if (count > 0) { - if ((PRUint32)count > *length) { + if (count > *length) { return ssl3_DecodeError(ss); } i->data = *b; @@ -4662,10 +4660,11 @@ SECStatus ssl_ConsumeSignatureScheme(sslSocket *ss, SSL3Opaque **b, PRUint32 *length, SSLSignatureScheme *out) { - PRInt32 tmp; + PRUint32 tmp; + SECStatus rv; - tmp = ssl3_ConsumeHandshakeNumber(ss, 2, b, length); - if (tmp < 0) { + rv = ssl3_ConsumeHandshakeNumber(ss, &tmp, 2, b, length); + if (rv != SECSuccess) { return SECFailure; /* Error code set already. */ } if (!ssl_IsSupportedSignatureScheme((SSLSignatureScheme)tmp)) { @@ -6587,7 +6586,7 @@ ssl3_SetCipherSuite(sslSocket *ss, ssl3CipherSuite chosenSuite, static SECStatus ssl3_HandleServerHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length) { - PRInt32 temp; /* allow for consume number failure */ + PRUint32 temp; PRBool suite_found = PR_FALSE; int i; int errCode = SSL_ERROR_RX_MALFORMED_SERVER_HELLO; @@ -6702,8 +6701,8 @@ ssl3_HandleServerHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length) } /* find selected cipher suite in our list. */ - temp = ssl3_ConsumeHandshakeNumber(ss, 2, &b, &length); - if (temp < 0) { + rv = ssl3_ConsumeHandshakeNumber(ss, &temp, 2, &b, &length); + if (rv != SECSuccess) { goto loser; /* alert has been sent */ } i = ssl3_config_match_init(ss); @@ -6748,8 +6747,8 @@ ssl3_HandleServerHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length) if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3) { /* find selected compression method in our list. */ - temp = ssl3_ConsumeHandshakeNumber(ss, 1, &b, &length); - if (temp < 0) { + rv = ssl3_ConsumeHandshakeNumber(ss, &temp, 1, &b, &length); + if (rv != SECSuccess) { goto loser; /* alert has been sent */ } suite_found = PR_FALSE; @@ -7257,16 +7256,17 @@ SECStatus ssl3_ParseCertificateRequestCAs(sslSocket *ss, SSL3Opaque **b, PRUint32 *length, PLArenaPool *arena, CERTDistNames *ca_list) { - PRInt32 remaining; + PRUint32 remaining; int nnames = 0; dnameNode *node; + SECStatus rv; int i; - remaining = ssl3_ConsumeHandshakeNumber(ss, 2, b, length); - if (remaining < 0) + rv = ssl3_ConsumeHandshakeNumber(ss, &remaining, 2, b, length); + if (rv != SECSuccess) return SECFailure; /* malformed, alert has been sent */ - if ((PRUint32)remaining > *length) + if (remaining > *length) goto alert_loser; ca_list->head = node = PORT_ArenaZNew(arena, dnameNode); @@ -7274,19 +7274,19 @@ ssl3_ParseCertificateRequestCAs(sslSocket *ss, SSL3Opaque **b, PRUint32 *length, goto no_mem; while (remaining > 0) { - PRInt32 len; + PRUint32 len; if (remaining < 2) goto alert_loser; /* malformed */ - node->name.len = len = ssl3_ConsumeHandshakeNumber(ss, 2, b, length); - if (len <= 0) + rv = ssl3_ConsumeHandshakeNumber(ss, &len, 2, b, length); + if (rv != SECSuccess) return SECFailure; /* malformed, alert has been sent */ - - remaining -= 2; - if (remaining < len) + if (len == 0 || remaining < len + 2) goto alert_loser; /* malformed */ + remaining -= 2; + node->name.len = len; node->name.data = *b; *b += len; *length -= len; @@ -7362,9 +7362,9 @@ ssl_ParseSignatureSchemes(const sslSocket *ss, PLArenaPool *arena, } for (; max; --max) { - PRInt32 tmp; - tmp = ssl3_ExtConsumeHandshakeNumber(ss, 2, &buf.data, &buf.len); - if (tmp < 0) { + PRUint32 tmp; + rv = ssl3_ExtConsumeHandshakeNumber(ss, &tmp, 2, &buf.data, &buf.len); + if (rv != SECSuccess) { PORT_Assert(0); PORT_SetError(SEC_ERROR_LIBRARY_FAILURE); return SECFailure; @@ -8228,7 +8228,7 @@ static SECStatus ssl3_HandleClientHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length) { sslSessionID *sid = NULL; - PRInt32 tmp; + PRUint32 tmp; unsigned int i; SECStatus rv; int errCode = SSL_ERROR_RX_MALFORMED_CLIENT_HELLO; @@ -8288,8 +8288,8 @@ ssl3_HandleClientHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length) dtls_RehandshakeCleanup(ss); } - tmp = ssl3_ConsumeHandshakeNumber(ss, 2, &b, &length); - if (tmp < 0) + rv = ssl3_ConsumeHandshakeNumber(ss, &tmp, 2, &b, &length); + if (rv != SECSuccess) goto loser; /* malformed, alert already sent */ /* Translate the version. */ @@ -8342,9 +8342,9 @@ ssl3_HandleClientHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length) if (length) { /* Get length of hello extensions */ - PRInt32 extension_length; - extension_length = ssl3_ConsumeHandshakeNumber(ss, 2, &b, &length); - if (extension_length < 0) { + PRUint32 extension_length; + rv = ssl3_ConsumeHandshakeNumber(ss, &extension_length, 2, &b, &length); + if (rv != SECSuccess) { goto loser; /* alert already sent */ } if (extension_length != length) { @@ -9896,9 +9896,9 @@ ssl3_HandleRSAClientKeyExchange(sslSocket *ss, enc_pms.len = length; if (ss->ssl3.prSpec->version > SSL_LIBRARY_VERSION_3_0) { /* isTLS */ - PRInt32 kLen; - kLen = ssl3_ConsumeHandshakeNumber(ss, 2, &enc_pms.data, &enc_pms.len); - if (kLen < 0) { + PRUint32 kLen; + rv = ssl3_ConsumeHandshakeNumber(ss, &kLen, 2, &enc_pms.data, &enc_pms.len); + if (rv != SECSuccess) { PORT_SetError(SSL_ERROR_CLIENT_KEY_EXCHANGE_FAILURE); return SECFailure; } @@ -10218,6 +10218,7 @@ ssl3_HandleNewSessionTicket(sslSocket *ss, SSL3Opaque *b, PRUint32 length) { SECStatus rv; SECItem ticketData; + PRUint32 temp; SSL_TRC(3, ("%d: SSL3[%d]: handle session_ticket handshake", SSL_GETPID(), ss->fd)); @@ -10244,8 +10245,13 @@ ssl3_HandleNewSessionTicket(sslSocket *ss, SSL3Opaque *b, PRUint32 length) PORT_SetError(SSL_ERROR_RX_MALFORMED_NEW_SESSION_TICKET); return SECFailure; } - ss->ssl3.hs.newSessionTicket.ticket_lifetime_hint = - (PRUint32)ssl3_ConsumeHandshakeNumber(ss, 4, &b, &length); + + rv = ssl3_ConsumeHandshakeNumber(ss, &temp, 4, &b, &length); + if (rv != SECSuccess) { + PORT_SetError(SSL_ERROR_RX_MALFORMED_NEW_SESSION_TICKET); + return SECFailure; + } + ss->ssl3.hs.newSessionTicket.ticket_lifetime_hint = temp; rv = ssl3_ConsumeHandshakeVariable(ss, &ticketData, 2, &b, &length); if (rv != SECSuccess || length != 0) { @@ -10540,21 +10546,20 @@ ssl3_HandleCertificateStatus(sslSocket *ss, SSL3Opaque *b, PRUint32 length) SECStatus ssl_ReadCertificateStatus(sslSocket *ss, SSL3Opaque *b, PRUint32 length) { - PRInt32 status, len; + PRUint32 status, len; + SECStatus rv; PORT_Assert(!ss->sec.isServer); /* Consume the CertificateStatusType enum */ - status = ssl3_ConsumeHandshakeNumber(ss, 1, &b, &length); - if (status != 1 /* ocsp */) { - ssl3_DecodeError(ss); /* sets error code */ - return SECFailure; + rv = ssl3_ConsumeHandshakeNumber(ss, &status, 1, &b, &length); + if (rv != SECSuccess || status != 1 /* ocsp */) { + return ssl3_DecodeError(ss); } - len = ssl3_ConsumeHandshakeNumber(ss, 3, &b, &length); - if (len != length) { - ssl3_DecodeError(ss); /* sets error code */ - return SECFailure; + rv = ssl3_ConsumeHandshakeNumber(ss, &len, 3, &b, &length); + if (rv != SECSuccess || len != length) { + return ssl3_DecodeError(ss); } #define MAX_CERTSTATUS_LEN 0x1ffff /* 128k - 1 */ @@ -10611,8 +10616,8 @@ ssl3_CompleteHandleCertificate(sslSocket *ss, SSL3Opaque *b, PRUint32 length) { ssl3CertNode *c; ssl3CertNode *lastCert = NULL; - PRInt32 remaining = 0; - PRInt32 size; + PRUint32 remaining = 0; + PRUint32 size; SECStatus rv; PRBool isServer = ss->sec.isServer; PRBool isTLS; @@ -10628,10 +10633,10 @@ ssl3_CompleteHandleCertificate(sslSocket *ss, SSL3Opaque *b, PRUint32 length) ** normal no_certificates message to maximize interoperability. */ if (length) { - remaining = ssl3_ConsumeHandshakeNumber(ss, 3, &b, &length); - if (remaining < 0) + rv = ssl3_ConsumeHandshakeNumber(ss, &remaining, 3, &b, &length); + if (rv != SECSuccess) goto loser; /* fatal alert already sent by ConsumeHandshake. */ - if ((PRUint32)remaining > length) + if (remaining > length) goto decode_loser; } @@ -10662,15 +10667,14 @@ ssl3_CompleteHandleCertificate(sslSocket *ss, SSL3Opaque *b, PRUint32 length) } /* First get the peer cert. */ - remaining -= 3; - if (remaining < 0) + if (remaining < 3) goto decode_loser; - size = ssl3_ConsumeHandshakeNumber(ss, 3, &b, &length); - if (size <= 0) + remaining -= 3; + rv = ssl3_ConsumeHandshakeNumber(ss, &size, 3, &b, &length); + if (rv != SECSuccess) goto loser; /* fatal alert already sent by ConsumeHandshake. */ - - if (remaining < size) + if (size == 0 || remaining < size) goto decode_loser; certItem.data = b; @@ -10690,15 +10694,14 @@ ssl3_CompleteHandleCertificate(sslSocket *ss, SSL3Opaque *b, PRUint32 length) /* Now get all of the CA certs. */ while (remaining > 0) { - remaining -= 3; - if (remaining < 0) + if (remaining < 3) goto decode_loser; - size = ssl3_ConsumeHandshakeNumber(ss, 3, &b, &length); - if (size <= 0) + remaining -= 3; + rv = ssl3_ConsumeHandshakeNumber(ss, &size, 3, &b, &length); + if (rv != SECSuccess) goto loser; /* fatal alert already sent by ConsumeHandshake. */ - - if (remaining < size) + if (size == 0 || remaining < size) goto decode_loser; certItem.data = b; diff --git a/lib/ssl/ssl3ext.c b/lib/ssl/ssl3ext.c index 492096852a..2ccdc0a06a 100644 --- a/lib/ssl/ssl3ext.c +++ b/lib/ssl/ssl3ext.c @@ -171,15 +171,15 @@ ssl3_ParseExtensions(sslSocket *ss, SSL3Opaque **b, PRUint32 *length) while (*length) { SECStatus rv; - PRInt32 extension_type; + PRUint32 extension_type; SECItem extension_data = { siBuffer, NULL, 0 }; TLSExtension *extension; PRCList *cursor; /* Get the extension's type field */ - extension_type = ssl3_ConsumeHandshakeNumber(ss, 2, b, length); - if (extension_type < 0) { /* failure to decode extension_type */ - return SECFailure; /* alert already sent */ + rv = ssl3_ConsumeHandshakeNumber(ss, &extension_type, 2, b, length); + if (rv != SECSuccess) { + return SECFailure; /* alert already sent */ } SSL_TRC(10, ("%d: SSL3[%d]: parsing extension %d", @@ -505,22 +505,22 @@ ssl3_ExtDecodeError(const sslSocket *ss) } SECStatus -ssl3_ExtConsumeHandshake(const sslSocket *ss, void *v, PRInt32 bytes, +ssl3_ExtConsumeHandshake(const sslSocket *ss, void *v, PRUint32 bytes, SSL3Opaque **b, PRUint32 *length) { return ssl3_ConsumeHandshake((sslSocket *)ss, v, bytes, b, length); } -PRInt32 -ssl3_ExtConsumeHandshakeNumber(const sslSocket *ss, PRInt32 bytes, - SSL3Opaque **b, PRUint32 *length) +SECStatus +ssl3_ExtConsumeHandshakeNumber(const sslSocket *ss, PRUint32 *num, + PRUint32 bytes, SSL3Opaque **b, PRUint32 *length) { - return ssl3_ConsumeHandshakeNumber((sslSocket *)ss, bytes, b, length); + return ssl3_ConsumeHandshakeNumber((sslSocket *)ss, num, bytes, b, length); } SECStatus ssl3_ExtConsumeHandshakeVariable(const sslSocket *ss, SECItem *i, - PRInt32 bytes, SSL3Opaque **b, + PRUint32 bytes, SSL3Opaque **b, PRUint32 *length) { return ssl3_ConsumeHandshakeVariable((sslSocket *)ss, i, bytes, b, length); diff --git a/lib/ssl/ssl3ext.h b/lib/ssl/ssl3ext.h index f93ad65bdf..067b4f94c6 100644 --- a/lib/ssl/ssl3ext.h +++ b/lib/ssl/ssl3ext.h @@ -145,12 +145,13 @@ SECStatus ssl3_ExtAppendHandshakeVariable(const sslSocket *ss, void ssl3_ExtSendAlert(const sslSocket *ss, SSL3AlertLevel level, SSL3AlertDescription desc); void ssl3_ExtDecodeError(const sslSocket *ss); -SECStatus ssl3_ExtConsumeHandshake(const sslSocket *ss, void *v, PRInt32 bytes, +SECStatus ssl3_ExtConsumeHandshake(const sslSocket *ss, void *v, PRUint32 bytes, SSL3Opaque **b, PRUint32 *length); -PRInt32 ssl3_ExtConsumeHandshakeNumber(const sslSocket *ss, PRInt32 bytes, - SSL3Opaque **b, PRUint32 *length); +SECStatus ssl3_ExtConsumeHandshakeNumber(const sslSocket *ss, PRUint32 *num, + PRUint32 bytes, SSL3Opaque **b, + PRUint32 *length); SECStatus ssl3_ExtConsumeHandshakeVariable(const sslSocket *ss, SECItem *i, - PRInt32 bytes, SSL3Opaque **b, + PRUint32 bytes, SSL3Opaque **b, PRUint32 *length); #endif diff --git a/lib/ssl/ssl3exthandle.c b/lib/ssl/ssl3exthandle.c index 2a80e2690c..ec1dc8cff9 100644 --- a/lib/ssl/ssl3exthandle.c +++ b/lib/ssl/ssl3exthandle.c @@ -223,7 +223,8 @@ SECStatus ssl3_HandleServerNameXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRUint16 ex_type, SECItem *data) { SECItem *names = NULL; - PRInt32 listLenBytes = 0; + PRUint32 listLenBytes = 0; + SECStatus rv; if (!ss->sec.isServer) { return SECSuccess; /* ignore extension */ @@ -236,8 +237,8 @@ ssl3_HandleServerNameXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRUint1 } /* length of server_name_list */ - listLenBytes = ssl3_ExtConsumeHandshakeNumber(ss, 2, &data->data, &data->len); - if (listLenBytes < 0) { + rv = ssl3_ExtConsumeHandshakeNumber(ss, &listLenBytes, 2, &data->data, &data->len); + if (rv != SECSuccess) { goto loser; /* alert already sent */ } if (listLenBytes == 0 || listLenBytes != data->len) { @@ -247,12 +248,11 @@ ssl3_HandleServerNameXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRUint1 /* Read ServerNameList. */ while (data->len > 0) { SECItem tmp; - SECStatus rv; - PRInt32 type; + PRUint32 type; /* Read Name Type. */ - type = ssl3_ExtConsumeHandshakeNumber(ss, 1, &data->data, &data->len); - if (type < 0) { /* i.e., SECFailure cast to PRint32 */ + rv = ssl3_ExtConsumeHandshakeNumber(ss, &type, 1, &data->data, &data->len); + if (rv != SECSuccess) { /* alert sent in ConsumeHandshakeNumber */ goto loser; } @@ -542,7 +542,7 @@ ssl3_SelectAppProtocol(const sslSocket *ss, TLSExtensionData *xtnData, SECStatus ssl3_ServerHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRUint16 ex_type, SECItem *data) { - int count; + PRUint32 count; SECStatus rv; /* We expressly don't want to allow ALPN on renegotiation, @@ -556,8 +556,8 @@ ssl3_ServerHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRU /* Unlike NPN, ALPN has extra redundant length information so that * the extension is the same in both ClientHello and ServerHello. */ - count = ssl3_ExtConsumeHandshakeNumber(ss, 2, &data->data, &data->len); - if (count != data->len) { + rv = ssl3_ExtConsumeHandshakeNumber(ss, &count, 2, &data->data, &data->len); + if (rv != SECSuccess || count != data->len) { ssl3_ExtDecodeError(ss); return SECFailure; } @@ -621,7 +621,7 @@ SECStatus ssl3_ClientHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRUint16 ex_type, SECItem *data) { SECStatus rv; - PRInt32 list_len; + PRUint32 list_len; SECItem protocol_name; if (ssl3_ExtensionNegotiated(ss, ssl_next_proto_nego_xtn)) { @@ -639,9 +639,10 @@ ssl3_ClientHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRU return SECFailure; } - list_len = ssl3_ExtConsumeHandshakeNumber(ss, 2, &data->data, &data->len); + rv = ssl3_ExtConsumeHandshakeNumber(ss, &list_len, 2, &data->data, + &data->len); /* The list has to be the entire extension. */ - if (list_len != data->len) { + if (rv != SECSuccess || list_len != data->len) { ssl3_ExtSendAlert(ss, alert_fatal, decode_error); PORT_SetError(SSL_ERROR_NEXT_PROTOCOL_DATA_INVALID); return SECFailure; @@ -1363,9 +1364,9 @@ ssl3_ProcessSessionTicketCommon(sslSocket *ss, SECItem *data) PRUint32 padding_length; unsigned char *buffer; unsigned int buffer_len; - PRInt32 temp; + PRUint32 temp; SECItem cert_item; - PRInt8 nameType = TLS_STE_NO_SERVER_NAME; + PRUint32 nameType; SECItem macParam = { siBuffer, NULL, 0 }; SECItem alpn_item; SECItem ivItem; @@ -1499,52 +1500,52 @@ ssl3_ProcessSessionTicketCommon(sslSocket *ss, SECItem *data) } /* Read ticket_version and reject if the version is wrong */ - temp = ssl3_ExtConsumeHandshakeNumber(ss, 2, &buffer, &buffer_len); - if (temp != TLS_EX_SESS_TICKET_VERSION) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 2, &buffer, &buffer_len); + if (rv != SECSuccess || temp != TLS_EX_SESS_TICKET_VERSION) goto no_ticket; parsed_session_ticket->ticket_version = (SSL3ProtocolVersion)temp; /* Read SSLVersion. */ - temp = ssl3_ExtConsumeHandshakeNumber(ss, 2, &buffer, &buffer_len); - if (temp < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 2, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; parsed_session_ticket->ssl_version = (SSL3ProtocolVersion)temp; /* Read cipher_suite. */ - temp = ssl3_ExtConsumeHandshakeNumber(ss, 2, &buffer, &buffer_len); - if (temp < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 2, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; parsed_session_ticket->cipher_suite = (ssl3CipherSuite)temp; /* Read compression_method. */ - temp = ssl3_ExtConsumeHandshakeNumber(ss, 1, &buffer, &buffer_len); - if (temp < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 1, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; parsed_session_ticket->compression_method = (SSLCompressionMethod)temp; /* Read cipher spec parameters. */ - temp = ssl3_ExtConsumeHandshakeNumber(ss, 1, &buffer, &buffer_len); - if (temp < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 1, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; parsed_session_ticket->authType = (SSLAuthType)temp; - temp = ssl3_ExtConsumeHandshakeNumber(ss, 4, &buffer, &buffer_len); - if (temp < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 4, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; - parsed_session_ticket->authKeyBits = (PRUint32)temp; - temp = ssl3_ExtConsumeHandshakeNumber(ss, 1, &buffer, &buffer_len); - if (temp < 0) + parsed_session_ticket->authKeyBits = temp; + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 1, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; parsed_session_ticket->keaType = (SSLKEAType)temp; - temp = ssl3_ExtConsumeHandshakeNumber(ss, 4, &buffer, &buffer_len); - if (temp < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 4, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; - parsed_session_ticket->keaKeyBits = (PRUint32)temp; + parsed_session_ticket->keaKeyBits = temp; /* Read certificate slot */ parsed_session_ticket->certType.authType = parsed_session_ticket->authType; - temp = ssl3_ExtConsumeHandshakeNumber(ss, 1, &buffer, &buffer_len); - if (temp < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 1, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; switch (parsed_session_ticket->authType) { case ssl_auth_ecdsa: @@ -1562,18 +1563,18 @@ ssl3_ProcessSessionTicketCommon(sslSocket *ss, SECItem *data) } /* Read wrapped master_secret. */ - temp = ssl3_ExtConsumeHandshakeNumber(ss, 1, &buffer, &buffer_len); - if (temp < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 1, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; parsed_session_ticket->ms_is_wrapped = (PRBool)temp; - temp = ssl3_ExtConsumeHandshakeNumber(ss, 4, &buffer, &buffer_len); - if (temp < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 4, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; parsed_session_ticket->msWrapMech = (CK_MECHANISM_TYPE)temp; - temp = ssl3_ExtConsumeHandshakeNumber(ss, 2, &buffer, &buffer_len); - if (temp < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 2, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; parsed_session_ticket->ms_length = (PRUint16)temp; if (parsed_session_ticket->ms_length == 0 || /* sanity check MS. */ @@ -1590,8 +1591,8 @@ ssl3_ProcessSessionTicketCommon(sslSocket *ss, SECItem *data) buffer_len -= parsed_session_ticket->ms_length; /* Read client_identity */ - temp = ssl3_ExtConsumeHandshakeNumber(ss, 1, &buffer, &buffer_len); - if (temp < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 1, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; parsed_session_ticket->client_identity.client_auth_type = (ClientAuthenticationType)temp; @@ -1612,15 +1613,16 @@ ssl3_ProcessSessionTicketCommon(sslSocket *ss, SECItem *data) goto no_ticket; } /* Read timestamp. */ - temp = ssl3_ExtConsumeHandshakeNumber(ss, 4, &buffer, &buffer_len); - if (temp < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 4, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; - parsed_session_ticket->timestamp = (PRUint32)temp; + parsed_session_ticket->timestamp = temp; /* Read server name */ - nameType = - ssl3_ExtConsumeHandshakeNumber(ss, 1, &buffer, &buffer_len); - if (nameType != TLS_STE_NO_SERVER_NAME) { + rv = ssl3_ExtConsumeHandshakeNumber(ss, &nameType, 1, &buffer, &buffer_len); + if (rv != SECSuccess) + goto no_ticket; + if ((PRInt8)nameType != TLS_STE_NO_SERVER_NAME) { SECItem name_item; rv = ssl3_ExtConsumeHandshakeVariable(ss, &name_item, 2, &buffer, &buffer_len); @@ -1630,12 +1632,12 @@ ssl3_ProcessSessionTicketCommon(sslSocket *ss, SECItem *data) &name_item); if (rv != SECSuccess) goto no_ticket; - parsed_session_ticket->srvName.type = nameType; + parsed_session_ticket->srvName.type = (PRUint8)nameType; } /* Read extendedMasterSecretUsed */ - temp = ssl3_ExtConsumeHandshakeNumber(ss, 1, &buffer, &buffer_len); - if (temp < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &temp, 1, &buffer, &buffer_len); + if (rv != SECSuccess) goto no_ticket; PORT_Assert(temp == PR_TRUE || temp == PR_FALSE); parsed_session_ticket->extendedMasterSecretUsed = (PRBool)temp; @@ -2484,7 +2486,8 @@ ssl3_HandleSupportedPointFormatsXtn(const sslSocket *ss, TLSExtensionData *xtnDa static SECStatus ssl_UpdateSupportedGroups(sslSocket *ss, SECItem *data) { - PRInt32 list_len; + SECStatus rv; + PRUint32 list_len; unsigned int i; const sslNamedGroupDef *enabled[SSL_NAMED_GROUP_COUNT] = { 0 }; PORT_Assert(SSL_NAMED_GROUP_COUNT == PR_ARRAY_SIZE(enabled)); @@ -2495,8 +2498,8 @@ ssl_UpdateSupportedGroups(sslSocket *ss, SECItem *data) } /* get the length of elliptic_curve_list */ - list_len = ssl3_ConsumeHandshakeNumber(ss, 2, &data->data, &data->len); - if (list_len < 0 || data->len != list_len || (data->len % 2) != 0) { + rv = ssl3_ConsumeHandshakeNumber(ss, &list_len, 2, &data->data, &data->len); + if (rv != SECSuccess || data->len != list_len || (data->len % 2) != 0) { (void)ssl3_DecodeError(ss); return SECFailure; } @@ -2510,9 +2513,10 @@ ssl_UpdateSupportedGroups(sslSocket *ss, SECItem *data) /* Read groups from data and enable if in |enabled| */ while (data->len) { const sslNamedGroupDef *group; - PRInt32 curve_name = - ssl3_ConsumeHandshakeNumber(ss, 2, &data->data, &data->len); - if (curve_name < 0) { + PRUint32 curve_name; + rv = ssl3_ConsumeHandshakeNumber(ss, &curve_name, 2, &data->data, + &data->len); + if (rv != SECSuccess) { return SECFailure; /* fatal alert already sent */ } group = ssl_LookupNamedGroup(curve_name); diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h index 0669252d65..208c5eb73e 100644 --- a/lib/ssl/sslimpl.h +++ b/lib/ssl/sslimpl.h @@ -1645,12 +1645,13 @@ extern SECStatus ssl3_AppendHandshakeVariable(sslSocket *ss, const SSL3Opaque *src, PRInt32 bytes, PRInt32 lenSize); extern SECStatus ssl3_AppendSignatureAndHashAlgorithm( sslSocket *ss, const SSLSignatureAndHashAlg *sigAndHash); -extern SECStatus ssl3_ConsumeHandshake(sslSocket *ss, void *v, PRInt32 bytes, +extern SECStatus ssl3_ConsumeHandshake(sslSocket *ss, void *v, PRUint32 bytes, SSL3Opaque **b, PRUint32 *length); -extern PRInt32 ssl3_ConsumeHandshakeNumber(sslSocket *ss, PRInt32 bytes, - SSL3Opaque **b, PRUint32 *length); +extern SECStatus ssl3_ConsumeHandshakeNumber(sslSocket *ss, PRUint32 *num, + PRUint32 bytes, SSL3Opaque **b, + PRUint32 *length); extern SECStatus ssl3_ConsumeHandshakeVariable(sslSocket *ss, SECItem *i, - PRInt32 bytes, SSL3Opaque **b, + PRUint32 bytes, SSL3Opaque **b, PRUint32 *length); extern PRUint8 *ssl_EncodeUintX(PRUint64 value, unsigned int bytes, PRUint8 *to); diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c index 362460c40f..c2e40bed3e 100644 --- a/lib/ssl/tls13con.c +++ b/lib/ssl/tls13con.c @@ -1669,7 +1669,7 @@ SECStatus tls13_HandleHelloRetryRequest(sslSocket *ss, SSL3Opaque *b, PRUint32 length) { SECStatus rv; - PRInt32 tmp; + PRUint32 tmp; SSL3ProtocolVersion version; SSL_TRC(3, ("%d: TLS13[%d]: handle hello retry request", @@ -1718,8 +1718,8 @@ tls13_HandleHelloRetryRequest(sslSocket *ss, SSL3Opaque *b, PRUint32 length) } /* Extensions. */ - tmp = ssl3_ConsumeHandshakeNumber(ss, 2, &b, &length); - if (tmp < 0) { + rv = ssl3_ConsumeHandshakeNumber(ss, &tmp, 2, &b, &length); + if (rv != SECSuccess) { return SECFailure; /* error code already set */ } /* Extensions must be non-empty and use the remainder of the message. @@ -1757,7 +1757,7 @@ tls13_HandleCertificateRequest(sslSocket *ss, SSL3Opaque *b, PRUint32 length) TLS13CertificateRequest *certRequest = NULL; SECItem context = { siBuffer, NULL, 0 }; PLArenaPool *arena; - PRInt32 extensionsLength; + PRUint32 extensionsLength; SSL_TRC(3, ("%d: TLS13[%d]: handle certificate_request sequence", SSL_GETPID(), ss->fd)); @@ -1816,8 +1816,8 @@ tls13_HandleCertificateRequest(sslSocket *ss, SSL3Opaque *b, PRUint32 length) goto loser; /* alert already sent */ /* Verify that the extensions length is correct. */ - extensionsLength = ssl3_ConsumeHandshakeNumber(ss, 2, &b, &length); - if (extensionsLength < 0) { + rv = ssl3_ConsumeHandshakeNumber(ss, &extensionsLength, 2, &b, &length); + if (rv != SECSuccess) { goto loser; /* alert already sent */ } if (extensionsLength != length) { @@ -3008,7 +3008,7 @@ static SECStatus tls13_HandleEncryptedExtensions(sslSocket *ss, SSL3Opaque *b, PRUint32 length) { SECStatus rv; - PRInt32 innerLength; + PRUint32 innerLength; SECItem oldNpn = { siBuffer, NULL, 0 }; PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss)); @@ -3023,8 +3023,8 @@ tls13_HandleEncryptedExtensions(sslSocket *ss, SSL3Opaque *b, PRUint32 length) return SECFailure; } - innerLength = ssl3_ConsumeHandshakeNumber(ss, 2, &b, &length); - if (innerLength < 0) { + rv = ssl3_ConsumeHandshakeNumber(ss, &innerLength, 2, &b, &length); + if (rv != SECSuccess) { return SECFailure; /* Alert already sent. */ } if (innerLength != length) { @@ -3843,7 +3843,6 @@ static SECStatus tls13_HandleNewSessionTicket(sslSocket *ss, SSL3Opaque *b, PRUint32 length) { SECStatus rv; - PRInt32 tmp; PRUint32 utmp; NewSessionTicket ticket = { 0 }; SECItem data; @@ -3864,13 +3863,13 @@ tls13_HandleNewSessionTicket(sslSocket *ss, SSL3Opaque *b, PRUint32 length) } ticket.received_timestamp = ssl_Time(); - tmp = ssl3_ConsumeHandshakeNumber(ss, 4, &b, &length); - if (tmp < 0) { + rv = ssl3_ConsumeHandshakeNumber(ss, &ticket.ticket_lifetime_hint, 4, &b, + &length); + if (rv != SECSuccess) { FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_NEW_SESSION_TICKET, decode_error); return SECFailure; } - ticket.ticket_lifetime_hint = (PRUint32)tmp; ticket.ticket.type = siBuffer; rv = ssl3_ConsumeHandshake(ss, &utmp, sizeof(utmp), diff --git a/lib/ssl/tls13exthandle.c b/lib/ssl/tls13exthandle.c index a206d198ff..a3d1be6030 100644 --- a/lib/ssl/tls13exthandle.c +++ b/lib/ssl/tls13exthandle.c @@ -208,13 +208,13 @@ static SECStatus tls13_HandleKeyShareEntry(const sslSocket *ss, TLSExtensionData *xtnData, SECItem *data) { SECStatus rv; - PRInt32 group; + PRUint32 group; const sslNamedGroupDef *groupDef; TLS13KeyShareEntry *ks = NULL; SECItem share = { siBuffer, NULL, 0 }; - group = ssl3_ExtConsumeHandshakeNumber(ss, 2, &data->data, &data->len); - if (group < 0) { + rv = ssl3_ExtConsumeHandshakeNumber(ss, &group, 2, &data->data, &data->len); + if (rv != SECSuccess) { PORT_SetError(SSL_ERROR_RX_MALFORMED_KEY_SHARE); goto loser; } @@ -285,7 +285,7 @@ SECStatus tls13_ClientHandleKeyShareXtnHrr(const sslSocket *ss, TLSExtensionData *xtnData, PRUint16 ex_type, SECItem *data) { SECStatus rv; - PRInt32 tmp; + PRUint32 tmp; const sslNamedGroupDef *group; PORT_Assert(!ss->sec.isServer); @@ -294,8 +294,8 @@ tls13_ClientHandleKeyShareXtnHrr(const sslSocket *ss, TLSExtensionData *xtnData, SSL_TRC(3, ("%d: SSL3[%d]: handle key_share extension in HRR", SSL_GETPID(), ss->fd)); - tmp = ssl3_ExtConsumeHandshakeNumber(ss, 2, &data->data, &data->len); - if (tmp < 0) { + rv = ssl3_ExtConsumeHandshakeNumber(ss, &tmp, 2, &data->data, &data->len); + if (rv != SECSuccess) { return SECFailure; /* error code already set */ } if (data->len) { @@ -335,7 +335,7 @@ SECStatus tls13_ServerHandleKeyShareXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRUint16 ex_type, SECItem *data) { SECStatus rv; - PRInt32 length; + PRUint32 length; PORT_Assert(ss->sec.isServer); PORT_Assert(PR_CLIST_IS_EMPTY(&xtnData->remoteKeyShares)); @@ -349,9 +349,9 @@ tls13_ServerHandleKeyShareXtn(const sslSocket *ss, TLSExtensionData *xtnData, PR /* Redundant length because of TLS encoding (this vector consumes * the entire extension.) */ - length = ssl3_ExtConsumeHandshakeNumber(ss, 2, &data->data, - &data->len); - if (length < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &length, 2, &data->data, + &data->len); + if (rv != SECSuccess) goto loser; if (length != data->len) { /* Check for consistency */ @@ -684,7 +684,8 @@ SECStatus tls13_ClientHandlePreSharedKeyXtn(const sslSocket *ss, TLSExtensionData *xtnData, PRUint16 ex_type, SECItem *data) { - PRInt32 index; + PRUint32 index; + SECStatus rv; SSL_TRC(3, ("%d: SSL3[%d]: handle pre_shared_key extension", SSL_GETPID(), ss->fd)); @@ -694,8 +695,8 @@ tls13_ClientHandlePreSharedKeyXtn(const sslSocket *ss, TLSExtensionData *xtnData return SECSuccess; } - index = ssl3_ExtConsumeHandshakeNumber(ss, 2, &data->data, &data->len); - if (index < 0) + rv = ssl3_ExtConsumeHandshakeNumber(ss, &index, 2, &data->data, &data->len); + if (rv != SECSuccess) return SECFailure; /* This should be the end of the extension. */