1006804 - http2 enforce ephemeral cipher suite requirement

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

Implementations MUST negotiate - and therefore use - ephemeral cipher
   suites, such as ephemeral Diffie-Hellman (DHE) or the elliptic curve
   variant (ECDHE) with a minimum size of 2048 bits (DHE) or security
   level of 128 bits (ECDHE).

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: 0001-psm-interface-for-kea-size-and-make-kea-available-in.patch

does this do the trick nick?

Comment 2

[Patrick McManus [:mcmanus]]

to document an earlier call with mt and ekr aiui ecdhe security level of 128 bits requires KEAKeyBits >=256 here.

Comment 3

[u408661]

Comment on attachment 8418346 [details] [diff] [review]
0001-psm-interface-for-kea-size-and-make-kea-available-in.patch

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

Works fine when I test locally with a real cert against nghttp2, however, it causes the xpcshell test to fail locally. This is because node doesn't negotiate a good TLS profile. I spent some (way too much, actually) time when we were in Paris (I think) trying to get node to negotiate a sane TLS profile, but either the node documentation lies, or I'm dumber than I thought, because I couldn't for the life of me get it to negotiate anything even remotely approximating sane (nothing better than TLS_RSA_WITH_AES_128_CBC_SHA). So we have 2 options - (1) try to fix node, or (2) make the xpcshell test not confirm the tls profile. I'm in favor of option (2), but that could just be me being lazy.

Comment 4

[u408661]

Attachment: 0002-Bug-1006804-Enforce-TLS-key-type-and-size-restrictions-in-HTTP-2.-r-mcmanus.patch

OK, here's the patch that builds on top of yours that makes the checks work (and disables the checks in the unit test until we can figure out how to make node's TLS negotiation not suck). FWIW, I also did a quick test w/twitter, and this works with a Real Site, not just my junky local test site.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8418346 [details] [diff] [review]
0001-psm-interface-for-kea-size-and-make-kea-available-in.patch

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

My only concern is that channelInfo.keaKeyBits is a uint32_t, and we're storing that value in a int16_t. Other than that, r=me.

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +903,5 @@
> +      status->mKeyLength = cipherInfo.symKeyBits;
> +      status->mSecretKeyLength = cipherInfo.effectiveKeyBits;
> +      status->mCipherName.Assign(cipherInfo.cipherSuiteName);
> +      infoObject->SetKEAUsed(cipherInfo.keaType);
> +      infoObject->SetKEAKeyBits(channelInfo.keaKeyBits);

We should range-check channelInfo.keaKeyBits, because I think we have a uint32_t being stored in a int16_t here.

Comment 6

[Patrick McManus [:mcmanus]]

david - good point. never know when we'll get our first billion bit key :)

rather than bounds check it and add a new limit, I just changed the attribute to also be u32.

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 8419043 [details] [diff] [review]
0002-Bug-1006804-Enforce-TLS-key-type-and-size-restrictions-in-HTTP-2.-r-mcmanus.patch

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

::: netwerk/protocol/http/Http2Session.cpp
@@ +2855,2 @@
>  
> +  uint16_t keybits = ssl->GetKEAKeyBits();

patch 0001 is going to change to make this a u32.. so this will bitrot.

Comment 8

[Patrick McManus [:mcmanus]]

Attachment: 0001-bug-1006804-psm-interface-for-kea-size-and-make-kea-.patch

Comment 9

[u408661]

Attachment: 0002-Bug-1006804-Enforce-TLS-key-type-and-size-restrictions-in-HTTP-2.-r-mcmanus.patch

Here's an un-bitrotted version of the patch, for sanity.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8419697 [details] [diff] [review]
0001-bug-1006804-psm-interface-for-kea-size-and-make-kea-.patch

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

7 lines context please

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +896,5 @@
> +      RefPtr<nsSSLStatus> status(infoObject->SSLStatus());
> +      if (!status) {
> +        status = new nsSSLStatus();
> +        infoObject->SetSSLStatus(status);
> +      }

EnsureSSLStatus would be so nice...

Comment 11

[Patrick McManus [:mcmanus]]

part 1
 https://hg.mozilla.org/integration/mozilla-inbound/rev/269ba1d7f4df

will leave open for nick to land the other half.. not sure if he's ready.

Comment 12

[u408661]

And here's part 2!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cdddbe2d85b4

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/269ba1d7f4df
https://hg.mozilla.org/mozilla-central/rev/cdddbe2d85b4