Closed Bug 1377618 (CVE-2017-7805) Opened 7 years ago Closed 7 years ago

Potential UAF in TLS 1.2 server when verifying client authentication

Categories

(NSS :: Libraries, defect)

3.25
defect
Not set
normal

Tracking

(firefox-esr5256+ fixed, firefox55 wontfix, firefox56+ fixed, firefox57+ fixed)

RESOLVED FIXED
Tracking Status
firefox-esr52 56+ fixed
firefox55 --- wontfix
firefox56 + fixed
firefox57 + fixed

People

(Reporter: mt, Assigned: mt)

References

Details

(Keywords: csectype-uaf, regression, sec-high, Whiteboard: [adv-main56+][adv-esr52.4+][post-critsmash-triage] (can affect Firefox via WebRTC))

Attachments

(2 files)

This is a regression caused by Bug 1179338, but I only noticed it when I made some changes to the sslBuffer functions.

In TLS 1.2, handshake hashes are calculated at the time they are needed.  The handshake hashes are tweaked to include a pointer to the message buffer in the field pointer_to_hash_input:

https://searchfox.org/nss/rev/b5e2bb64574eefa20a9143d908bd4e09a125070e/lib/ssl/ssl3con.c#11668

This is necessary because the hashes need to cover the previous handshake message, but not the current message.  Later in the same function, the saved transcript is updated with the current message.  ssl3_UpdateHandshakeHashes() is called to add the current handshake message.  For TLS 1.2, this means appending those to ss->ssl3.hs.messages, which is an sslBuffer.

The problem is that if the handshake transcript exceeds the space available in the current buffer, a new buffer will be allocated.  This is fairly unlikely, because the buffer is initialized to (MAX_FRAGMENT_LENGTH + 2048), which is 18k.  However, if the handshake transcript up and including the client Certificate is almost 18k, then this reallocation will occur when handling the CertificateVerify as the total transcript goes over 18k.

