1377618 - (CVE-2017-7805) Potential UAF in TLS 1.2 server when verifying client authentication

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Martin Thomson [:mt:]]

Attachment: use length only, not a pointer

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?

Comment 1

[Kai Engert [:KaiE:]]

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?

Comment 2

[Kai Engert [:KaiE:]]

Attachment: patch merged to 3.28 branch

Comment 3

[Martin Thomson [:mt:]]

I landed this in the NSS_TLS13_DRAFT19_BRANCH as part of Bug 1368980.

Comment 4

[Martin Thomson [:mt:]]

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.

Comment 5

[Huzaifa Sidhpurwala]

Is Mozilla going to assign a CVE id to this flaw or Red Hat should?

Comment 6

[Martin Thomson [:mt:]]

We're waiting on Dan to make that call.

Comment 7

[Daniel Veditz [:dveditz]]

(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.

Comment 8

[Kai Engert [:KaiE:]]

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?

Comment 9

[Martin Thomson [:mt:]]

Comment on attachment 8882762 [details] [diff] [review]
use length only, not a pointer

Clearly Kai thinks that this is OK :)

Comment 10

[Martin Thomson [:mt:]]

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?

Comment 11

[Gerry Chang [:gchang]]

Track 56+/57+ as sec-high.

Comment 12

[Gerry Chang [:gchang]]

Hi Dan,
Do you think if it's worth uplifting to Beta56?

Comment 13

[Kai Engert [:KaiE:]]

(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.

Comment 14

[Daniel Veditz [:dveditz]]

(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.

Comment 15

[Kai Engert [:KaiE:]]

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

Comment 16

[Kai Engert [:KaiE:]]

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.)

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8882762 [details] [diff] [review]
use length only, not a pointer

Fix a sec-high. Beta56+.

Comment 18

[Gerry Chang [:gchang]]

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.

Comment 19

[Kai Engert [:KaiE:]]

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 ?

Comment 20

[Gerry Chang [: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.

Comment 21

[Kai Engert [:KaiE:]]

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 ?

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Maybe even earlier in case we have problems and need to diagnose, fix, and uplift. 
Al, what do you think?

Comment 23

[Al Billings [:abillings - ex-MoCo]]

I think September 11 would be fine.

Comment 24

[Daniel Veditz [:dveditz]]

(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.

Comment 25

[Kai Engert [:KaiE:]]

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 26

[Kai Engert [:KaiE:]]

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 27

[Kai Engert [:KaiE:]]

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)

Comment 28

[Kai Engert [:KaiE:]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a4c4f7507a05
https://hg.mozilla.org/releases/mozilla-esr52/rev/113da8d46aa4

Comment 30

[Kai Engert [:KaiE:]]

(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