Closed Bug 1006804 Opened 10 years ago Closed 10 years ago

http2 enforce ephemeral cipher suite requirement


(Core :: Networking: HTTP, defect)

29 Branch
Not set





(Reporter: mcmanus, Assigned: u408661)


(Whiteboard: [spdy])


(2 files, 2 obsolete files)

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).
Assignee: nobody → hurley
does this do the trick nick?
Attachment #8418346 - Flags: feedback?(hurley)
to document an earlier call with mt and ekr aiui ecdhe security level of 128 bits requires KEAKeyBits >=256 here.
Comment on attachment 8418346 [details] [diff] [review]

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.
Attachment #8418346 - Flags: feedback?(hurley) → feedback+
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.
Attachment #8419043 - Flags: review?(mcmanus)
Attachment #8419043 - Flags: review?(mcmanus) → review+
Attachment #8418346 - Flags: review?(dkeeler)
Comment on attachment 8418346 [details] [diff] [review]

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.
Attachment #8418346 - Flags: review?(dkeeler) → review+
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 on attachment 8419043 [details] [diff] [review]

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.
Attachment #8418346 - Attachment is obsolete: true
Attachment #8419697 - Flags: review?(honzab.moz)
Attachment #8419697 - Flags: review+
Here's an un-bitrotted version of the patch, for sanity.
Attachment #8419043 - Attachment is obsolete: true
Attachment #8419718 - Flags: review+
Comment on attachment 8419697 [details] [diff] [review]

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...
Attachment #8419697 - Flags: review?(honzab.moz) → review+
part 1

will leave open for nick to land the other half.. not sure if he's ready.
Keywords: leave-open
And here's part 2!

Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.