diff --git a/security/nss/lib/ssl/ssl3ext.c b/security/nss/lib/ssl/ssl3ext.c index 7e674f0e03..3b0d8abd78 100644 --- a/security/nss/lib/ssl/ssl3ext.c +++ b/security/nss/lib/ssl/ssl3ext.c @@ -321,6 +321,26 @@ ssl3_ExtensionAdvertised(const sslSocket *ss, PRUint16 ex_type) xtnData->numAdvertised, ex_type); } +void +ssl3_RecordExtensionNegotiated(const sslSocket *ss, TLSExtensionData *xtnData, + PRUint16 ex_type) +{ + /* Record that an extension was negotiated during a full TLS handshake. + * This function must NOT be used to track extensions carried in + * post-handshake messages (e.g. CertificateRequest during PHA); + * their negotiation state should instead be stored in dedicated fields on + * TLSExtensionData or sslSocket (e.g. xtnData->compressionAlg for + * certificate compression). */ + PORT_Assert(!ss->firstHsDone || + ss->opt.enableRenegotiation != SSL_RENEGOTIATE_NEVER); + PORT_Assert(!arrayContainsExtension(xtnData->negotiated, + xtnData->numNegotiated, ex_type)); + PORT_Assert(xtnData->numNegotiated < SSL_MAX_EXTENSIONS); + if (xtnData->numNegotiated < SSL_MAX_EXTENSIONS) { + xtnData->negotiated[xtnData->numNegotiated++] = ex_type; + } +} + /* Go through hello extensions in |b| and deserialize * them into the list in |ss->ssl3.hs.remoteExtensions|. * The only checking we do in this point is for duplicates. diff --git a/security/nss/lib/ssl/ssl3ext.h b/security/nss/lib/ssl/ssl3ext.h index 97319c7d9a..8abcbb63bb 100644 --- a/security/nss/lib/ssl/ssl3ext.h +++ b/security/nss/lib/ssl/ssl3ext.h @@ -157,6 +157,9 @@ void ssl3_ResetExtensionData(TLSExtensionData *xtnData, const sslSocket *ss); PRBool ssl3_ExtensionNegotiated(const sslSocket *ss, PRUint16 ex_type); PRBool ssl3_ExtensionAdvertised(const sslSocket *ss, PRUint16 ex_type); +void ssl3_RecordExtensionNegotiated(const sslSocket *ss, + TLSExtensionData *xtnData, + PRUint16 ex_type); SECStatus ssl3_RegisterExtensionSender(const sslSocket *ss, TLSExtensionData *xtnData, diff --git a/security/nss/lib/ssl/ssl3exthandle.c b/security/nss/lib/ssl/ssl3exthandle.c index 206cb00e48..c044b82cf4 100644 --- a/security/nss/lib/ssl/ssl3exthandle.c +++ b/security/nss/lib/ssl/ssl3exthandle.c @@ -176,7 +176,7 @@ ssl3_HandleServerNameXtn(const sslSocket *ss, TLSExtensionData *xtnData, ssl3_FreeSniNameArray(xtnData); xtnData->sniNameArr = names; xtnData->sniNameArrSize = 1; - xtnData->negotiated[xtnData->numNegotiated++] = ssl_server_name_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_server_name_xtn); } return SECSuccess; @@ -356,7 +356,7 @@ ssl3_SelectAppProtocol(const sslSocket *ss, TLSExtensionData *xtnData, } xtnData->nextProtoState = SSL_NEXT_PROTO_NEGOTIATED; - xtnData->negotiated[xtnData->numNegotiated++] = extension; + ssl3_RecordExtensionNegotiated(ss, xtnData, extension); return SECITEM_CopyItem(NULL, &xtnData->nextProto, &result); } @@ -458,7 +458,7 @@ ssl3_ClientHandleAppProtoXtn(const sslSocket *ss, TLSExtensionData *xtnData, SECITEM_FreeItem(&xtnData->nextProto, PR_FALSE); xtnData->nextProtoState = SSL_NEXT_PROTO_SELECTED; - xtnData->negotiated[xtnData->numNegotiated++] = ssl_app_layer_protocol_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_app_layer_protocol_xtn); return SECITEM_CopyItem(NULL, &xtnData->nextProto, &protocol_name); } @@ -526,7 +526,7 @@ ssl3_ServerHandleStatusRequestXtn(const sslSocket *ss, TLSExtensionData *xtnData PORT_Assert(ss->sec.isServer); /* remember that we got this extension. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_cert_status_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_cert_status_xtn); if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) { sender = tls13_ServerSendStatusRequestXtn; @@ -604,7 +604,7 @@ ssl3_ClientHandleStatusRequestXtn(const sslSocket *ss, TLSExtensionData *xtnData } /* Keep track of negotiated extensions. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_cert_status_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_cert_status_xtn); return SECSuccess; } @@ -857,7 +857,7 @@ ssl3_ClientHandleSessionTicketXtn(const sslSocket *ss, TLSExtensionData *xtnData } /* Keep track of negotiated extensions. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_session_ticket_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_session_ticket_xtn); return SECSuccess; } @@ -1295,7 +1295,7 @@ ssl3_ServerHandleSessionTicketXtn(const sslSocket *ss, TLSExtensionData *xtnData } /* Keep track of negotiated extensions. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_session_ticket_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_session_ticket_xtn); /* Parse the received ticket sent in by the client. We are * lenient about some parse errors, falling back to a fullshake @@ -1373,7 +1373,7 @@ ssl3_HandleRenegotiationInfoXtn(const sslSocket *ss, TLSExtensionData *xtnData, /* remember that we got this extension and it was correct. */ CONST_CAST(sslSocket, ss) ->peerRequestedProtection = 1; - xtnData->negotiated[xtnData->numNegotiated++] = ssl_renegotiation_info_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_renegotiation_info_xtn); if (ss->sec.isServer) { /* prepare to send back the appropriate response */ rv = ssl3_RegisterExtensionSender(ss, xtnData, @@ -1508,7 +1508,7 @@ ssl3_ClientHandleUseSRTPXtn(const sslSocket *ss, TLSExtensionData *xtnData, } /* OK, this looks fine. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_use_srtp_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_use_srtp_xtn); xtnData->dtlsSRTPCipherSuite = cipher; return SECSuccess; } @@ -1579,7 +1579,7 @@ ssl3_ServerHandleUseSRTPXtn(const sslSocket *ss, TLSExtensionData *xtnData, /* OK, we have a valid cipher and we've selected it */ xtnData->dtlsSRTPCipherSuite = cipher; - xtnData->negotiated[xtnData->numNegotiated++] = ssl_use_srtp_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_use_srtp_xtn); return ssl3_RegisterExtensionSender(ss, xtnData, ssl_use_srtp_xtn, @@ -1625,8 +1625,12 @@ ssl3_HandleSigAlgsXtn(const sslSocket *ss, TLSExtensionData *xtnData, return SECFailure; } - /* Keep track of negotiated extensions. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_signature_algorithms_xtn; + /* Keep track of negotiated extensions. Only the server consumes this + * entry; on the client, skipping prevents numNegotiated overflow + * during repeated post-handshake CertificateRequests. */ + if (ss->sec.isServer) { + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_signature_algorithms_xtn); + } return SECSuccess; } @@ -1696,7 +1700,7 @@ ssl3_HandleExtendedMasterSecretXtn(const sslSocket *ss, TLSExtensionData *xtnDat SSL_GETPID(), ss->fd)); /* Keep track of negotiated extensions. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_extended_master_secret_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_extended_master_secret_xtn); if (ss->sec.isServer) { return ssl3_RegisterExtensionSender(ss, xtnData, @@ -1743,7 +1747,7 @@ ssl3_ClientHandleSignedCertTimestampXtn(const sslSocket *ss, TLSExtensionData *x } *scts = *data; /* Keep track of negotiated extensions. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_signed_cert_timestamp_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_signed_cert_timestamp_xtn); return SECSuccess; } @@ -1779,7 +1783,7 @@ ssl3_ServerHandleSignedCertTimestampXtn(const sslSocket *ss, return SECFailure; } - xtnData->negotiated[xtnData->numNegotiated++] = ssl_signed_cert_timestamp_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_signed_cert_timestamp_xtn); PORT_Assert(ss->sec.isServer); return ssl3_RegisterExtensionSender(ss, xtnData, ssl_signed_cert_timestamp_xtn, @@ -1911,7 +1915,7 @@ ssl_HandleSupportedGroupsXtn(const sslSocket *ss, TLSExtensionData *xtnData, } /* Remember that we negotiated this extension. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_supported_groups_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_supported_groups_xtn); return SECSuccess; } @@ -1952,7 +1956,7 @@ ssl_HandleRecordSizeLimitXtn(const sslSocket *ss, TLSExtensionData *xtnData, /* We can't enforce the maximum on a server. But we do need to ensure * that we don't apply a limit that is too large. */ xtnData->recordSizeLimit = PR_MIN(maxLimit, limit); - xtnData->negotiated[xtnData->numNegotiated++] = ssl_record_size_limit_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_record_size_limit_xtn); return SECSuccess; } diff --git a/security/nss/lib/ssl/tls13exthandle.c b/security/nss/lib/ssl/tls13exthandle.c index 1f88016a1e..aea4f2e348 100644 --- a/security/nss/lib/ssl/tls13exthandle.c +++ b/security/nss/lib/ssl/tls13exthandle.c @@ -354,8 +354,7 @@ tls13_ServerHandleKeyShareXtn(const sslSocket *ss, TLSExtensionData *xtnData, } /* Keep track of negotiated extensions. */ - xtnData->negotiated[xtnData->numNegotiated++] = - ssl_tls13_key_share_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_tls13_key_share_xtn); return SECSuccess; @@ -589,10 +588,7 @@ tls13_ServerHandlePreSharedKeyXtn(const sslSocket *ss, TLSExtensionData *xtnData if (numBinders != numIdentities) goto alert_loser; - /* Keep track of negotiated extensions. Note that this does not - * mean we are resuming. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_tls13_pre_shared_key_xtn; - + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_tls13_pre_shared_key_xtn); return SECSuccess; alert_loser: @@ -653,7 +649,7 @@ tls13_ClientHandlePreSharedKeyXtn(const sslSocket *ss, TLSExtensionData *xtnData } /* Keep track of negotiated extensions. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_tls13_pre_shared_key_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_tls13_pre_shared_key_xtn); return SECSuccess; } @@ -696,7 +692,7 @@ tls13_ServerHandleEarlyDataXtn(const sslSocket *ss, TLSExtensionData *xtnData, return SECFailure; } - xtnData->negotiated[xtnData->numNegotiated++] = ssl_tls13_early_data_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_tls13_early_data_xtn); return SECSuccess; } @@ -721,7 +717,7 @@ tls13_ClientHandleEarlyDataXtn(const sslSocket *ss, TLSExtensionData *xtnData, } /* Keep track of negotiated extensions. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_tls13_early_data_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_tls13_early_data_xtn); return SECSuccess; } @@ -911,7 +907,7 @@ tls13_ServerHandleCookieXtn(const sslSocket *ss, TLSExtensionData *xtnData, } /* Keep track of negotiated extensions. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_tls13_cookie_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_tls13_cookie_xtn); return SECSuccess; } @@ -942,7 +938,7 @@ tls13_ServerHandlePostHandshakeAuthXtn(const sslSocket *ss, } /* Keep track of negotiated extensions. */ - xtnData->negotiated[xtnData->numNegotiated++] = ssl_tls13_post_handshake_auth_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_tls13_post_handshake_auth_xtn); return SECSuccess; } @@ -1005,8 +1001,7 @@ tls13_ServerHandlePskModesXtn(const sslSocket *ss, TLSExtensionData *xtnData, } /* Keep track of negotiated extensions. */ - xtnData->negotiated[xtnData->numNegotiated++] = - ssl_tls13_psk_key_exchange_modes_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_tls13_psk_key_exchange_modes_xtn); return SECSuccess; } @@ -1364,8 +1359,7 @@ tls13_ServerHandleEsniXtn(const sslSocket *ss, TLSExtensionData *xtnData, } /* Keep track of negotiated extensions. */ - xtnData->negotiated[xtnData->numNegotiated++] = - ssl_tls13_encrypted_sni_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_tls13_encrypted_sni_xtn); PORT_ZFree(plainText, data->len); return SECSuccess; @@ -1447,8 +1441,7 @@ tls13_ClientHandleDelegatedCredentialsXtn(const sslSocket *ss, return SECFailure; /* code already set */ } - xtnData->negotiated[xtnData->numNegotiated++] = - ssl_delegated_credentials_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_delegated_credentials_xtn); return SECSuccess; } @@ -1485,8 +1478,7 @@ tls13_ServerHandleDelegatedCredentialsXtn(const sslSocket *ss, SECItem *data) { xtnData->peerRequestedDelegCred = PR_TRUE; - xtnData->negotiated[xtnData->numNegotiated++] = - ssl_delegated_credentials_xtn; + ssl3_RecordExtensionNegotiated(ss, xtnData, ssl_delegated_credentials_xtn); return ssl3_RegisterExtensionSender( ss, xtnData, ssl_delegated_credentials_xtn,