Closed Bug 1483128 (CVE-2018-12384) Opened 3 years ago Closed 3 years ago

ServerHello.random is all zero when handling a v2-compatible ClientHello

Categories

(NSS :: Libraries, defect)

defect
Not set
critical

Tracking

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 unaffected, firefox62 unaffected, firefox63 unaffected)

RESOLVED FIXED
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 --- unaffected

People

(Reporter: mt, Assigned: mt)

Details

(Keywords: sec-high)

Attachments

(4 files, 1 obsolete file)

When we added the very first support for TLS 1.3 in bug 1057463, that moved the generation of the ServerHello.random to ssl3_HandleClientHello():

https://searchfox.org/nss/diff/e89d3055c719e257017518b56dff5cf4b7f6d480/lib/ssl/ssl3con.c#8109

Unfortunately, that removed the call to ssl3_GetNewRandom() on the server side when handling an SSLv2-compatible ClientHello.  It was previously in ssl3_SendServerHello().  Now, if an SSLv2-compatible ClientHello is received, the server doesn't generate a new random: it sends an all-zero value.

This results in full malleability of the ClientHello.  The only restrictions are minor: an attacker can't negotiate TLS 1.3, an extended master secret, or to include the downgrade SCSV if it is reducing the version.

That means the server is exposed to almost arbitrary downgrade.  An attacker can cause a server to use the weakest configuration it is willing to operate.  However, this does not appear to result in a compromise of the pre-master secret, because an honest client is involved in generating either a DH share or the pre-master secret (for static RSA).

No fresh entropy from the server means that the server doesn't verify that the client is live.  Duplicate ClientHello.random values result in identical key exchange signatures, pre-master secrets, and Finished values.  The attacks in RFC 7627 are probably a subset of what is possible.

Unless I've missed something, this has no obvious mitigation other than fixing the code.  Support for a SSLv2-compatible ClientHello cannot be disabled in configuration.  And we don't have our TLS 1.3 downgrade measures in place, which would be effective here.  As long as a server accepts TLS 1.2 or lower, it's vulnerable to attack.

I have a fix, and I can try to hide that fix under other work related to the release of RFC 8446, but this is hard to hide.

We might also want to check - as a client - for an all-zero ServerHello.random.

I believe that Firefox is not affected.  Our only use of NSS as a server is in DTLS and test code.  DTLS isn't affected by this problem.

We'll need a CVE on this one.  And new releases for any NSS version in the last 3 years that we still intend to support.
This retains the ability to negotiate draft versions of DTLS 1.3, but uses the
final RFC version for TLS 1.3.

This also refactors the handling of the downgrade sentinel.  As we've discovered
- to our dismay - some MitM boxes forward handshake messages when they
shouldn't.  This could result in triggering the downgrade sentinel.  I've done
two things here:

- The server always sets the sentinel.  It reduces the assumed version if it
  only supports a draft version though on the basis that the client might
  expect the full version.

- The client has a new option SSL_ENABLE_HELLO_DOWNGRADE_CHECK which is disabled
  by default.  The client will reject a handshake that appears to be a downgrade
  only when this is explicitly enabled.  The client will allow an apparent
  downgrade to TLS 1.2 if it is running a draft version of TLS 1.3.

The allowance for a draft version is now only effective for DTLS 1.3.

Tests for version downgrade have been updated and enabled.  These were rotten in
a few ways, but nothing dramatic.
This is indeed bad.

That said, I think your analysis isn't *quite* right. Because of the Finished, the attacker cannot tamper with the CH and have the outcome be acceptable to either client or server; the client's Finished will not verify and then neither will the server's. And because the server just signs over the random values, tampering with the CH isn't detectable at the SKE stage either.

The best attack I know of is that the attacker can replay the CH and any subsequent messages to the server. If you are using a block cipher, it's just a replay attack, but if you are using a stream cipher (RC4 or GCM) then of course you get very bad failure modes.
Note that the attack in c2 only works if the client offers a SSLv2 CH, because if the client offers an SSLv3 CH, then you go down the right code path.
Added an extra test for the downgrade sentinel before enabling false
start. 