The value of pointer_to_hash_input is left pointing at a freed buffer and you get a UAF when the handshake hashes (needed to verify the client's signature) are calculated.

I only noticed this because I changed sslBuffer_Grow to start at a lower initial value and it happened.

The fix here is pretty simple.  I've attached a patch.  I plan to land this as part of a bunch of other work, which should hide the change, however that is only on a branch currently and won't merge into NSS proper for some time.

Kai, do you think we need to land this in trunk or uplift it anywhere?
Attachment #8882762 - Flags: feedback?(kaie)
Whiteboard: Server only; Firefox not affected
Martin, thanks for reporting this bug.

Since Daniel marked this as sec-high, I think we should fix it for firefox-ESR, which would mean fixing the NSS 3.28.x branch.

The question is which timing we should use. The next release opportunity is August 8, not sure if we can/should turn around that quickly, or rather target the September 26.

I think that we should fix the ESR branch an a future regular Firefox release with the identical timing.

So, it's either:
(a)
Aug 8
Firefox 55 + NSS 3.31.x
Firefox ESR 52.3 + NSS 3.28.x

or:
(b)
Sep 26
Firefox 56 + NSS 3.32.x
Firefox ESR 52.4 + NSS 3.28.x

Because this is rated sec-high, we might want to delay the NSS trunk commit until closer to the intended release date.

Daniel, any thoughts on the preferred timing from the Mozilla side?
Flags: needinfo?(dveditz)
I landed this in the NSS_TLS13_DRAFT19_BRANCH as part of Bug 1368980.
Note that I missed the "Firefox not affected" part.  It is, via WebRTC - a case where we always use client authentication, so it's perhaps more likely than otherwise.
Whiteboard: Server only; Firefox not affected → Server only
Is Mozilla going to assign a CVE id to this flaw or Red Hat should?
We're waiting on Dan to make that call.
(In reply to Kai Engert (:kaie:) from comment #1)
> Daniel, any thoughts on the preferred timing from the Mozilla side?

I don't have much of a preference. Earlier is usually better but we're coming up pretty close to the 55 release.

(In reply to Huzaifa Sidhpurwala from comment #5)
> Is Mozilla going to assign a CVE id to this flaw or Red Hat should?

Mozilla usually assigns CVEs for NSS. You can use CVE-2017-7805 for this one.
Alias: CVE-2017-7805
Flags: needinfo?(dveditz)
Whiteboard: Server only → (can affect Firefox via WebRTC)
Dan, another question, since RyanVM had suggested it a while ago:

If we fix this for ESR, do you want bug 1273678 (with regression fix bug 1381784) as ride-along fixes for ESR, too?
Flags: needinfo?(dveditz)
Comment on attachment 8882762 [details] [diff] [review]
use length only, not a pointer

Clearly Kai thinks that this is OK :)
Attachment #8882762 - Flags: feedback?(kaie) → review+
Landed on trunk: https://hg.mozilla.org/projects/nss/rev/839200ce0943166a079284bdf45dcc37bb672925

Now we have 3.28, 3.33 and the TLS 1.3 branch.  Anything else?
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Group: crypto-core-security → core-security-release
Track 56+/57+ as sec-high.
Hi Dan,
Do you think if it's worth uplifting to Beta56?
(In reply to Martin Thomson [:mt:] from comment #10)
> Landed on trunk:
> https://hg.mozilla.org/projects/nss/rev/
> 839200ce0943166a079284bdf45dcc37bb672925
> 
> Now we have 3.28, 3.33 and the TLS 1.3 branch.  Anything else?

I don't see a commit for 3.28.x yet.
(In reply to Kai Engert (:kaie:) from comment #8)
> If we fix this for ESR, do you want bug 1273678 (with regression fix bug
> 1381784) as ride-along fixes for ESR, too?

I don't think so. It wasn't causing that many stability issues, had at least one serious uncaught regression, and I'm not really worried about a racy shutdown bug being turned into an exploit.

(In reply to Gerry Chang [:gchang] from comment #12)
> Do you think if it's worth uplifting to Beta56?

_This_ bug, yes -- we need to fix it in Beta 56 and ESR.
Flags: needinfo?(dveditz)
Comment on attachment 8882762 [details] [diff] [review]
use length only, not a pointer

Requesting firefox-56-beta approval, for uplifing this change in an updated NSS snapshot.

(Firefox 56 currently uses NSS 3.32, we'd create a new NSS 3.32.1 release, with this fix being the only code change in the updated NSS release.)


Approval Request Comment
[Feature/Bug causing the regression]: bug 1179338

[User impact if declined]: sec-high use-after-free

[Is this code covered by automated tests?]: yes

[Has the fix been verified in Nightly?]: yes (with NSS 3.33 beta code)

[Needs manual test from QE? If yes, steps to reproduce]: no

[List of other uplifts needed for the feature/fix]: none

[Is the change risky?]: no

[Why is the change risky/not risky?]: Scope of the affected code is small,
                                      therefore easy to review for correctness.
                                      The incorrectly used variable is removed
                                      and it's tested.

[String changes made/needed]: none
Attachment #8882762 - Flags: approval-mozilla-beta?
Comment on attachment 8886591 [details] [diff] [review]
patch merged to 3.28 branch

Requesting firefox-esr-52.4 approval, for uplifing this change in an updated NSS snapshot.

(Firefox ESR 52.x currently uses NSS 3.28.5, we'd create a new NSS 3.28.6 release, with this fix being the only code change in the updated NSS release.)


[Approval Request Comment]

sec-high

User impact if declined:
  sec-high use-after-free

Fix Landed on Version: 
  57 (suggested for 56)

Risk to taking this patch (and alternatives if risky):
  Not risky, scope of the affected code is small,
  therefore easy to review for correctness.
  The incorrectly used variable is removed and it's tested.

String or UUID changes made by this patch:
  none

(See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.)
Attachment #8886591 - Flags: approval-mozilla-esr52?
Comment on attachment 8882762 [details] [diff] [review]
use length only, not a pointer

Fix a sec-high. Beta56+.
Attachment #8882762 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8886591 [details] [diff] [review]
patch merged to 3.28 branch

Fix a sec-high. ESR52+. Let's uplift this to ESR52.4.
Attachment #8886591 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Dan, Gerry, do you think we should postpone creation of NSS branch releases and uplifting to Firefox branches until late in the Firefox release cycle, to avoid drawing attention to this fix?

(I think any creation of an NSS release branch rings a bell for anyone looking for exploits.)

What is the deadline for landing into beta/esr52 to get the fix included in the release?

How about Monday September 18 ?
Flags: needinfo?(gchang)
I'm ok with it as long as we can make sure this patch needs to be in m-b before merging to m-r. But, we still need Dan's second opinion on this.
Flags: needinfo?(gchang) → needinfo?(dveditz)
Right, given that beta merges over a bit earlier, we probably should do it a bit earlier.

How about creating the branches/releases by September 11 ?
Maybe even earlier in case we have problems and need to diagnose, fix, and uplift. 
Al, what do you think?
Flags: needinfo?(abillings)
I think September 11 would be fine.
Flags: needinfo?(abillings)
(In reply to Gerry Chang [:gchang] from comment #20)
> I'm ok with it as long as we can make sure this patch needs to be in m-b
> before merging to m-r. But, we still need Dan's second opinion on this.

That sounds good to me, too.
Flags: needinfo?(dveditz)
I didn't receive bugmail for comment 22, 23, 24.
Last I had received was for comment 20.
I wonder what's wrong and which other emails I've missed.
The key in my bugzilla secure email preferences is a S/MIME cert that has expired in 2013 already, so that wouldn't explain why it stopped working recently.
Maybe I should replace that with a currently valid GPG key...
Comment on attachment 8882762 [details] [diff] [review]
use length only, not a pointer

Landed into NSS 3.32 branch:
https://hg.mozilla.org/projects/nss/rev/2d7b65b7229020ff68d015f7c874b0446caf8240

(nss rtm creation and rtm uplift still pending)
Comment on attachment 8886591 [details] [diff] [review]
patch merged to 3.28 branch

Landed into NSS 3.28 branch:
https://hg.mozilla.org/projects/nss/rev/d3865e2957d0e5aeb32abb9d61b9eb6e8b9da4b8

(nss rtm creation and rtm uplift still pending)
The NSS releases are ready.

NSS_3_32_1_RTM must be uplifted to the Firefox 56 Beta branch.

NSS_3_28_6_RTM must be uplifted to the Firefox ESR 52 branch.

We must adjust the minimum version requirements in old-configure.in
Whiteboard: (can affect Firefox via WebRTC) → [adv-main56+][adv-esr52.4+] (can affect Firefox via WebRTC)
Flags: qe-verify-
Whiteboard: [adv-main56+][adv-esr52.4+] (can affect Firefox via WebRTC) → [adv-main56+][adv-esr52.4+][post-critsmash-triage] (can affect Firefox via WebRTC)
(In reply to Kai Engert (:kaie:) from comment #25)
> I didn't receive bugmail for comment 22, 23, 24.

false alert, bad filter on my side
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.