RFC 5246 non compliance with CertificateVerify fallback to SHA-1 (NSS client should abort connection if no overlap)

NEW
Unassigned

Status

NSS
Libraries
23 days ago
12 days ago

People

(Reporter: Hubert Kario, Unassigned)

Tracking

({regression})

3.33
regression

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

23 days ago
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

Comment 1

23 days ago
Tim, would you elaborate the rationale behind this fallback (bug 1335069 comment 0)?  I couldn't find any reference suggesting that behavior.
Flags: needinfo?(ttaubert)
(Reporter)

Comment 2

23 days ago
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.

Comment 3

23 days ago
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.
(Reporter)

Comment 4

23 days ago
> 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.

Comment 5

23 days ago
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.
(Reporter)

Comment 6

22 days ago
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.

Comment 7

22 days ago
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.

Comment 8

13 days ago
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?
Summary: RFC 5246 non compliance with CertificateVerify fallback to SHA-1 → RFC 5246 non compliance with CertificateVerify fallback to SHA-1 (NSS client should abort connection if no overlap)

Comment 9

13 days ago
Adding regression keyword, I understand Hubert saw that earlier versions of NSS to reject as expected.
Keywords: regression

Updated

13 days ago
Depends on: 1335069
Flags: needinfo?(ttaubert)

Comment 10

13 days ago
(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?
Flags: needinfo?(ekr)

Comment 11

13 days ago
(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.
Flags: needinfo?(ekr)
(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.
You need to log in before you can comment on or make changes to this bug.