I have associated this with BUg 1483128 to keep this hidden until we have a plan
for landing fixes.
Yeah, the key synchronization seems like it is still a problem, but Finished prevents the ClientHello from being malleable.  No sane client enables false start in the conditions under which this would be a problem for that either.
Bob,

This likely affects RedHat more than it does us.  It affects servers.

Can you let us know how you would like us to manage the fix?  The patches I have here are sufficient, and they disguise the issue somewhat, but we could also just disable the SSLv2-compatible ClientHello.

We would prefer to land the TLS 1.3 version changes soon, but we want to make sure we take the time to get everything in place and can wait until everything is ready if necessary.

Thanks.
Flags: needinfo?(rrelyea)
I would prefer to disable the SSLv2 CH if we can.
Thanks for identifying the issue.

I think martin can land his patches, but we shouldn't open this bug up until Red Hat security response can evaluate which releases we need to backport the issue.

We'll need to carry the fix in our older versions of RHEL (old versions of java still use this SSLv3 Hello). I think disabling SSLv2 hello in newer versions is fine (so having it disabled by default would be prefered).

By disable I prefer, "recognizing an SSL v2 hello and rejecting the connection".
Flags: needinfo?(rrelyea)
Let's give the security response people some time, just in case we have trouble.  In particular, we need to know which versions of NSS require fixes.  And what sort of fix is appropriate for each.

That is, for those older versions of RHEL, are you looking to ship NSS 3.39 (i.e., the current version), or are you OK with continuing to ship older NSS releases?  Ideally, the fix on NSS trunk is removal of the v2 hello support, though older branches could have a smaller correctness fix.

I don't want to add and maintain an option for this if at all possible.
I think landing the disclosed changes are fine. Daiki would like to pick up the fix for our existing NSS releases.
> That is, for those older versions of RHEL, are you looking to ship NSS 3.39 (i.e., the current version), or are you OK with 
> continuing to ship older NSS releases?  Ideally, the fix on NSS trunk is removal of the v2 hello support, 
> though older branches could have a smaller correctness fix.

We likely won't freeze NSS at any level until we pass the point where we need to support firefox on those platforms (for most libraries, we just freeze support, but because we need to keep firefox on a supported version, we need to rebase whenever we pick up a new version of ESR). I don't know the time frame for when we can drop firefox support on those releases (RHEL 6 is the issue).

For RHEL 8, I think we just disable v2 Hello support (so it will never be in the OS). I think we can deprecate the support in RHEL 7 (hopefully). The issue with RHEL 6 is we ship components that send ssl v2 hello messages to connect with SSL3, so we can't just turn it off in RHEL 6.

Initially for RHEL 7, we need to put the random fix in, and then turn SSL v2 hello off in a normal RHEL 7 release cycle (not in an async security fix patch).


Upshot: for the foreseeable future we'll need to keep some form of ssl v2 hello support in. When we EOL firefox support on RHEL 6, then we can drop it altogether. For the rest of the code base it should be turned off.

bob
OK, Daiki, if you are going to work on a fix for older branches, I will wait for that before landing any of these changes (which do not disable the v2 ClientHello) until that is ready.

It's unfortunate, but if we are going to start to phase this out in the way you describe here, then we'll need an option.  Compile- or run-time makes little difference to the maintenance overheads, so I'll leave that to you.
Add an option to disable the offending code.  This will need to go in
alongside the other fixes we have.
Comment on attachment 9001849 [details]
Bug 1483128 - Option to disable SSLv2-compatible ClientHello, r?ueno

