Closed Bug 1382994 Opened 2 years ago Closed 2 years ago

UAF in InitializeNSSWithFallbacks in nsNSSComponent.cpp

Categories

(Core :: Security: PSM, defect, P1)

54 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- wontfix
firefox55 + fixed
firefox56 + fixed

People

(Reporter: tbourvon, Assigned: tbourvon, Mentored)

References

Details

(Keywords: regression, sec-audit, Whiteboard: [psm-assigned][adv-main55-][post-critsmash-triage])

Attachments

(1 file)

Our WIP static-analysis checker has identified a UAF in InitializeNSSWithFallbacks in nsNSSComponent.cpp on line 1718. It detects the use of pointers owned by temporaries after they are deleted (https://bugzilla.mozilla.org/show_bug.cgi?id=1374024).

> mozilla-central/security/manager/ssl/nsNSSComponent.cpp:1718:65: error: calling `get` on a temporary, potentially allowing use after free of the raw pointer [mozilla-dangling-on-temporary]
>   const char* profilePathCStr = PromiseFlatCString(profilePath).get();
>                                                                 ^

Here |get()| returns a pointer to memory owned by |PromiseFlatCString|, which gets freed as soon as the temporary |PromiseFlatCString| is deleted (i.e. as soon as the assignment is over). |profilePathCStr| now points to freed memory.
Attached patch 1382994.patchSplinter Review
This patch fixes the UAF by keeping the temporary alive for the whole scope of the function.
It uses the method suggested in the heading of xpcom/string/nsTPromiseFlatString.h, which is to bind a const ref to the temporary, extending its lifetime to match the one of the reference.

This method copies the |nsCString| object to the stack, but this is a very minor cost and is better than the other solutions (such as duplicating the |const char*| returned by |.get()| or reusing |PromiseFlatCString(...).get()| multiple times at the call sites).
Attachment #8888767 - Flags: review?(ttaubert)
Comment on attachment 8888767 [details] [diff] [review]
1382994.patch

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

LGTM, but I'm not a PSM peer. Let's ask David.
Attachment #8888767 - Flags: review?(ttaubert)
Attachment #8888767 - Flags: review?(dkeeler)
Attachment #8888767 - Flags: feedback+
Comment on attachment 8888767 [details] [diff] [review]
1382994.patch

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

D'oh. Thanks.
For future reference, any bugs in this directory can go in Core :: Security: PSM.
Also, looks like we've got at least a few more instances of this if your analysis hasn't already found them:

https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/modules/libjar/nsJAR.cpp#328
https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/netwerk/mime/nsMIMEHeaderParamImpl.cpp#824
Attachment #8888767 - Flags: review?(dkeeler) → review+
Component: Security → Security: PSM
Priority: -- → P1
Whiteboard: [psm-assigned]
(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> Comment on attachment 8888767 [details] [diff] [review]
> 1382994.patch
> 
> Review of attachment 8888767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> D'oh. Thanks.
> For future reference, any bugs in this directory can go in Core :: Security:
> PSM.
> Also, looks like we've got at least a few more instances of this if your
> analysis hasn't already found them:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 7d2e89fb92331d7e4296391213c1e63db628e046/modules/libjar/nsJAR.cpp#328
> https://dxr.mozilla.org/mozilla-central/rev/
> 7d2e89fb92331d7e4296391213c1e63db628e046/netwerk/mime/nsMIMEHeaderParamImpl.
> cpp#824

My analysis found exactly 3 bugs, this one and the two you linked!
Looks like we don't need it after all, we could just ask you to find them...
This goes back to Fx54 AFAICT. Please request sec-approval on this before landing :)
Blocks: 1337950
Group: core-security → crypto-core-security
Version: unspecified → 54 Branch
Flags: needinfo?(tbourvon)
Comment on attachment 8888767 [details] [diff] [review]
1382994.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
UAFs are hard to exploit, but this one is in the middle of NSS initialization where lots of allocations happen, so the risk is definitely increased (and in a critical part of the code).

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

Which older supported branches are affected by this flaw?
This was introduced in Firefox 54 and affects all later branches.

If not all supported branches, which bug introduced the flaw?
N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should be easy to backport without too many issues.

How likely is this patch to cause regressions; how much testing does it need?
The fix is fairly straightforward and not very risky, it probably doesn't need testing.
Flags: needinfo?(tbourvon)
Attachment #8888767 - Flags: sec-approval?
Calling this a sec-audit and clearing sec-approval (as it doesn't apply to audit bugs). You can check this into trunk but if you think we should take this in Beta and ESR52, a case will need to be made this late in the cycle.

Since this only goes back to 54, a case could be made to take it on 55 though.
Keywords: sec-audit
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fd23c6134b93c256a2bdca91eacc60b6c43c662

I've confirmed this grafts cleanly Beta. I think we should nominate it for approval since it's UAF and the odds of someone trying to attack it go up now that it's out there. Patch is simple enough anyway.
Flags: needinfo?(tbourvon)
Attachment #8888767 - Flags: sec-approval?
Comment on attachment 8888767 [details] [diff] [review]
1382994.patch

Approval Request Comment
[Feature/Bug causing the regression]: UAF in InitializeNSSWithFallbacks
[User impact if declined]: users left potentially vulnerable to this security issue
[Is this code covered by automated tests?]: Yes (https://codecov.io/gh/marco-c/gecko-dev/src/master/security/manager/ssl/nsNSSComponent.cpp#L1718)
[Has the fix been verified in Nightly?]: builds but not hand-tested
[Needs manual test from QE? If yes, steps to reproduce]: probably not
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: very low risk
[Why is the change risky/not risky?]: small change with a proven method
[String changes made/needed]:
Flags: needinfo?(tbourvon)
Attachment #8888767 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/5fd23c6134b9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8888767 [details] [diff] [review]
1382994.patch

let's get this in 55.0b13, seems safe enough
Attachment #8888767 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [psm-assigned] → [psm-assigned][adv-main55-]
Group: crypto-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [psm-assigned][adv-main55-] → [psm-assigned][adv-main55-][post-critsmash-triage]
Group: core-security-release

Tristan, it looks like you are the assignee on this bug, but under a different email. Are you still working on this bug? If not, would you please unassign yourself?

Flags: needinfo?(tristanbourvon)

Sorry for the spurious NI request, missed that this bug had been resolved.

Flags: needinfo?(tristanbourvon)
You need to log in before you can comment on or make changes to this bug.