When NSS 3.33 is used with client certificates, and server advertises only MD-5 hash algorithm, NSS will use SHA-1 for signature instead of aborting the connection. This is non compliance with a RFC 5246 MUST clause: The hash and signature algorithms used in the signature MUST be one of those present in the supported_signature_algorithms field of the CertificateRequest message. Steps to reproduce: 1. set up a server that advertises support only for signature algorithms unknown to NSS in Certificate Request 2. Connect NSS to it Result: rejection of CV message by server Expected result: Connection abort by client Additional info: The bug seems to have been introduced in this changeset: https://hg.mozilla.org/projects/nss/rev/c61324648525#l3.95
Tim, would you elaborate the rationale behind this fallback (bug 1335069 comment 0)? I couldn't find any reference suggesting that behavior.
While the text is not explicit, the specification (after errata 2864) is very explicit in saying that the list cannot be empty: SignatureAndHashAlgorithm supported_signature_algorithms<2..2^16-1>; So an empty supported_signature_algorithms in Certificate Request should cause the client to send a decode_error alert to the server.
I agree with Hubert that this behavior is wrong: if the server provides an empty list, the client should not supply something not on the list. I'm actually surprised to see this because IIRC I noted this bug on a previous CL, though I'm too lazy to go back and try to find it. I disagree with Hubert that the client needs to send decode_error. I think it would also be acceptable to simply act as if no common algorithm exists. The specification never says that every decoding failure needs to elicit an alert.
> The specification never says that every decoding failure needs to elicit an alert. The RFC states: A server MUST accept ClientHello messages both with and without the extensions field, and (as for all other messages) it MUST check that the amount of data in the message precisely matches one of these formats; if not, then it MUST send a fatal "decode_error" alert. Since the necessity to check if encoding matches an expected format applies to all messages, sending "decode_error" alert is expected, if any alert is sent before connection close. But please note, I'm talking above about only the empty supported_signature_algorithms list. In case the list is not empty and the values are not acceptable, the handshake should end with the typical handshake_failure.
You're overreading the text. This check merely means that the Handshake message length must align with the end of the extensions. It does not refer to the inner fields.
yes, you're right. (I could have sworn that I read "and formats") still, in the alert definitions there is: Whenever an implementation encounters a condition which is defined as a fatal alert, it MUST send the appropriate alert prior to closing the connection. For all errors where an alert level is not explicitly specified, the sending party MAY determine at its discretion whether to treat this as a fatal error or not. and decode_error A message could not be decoded because some field was out of the specified range or the length of the message was incorrect. This message is always fatal and should never be observed in communication between proper implementations (except when messages were corrupted in the network). a condition of "field was out of range" is defined, the appropriate error "decode_error" is defined as "always fatal" so it needs to be sent and connection needs to be closed after sending it Also, I don't get why you're against doing that in the first place, it's exactly the same behaviour that TLS1.3 explicitly requires, so having the same error handling for both TLS 1.3 and TLS 1.2 only makes the code simpler, not more complex. And I think we can both agree that simpler code is less likely to harbour bugs - which is a good thing.
Again, I don't think you're reading the spec right. The conditions that are specified as a fatal alert are the ones elsewhere in the spec, not the alert definitions. As for the bigger picture question, I'm not necessarily against doing this particular thing, but what I am against is having an overly strict definition of when alerts must be sent, because sometimes that doesn't match our implementation well.
IIUC, there is agreement that the NSS client side is behaving incorrectly, it shouldn't fall back to SHA-1, and it should abort the connection. Should this be treated as a regression?
Adding regression keyword, I understand Hubert saw that earlier versions of NSS to reject as expected.
(In reply to Eric Rescorla (:ekr) from comment #3) > I agree with Hubert that this behavior is wrong: if the server provides an > empty list, the client should not supply something not on the list. I'm > actually surprised to see this because IIRC I noted this bug on a previous > CL, though I'm too lazy to go back and try to find it. > > I disagree with Hubert that the client needs to send decode_error. I think > it would also be acceptable to simply act as if no common algorithm exists. > The specification never says that every decoding failure needs to elicit an > alert. EKR, you're saying, the existing code is incorrect, if no common algorithms exists, NSS shouldn't do client authentication with SHA-1, NSS should be changed to not perform client authenticate at all. Is my understanding correct?
(I'm asking for this clarification, because apparently the discussion about the alert details has caused some distraction from the original problem, and in bug 1335069 Tim had (IMHO incorrectly) concluded that there's nothing to be done.)
Kai, Yes, I think the code is wrong. Specifically - If the peer doesn't send signature_algorithms, you assume a default set - If the peer does send signature_algorithms, even if empty, you should either listen to it, or (if empty) reject it as Hubert says.
(In reply to Kai Engert (:kaie:) from comment #11) > (I'm asking for this clarification, because apparently the discussion about > the alert details has caused some distraction from the original problem, and > in bug 1335069 Tim had (IMHO incorrectly) concluded that there's nothing to > be done.) I'm very surprised how you read this from my comment. I never said or implied there's nothing to be done. I simply had nothing to add to what Ekr already said in comment #3 because it's obviously something that should be fixed. I don't think there ever was any confusion about this.