Daiki Ueno [:ueno] has approved the revision.
Attachment #9001849 - Flags: review+
Comment on attachment 8999836 [details]
Bug 1483129 - TLS 1.3 RFC version, r?ekr

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Maybe easily.  The exploit is a key synchronization problem as a result of missing code.  The missing code is restored by this patch, but the title of the patch is unrelated.  (It's a fairly attractive patch title, as far as they go, but prosaic enough.)

https://phabricator.services.mozilla.com/D3581 is similarly unrelated, though it disables the affected feature.  I will request sec-approval for that too.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.

Which older supported branches are affected by this flaw?  Every NSS release in the past 3 years!

If not all supported branches, which bug introduced the flaw? This was an early commit for bug 1057463.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  The patch in https://phabricator.services.mozilla.com/D3581 also fixes the problem by disabling the affected code.  A smaller targeted patch is easy enough, though upgrading NSS is generally a better strategy.  A targeted patch will highlight the problem.

How likely is this patch to cause regressions; how much testing does it need?  Not significantly.  The affected code is otherwise well-tested.  Testing for randomness is tricky, that's all.
Attachment #8999836 - Flags: sec-approval?
Comment on attachment 9001849 [details]
Bug 1483128 - Option to disable SSLv2-compatible ClientHello, r?ueno

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not very.  It disables the code entirely, as we might be expected to do anyway.  There's a lot of code to explore to find it, and it's not the usual sort of problem (buffer overrun, UMR, UAF, etc...)  That it is on a security sensitive bug number is a red flag, but finding the problem isn't trivial.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  No.

Which older supported branches are affected by this flaw?  Everything in the past 3 years.

If not all supported branches, which bug introduced the flaw? See Comment 15.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Backporting this is relatively trivial.  Backporting a targeted fix will paint a nice red target on the problem, but it is possible.

How likely is this patch to cause regressions; how much testing does it need? This is trivial code.
Attachment #9001849 - Flags: sec-approval?
Note that the other attachment (D3382) is only here as a result of a dependency on the secure bug.  Once the others land, it's not going to make things worse.  It's just that it is based on sensitive code.
Is Mozilla planning to assign a cve to this issue, or Red Hat should?
Sec-approval+ for trunk.
We're going to want a Beta and ESR60 patch made and nominated. The last beta is on Thursday the 23rd so we need these patches in time to get into Thursday's build.

Mozilla will be assigning the CVE.
Attachment #9001849 - Flags: sec-approval? → sec-approval+
Note that it's only rh
Karthik, Benjamin, and I talked through this today. 

Here's our initial analysis:

1. My analysis in c2 is mostly correct.
2. It probably *is* possible to get False Start with an SSLv2 CH (if you just don't supply any groups, I believe we assume you just support the ones we support). However, the server can't write at this point, and the client's Finished precedes the clients' data and that should cause a Finished failure
3. We don't believe that it's possible to take an SSLv3 CH and by modifying the record headers make it an acceptable SSLv2 CH. The reason is that the 2nd byte of the SSLv2CH is the version and must be 3, but that is the first byte of the 3-byte length in SSLv3 CH and nobody sends an SSLv3 CH that is 65K long.
4. If the client supports a weak hash, and the server reuses its DH share (as NSS does for ECDH) you are susceptible to a SLOTH variant because the server doesn't contribute any randomness and therefore can be used as a signing oracle if you can produce a hash collision.


Two more notes:
1. I'm not entirely sure why I moved this and I wonder if we could just move it back to the code to send the server hello. MT, can you check and see if we need it at any time before that?
2. I note that we do less sanity checking of the SSLv2 CH message than we could. In particular (a) we don't check the message type and (b) we don't check that the SID isn't a crazy length. We should probably do both of these.
Oh, and (c) we don't seem to check that the cipher suite length is a multiple of 3.
Attachment #8999836 - Flags: sec-approval? → sec-approval+
(In reply to Eric Rescorla (:ekr) from comment #20)
> Note that it's only rh

Yeah, we won't issue a Firefox advisory (it should go in NSS release notes, maybe some other RedHat announcements), but as a mozilla.org project NSS falls under our CVE issuing role.
I think that ekr was addressing the request for backports for Firefox.  I've removed the tracking flags, because Firefox is unaffected.  (Maybe I shouldn't have asked for sec-approval here...)
> 1. I'm not entirely sure why I moved this and I wonder if we could just move it back to the code to send the server hello. MT, can you check and see if we need it at any time before that?

Yes, we're OK.  The variable isn't read until it's written out in ssl3_SendServerHello.  I will move the code back there.

The nice thing about moving it is that it hides the bug much better.

> 2. I note that we do less sanity checking of the SSLv2 CH message than we could. In particular (a) we don't check the message type and (b) we don't check that the SID isn't a crazy length. We should probably do both of these.

Yes, but I'm just disabling that code instead.  We can fix those later.
Comment on attachment 9001849 [details]
Bug 1483128 - Option to disable SSLv2-compatible ClientHello, r?ueno

Eric Rescorla (:ekr) has approved the revision.
Attachment #9001849 - Flags: review+
In terms of the schedule, there are three important events:

1. Land in tree
2. Ship updated NSS to RH customers
3. Issue an advisory

I'm expecting we do #1 this week. Bob, Daiki, what are your thoughts on #2 and #3.
Flags: needinfo?(rrelyea)
Flags: needinfo?(dueno)
(In reply to Eric Rescorla (:ekr) from comment #27)

> 3. Issue an advisory
> 
> I'm expecting we do #1 this week. Bob, Daiki, what are your thoughts on #2
> and #3.

Historically, there have been NSS release notes but we haven't issued NSS only advisories as Mozilla. When we have written advisories for NSS, it has been code that affected Firefox and we issued them within the Firefox advisories for the release of Firefox with which it shipped.
Al @c28: OK, but this does not affect Firefox, AFAICT. So what do you propose?
I think Redhat should issue an advisory if they're willing.

I'll assign CVE-2018-12384 from our pool (after double-checking with Dan earlier).
Alias: CVE-2018-12384
WFM, Redhat folks?
Comment on attachment 8999836 [details]
Bug 1483129 - TLS 1.3 RFC version, r?ekr

Eric Rescorla (:ekr) has approved the revision.
Attachment #8999836 - Flags: review+
Comment on attachment 9000132 [details]
Bug 1483416 - Disable false start if there might be a downgrade, r?ekr

Eric Rescorla (:ekr) has approved the revision.
Attachment #9000132 - Flags: review+
Kai, Bob, Daiki, this is all ready to go, but I want to make sure that we're OK with these changes before I land them.

ekr proposed:

1. Land in tree
2. Ship updated NSS to RH customers
3. Issue an advisory

We need to get NSS released.  This advisory and the release notes can go together, but I want to make sure that RedHat are ready.
Flags: needinfo?(kaie)
I haven't yet seen advice from RH/Huzaifa about their requested timing for fixing this in RHEL.
RH isn't ready yet. We don't have the backported patch yet. There has been no decision yet where to backport it.

At this time, you want to land a fix into NSS trunk, which gets consumed by NSS 3.39 and Firefox 63, only.
You're obscuring the fix. It's expected that it will take attackers a while until they understand the issue and how to exploit it.

Based on that, I think it's OK for Mozilla to go ahead and land the obscured fix into NSS trunk and FF 63, without waiting for RH.

Thanks for not yet talking about the issue in public. This will give us a few more weeks of time to prepare a rollout of backports of the fix.

I suspect that we missed the deadline for adding a fix to the ESR 60.3 release that will go out at the same time as FF 63.

We'll require backports for NSS 3.36.x, used by ESR 60. It could target ESR 60.4.
It depends on feedback from Huzaifa and RH's security team, if fixes for older branches are requested, too.
Flags: needinfo?(kaie)
It's probably obvious, but just be clear, when backporting to 3.36.x and older, we'll need to keep support for v2 compatible CH.
MT, do you want to create a separate phabricator revision for this attached to the main TLS 1.3 bug and I'll then LGTM it?
Sorry I missed that I had some questions here. Yes eric's plan looks fine.


BTW if you posted questions for me with phabricator (which isn't the case here), I'm not able to see them. I tried to get add a token to phabricator to be able to use it, and I wound up blocking my access to bugzilla. :(. I seem to be able to see unsecure phabricator things (though I can't comment as me).
Flags: needinfo?(rrelyea)
This isn't going to work for 3.36, which is stuck way back on -23.  I tried, but it's too much code to integrate.  So I have a patch that will move the randomness back in.  That's going to make this a little obvious, so I will leave it to the RH folks to sort out when they want it (noting again that we don't need the fix in Firefox, so ESR timing doesn't matter).
This is the simpler fix.  It's making the bug pretty obvious though.
Landed all the patches:
https://hg.mozilla.org/projects/nss/rev/ee357b00f2e6c44589dcd406097357888d59ef06
https://hg.mozilla.org/projects/nss/rev/91b6fb509cb01bbf4466f647cccfa2a2e060c504
https://hg.mozilla.org/projects/nss/rev/2ed9f6afd84eb7fd1033ffaf448655e1d5a79bb8

As I mentioned, I will leave the 3.36 patch for RH folks to manage, since that is more likely to reveal the problem.
Flags: needinfo?(dueno)
Now that this is landed, I think that we need to get notice out.  We should give this a week.  After that, I think that we need to make some sort of announcement.  For me, that means including notice in the NSS release notes.
(In reply to Martin Thomson [:mt:] from comment #42)
> Now that this is landed, I think that we need to get notice out.  We should
> give this a week.  After that, I think that we need to make some sort of
> announcement.  For me, that means including notice in the NSS release notes.

Huzaifa, what do you think?
Flags: needinfo?(huzaifas)
(In reply to Martin Thomson [:mt:] from comment #41)
> Landed all the patches:

Thanks. I conclude we're ready to start the 3.39 release branch.
Regarding what Red Hat needs: I've recently verified that OpenSSL 1.1.1 still supports SSLv2 Hello so I'd be vary of removing it in RHEL-8 releases. It definitely has to stay in RHEL-7 and earlier. We will probably be able to remove it completely only with the removal of TLS 1.0, and deprecate it with deprecation of TLS 1.0.
We can't easily test that ClientHello.random and ServerHello.random are truly
random in these tests, but we can catch mistakes the likes of which produced
this bug.  This just runs a few handshakes and tests that none of the random
values are equal to any other, or they are equal to zero.
The patch I just landed disables the SSLv2-compatible ClientHello by default.  I think that's the right setting for new deployments.  I'm happy to have it retained (modulo fixing the problems in comment 21) behind the option for some time; if RHEL-8 needs this on by default, though I would discourage doing that, then RH will need to maintain a patch for the default value.
Comment on attachment 9003979 [details]
Bug 1483128 - Move random generation, r?ekr

Eric Rescorla (:ekr) has approved the revision.
Attachment #9003979 - Flags: review+
Hello,

We are ok with making this flaw public, whenever you want. Please go ahead and do an advisory if you want!
Flags: needinfo?(huzaifas)
(In reply to Huzaifa Sidhpurwala from comment #49)
> Hello,
> 
> We are ok with making this flaw public, whenever you want. Please go ahead
> and do an advisory if you want!

See comment 30. The previous request was for Redhat to handle this (if anyone). 

Mozilla isn't issuing an advisory for something that doesn't affect Firefox. We don't have an NSS advisory page and have, historically, only mentioned NSS in advisories in the context of Firefox (and Thunderbird).
Flags: needinfo?(huzaifas)
Comment on attachment 9004441 [details]
Bug 1483128 - Test that randoms aren't fixed, r?ekr

Eric Rescorla (:ekr) has approved the revision.
Attachment #9004441 - Flags: review+
(In reply to Kai Engert (:kaie:) from comment #51)
> We published this issue in the NSS release notes on Friday:
> https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.
> 39_release_notes
> https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.36.
> 5_release_notes

Thanks, Kai.
Flags: needinfo?(huzaifas)
Test landed (3.40 only): https://hg.mozilla.org/projects/nss/rev/f182a11fbe532b1b9edb76a57a0d4609d9d8ab75
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.39
Group: crypto-core-security → core-security-release
Attachment #9003979 - Attachment is obsolete: true
Group: core-security-release
QA Contact: franziskuskiefer
You need to log in before you can comment on or make changes to this bug.