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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Hubert Kario]

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

[Daiki Ueno [:ueno]]

Tim, would you elaborate the rationale behind this fallback (bug 1335069 comment 0)?  I couldn't find any reference suggesting that behavior.

Comment 2

[Hubert Kario]

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

[Eric Rescorla (:ekr)]

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.

Comment 4

[Hubert Kario]

> 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

[Eric Rescorla (:ekr)]

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.

Comment 6

[Hubert Kario]

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

[Eric Rescorla (:ekr)]

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

[Kai Engert (:KaiE:)]

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?

Comment 9

[Kai Engert (:KaiE:)]

Adding regression keyword, I understand Hubert saw that earlier versions of NSS to reject as expected.

Comment 10

[Kai Engert (:KaiE:)]

(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?

Comment 11

[Kai Engert (:KaiE:)]

(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.)

Comment 12

[Eric Rescorla (:ekr)]

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.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 14

[Daiki Ueno [:ueno]]

Attachment: Bug 1412829, reject empty supported_signature_algorithms in CR in TLS 1.2

This basically reverts bug 1335069 to align with RFC 5246.

Comment 15

[Daiki Ueno [:ueno]]

Pushed as:
https://hg.mozilla.org/projects/nss/rev/01f74d6a5cb2