Open Bug 1431387 Opened 2 years ago Updated Last year

Avoid the special handling for SSL_ERROR_UNSUPPORTED_SIGNATURE_ALGORITHM in ssl3_HandleClientHello

Categories

(NSS :: Libraries, enhancement, P3)

3.36
enhancement

Tracking

(Not tracked)

People

(Reporter: kaie, Unassigned, NeedInfo)

References

Details

Attachments

(1 file)

In bug 1413634 we added special handling for error code SSL_ERROR_UNSUPPORTED_SIGNATURE_ALGORITHM to function ssl3_HandleClientHello.

This was done, because that function currently uses SSL_ERROR_RX_MALFORMED_CLIENT_HELLO as the general fallback error code. However, if ssl3_HandleParsedExtensions fails because there was no overlap in signature algorithms, we want to return that more precise error code to the caller.

It would be better to simply return the error code produced by ssl3_HandleParsedExtensions.

Unfortunately that simple approach doesn't work, it results in multiple tests to fail:
ssl_gtest.sh: #8338: 'TlsExtensionTest13Stream: ResumeEmptyPskLabel' - FAILED
ssl_gtest.sh: #8339: 'TlsExtensionTest13Stream: ResumeBinderTooShort' - FAILED
ssl_gtest.sh: #8340: 'TlsExtensionTest13Stream: ResumeTwoIdentitiesOneBinder' - FAILED
ssl_gtest.sh: #8341: 'TlsExtensionTest13Stream: ResumeOneIdentityTwoBinders' - FAILED
ssl_gtest.sh: #8342: 'GenericStream/TlsConnectGeneric: ConnectResumeCorruptTicket/0 (0, 772)' - FAILED
ssl_gtest.sh: #8343: 'GenericStream/TlsConnectGeneric: ConnectResumeCorruptTicket/1 (0, 771)' - FAILED
ssl_gtest.sh: #8344: 'GenericStream/TlsConnectGeneric: ConnectResumeCorruptTicket/2 (0, 770)' - FAILED
ssl_gtest.sh: #8345: 'GenericStream/TlsConnectGeneric: ConnectResumeCorruptTicket/3 (0, 769)' - FAILED
ssl_gtest.sh: #8346: 'GenericDatagram/TlsConnectGeneric: ConnectResumeCorruptTicket/0 (1, 772)' - FAILED
ssl_gtest.sh: #8347: 'GenericDatagram/TlsConnectGeneric: ConnectResumeCorruptTicket/1 (1, 771)' - FAILED
ssl_gtest.sh: #8348: 'GenericDatagram/TlsConnectGeneric: ConnectResumeCorruptTicket/2 (1, 770)' - FAILED

This bug suggests to investigate if the above tests could be adjusted to allow the simpler code to work without test failures.
Seems easy enough to just change the expectation in those tests.  The ConnectResumeCorruptTicket tests return SEC_ERROR_BAD_DATA which is pretty bad.  We should change the code in that case.

Using ssl_MapLowLevelError() would work for ConnectResumeCorruptTicket, but please don't.  Instead, I prefer changing the code in ssl3_ProcessSessionTicketCommon and tls13_RecoverHashState to a new code (maybe SSL_ERROR_INVALID_TICKET) if the ticket can't be parsed.
Correction: to *set* a new code if the ticket can't be parsed.
Kai, do you plan to fix this?
Flags: needinfo?(kaie)
Yes, I can work on it, but I don't know yet how soon I'll have time for it. I'll keep the needinfo for me.
You need to log in before you can comment on or make changes to this bug.