1132941 - with TLS 1.2, ssl3_SendServerKeyExchange sends short header length for kt_rsa

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

Attachment: Patch v1

I found this issue while working on DHE TLS server side support (bug 102794).

With TLS 1.2, ssl3_SendServerKeyExchange sends a server_key_exchange header with a length, that doesn't include the number of bytes used for the SignatureAndHashAlgorithm (which are only sent with TLS 1.2).

I read that the ServerKeyExchange message is sent for RSA only in certain scenarios, and I haven't yet tested such a scenario.

However, an equivalent increase of the header length was necessary to make the implementation of the DHE ServerKeyExchange work, so I conclude this change is necessary for the RSA scenario, too.

Comment 1

[Kai Engert (:KaiE:)]

Comment on attachment 8564183 [details] [diff] [review]
Patch v1

I'm not sure if Adam does NSS reviews, so I'm asking Wan-Teh, too.

Comment 2

[Adam Langley]

I think this change is good, but it's for RSA-EXPORT ciphers no? (It's tough to tell in the Bugzilla review interface because it doesn't have all the source.) I'd be very surprised if RSA-EXPORT is getting used as I don't believe that FF (or anything) enables it these days.

Comment 3

[Kai Engert (:KaiE:)]

(In reply to Adam Langley from comment #2)
> I think this change is good, but it's for RSA-EXPORT ciphers no? (It's tough
> to tell in the Bugzilla review interface because it doesn't have all the
> source.) I'd be very surprised if RSA-EXPORT is getting used as I don't
> believe that FF (or anything) enables it these days.

You're right, this code is only called on RSA with stepdown.

Nevertheless, while it hasn't been ripped out yet, I'd like to fix it. I ran into this problem while working on bug 102794.

Can I consider comment 2 as an r+ ?

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 8564183 [details] [diff] [review]
Patch v1

r=agl

because he said it's a good change

Comment 5

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/ff8bc21b04d7

Comment 6

[Wan-Teh Chang]

Comment on attachment 8564183 [details] [diff] [review]
Patch v1

Review of attachment 8564183 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Kai, your patch fixes an inconsistency of our code.
I suggest a better fix below because I found that the TLS
1.2 code in question should be removed.

::: lib/ssl/ssl3con.c
@@ +8894,5 @@
>  	    rv = ssl3_AppendSignatureAndHashAlgorithm(ss, &sigAndHash);
>  	    if (rv != SECSuccess) {
>  		goto loser; 	/* err set by AppendHandshake. */
>  	    }
>  	}

It is better to remove the new code you just added and this
block of code (lines 8893-8898). The reason is that all
export cipher suites are disallowed in TLS 1.1 or later.

Here are some excerpts from TLS 1.2 (RFC 5246), Section 7.4.3
about this issue:

      It is not legal to send the ServerKeyExchange message for the
      following key exchange methods:

         RSA
         DH_DSS
         DH_RSA

      struct {
          select (KeyExchangeAlgorithm) {
              case dh_anon:
                  ServerDHParams params;
              case dhe_dss:
              case dhe_rsa:
                  ServerDHParams params;
                  digitally-signed struct {
                      opaque client_random[32];
                      opaque server_random[32];
                      ServerDHParams params;
                  } signed_params;
              case rsa:
              case dh_dss:
              case dh_rsa:
                  struct {} ;
                 /* message is omitted for rsa, dh_dss, and dh_rsa */
              /* may be extended, e.g., for ECDH -- see [TLSECC] */
          };
      } ServerKeyExchange;

Note the case rsa in the ServerKeyExchange struct. You can compare
this section with Section 7.4.3 in TLS 1.0 (RFC 2246).