UAF in nsJAR::GetInputStreamWithSpec in nsJAR.cpp

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: tbourvon, Assigned: tbourvon, Mentored)

Tracking

({sec-audit})

unspecified
mozilla56
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr5255+ fixed, firefox54 wontfix, firefox55+ fixed, firefox56+ fixed)

Details

(Whiteboard: [adv-main55-][adv-esr52.3-][post-critsmash-triage])

Attachments

(1 attachment)

(Assignee)

Description

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

> mozilla-central/modules/libjar/nsJAR.cpp:328:54: error: calling `get` on a temporary, potentially allowing use after free of the raw pointer [mozilla-dangling-on-temporary]
>   const char *entry = PromiseFlatCString(aEntryName).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). |entry| now points to freed memory.
(Assignee)

Comment 1

2 years ago
Posted patch 1383000.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 #8888773 - Flags: review?(aklotz)
Attachment #8888773 - Flags: review?(aklotz) → review+
Ouch, goes back to Gecko 10 from the looks of it. Please request sec-approval on this one too :)
Flags: needinfo?(tbourvon)
(Assignee)

Comment 3

2 years ago
Comment on attachment 8888773 [details] [diff] [review]
1383000.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not very easily. It's a hard to exploit UAF, but you never know.

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?
All of them.

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 by itself.

How likely is this patch to cause regressions; how much testing does it need?
Very low risks, testing probably not required.
Flags: needinfo?(tbourvon)
Attachment #8888773 - 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.
Keywords: sec-audit
Attachment #8888773 - Flags: sec-approval?
https://hg.mozilla.org/integration/mozilla-inbound/rev/22649352b9f4137bb410fcfffbd1c2fa5ca0659c

I've confirmed this grafts cleanly Beta and ESR52. I think we should nominate it for approval for both branches 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)
(Assignee)

Comment 6

2 years ago
Comment on attachment 8888773 [details] [diff] [review]
1383000.patch

Approval Request Comment
[Feature/Bug causing the regression]: UAF in nsJAR::GetInputStreamWithSpec
[User impact if declined]: users potentially left open to this vulnerability
[Is this code covered by automated tests?]: Yes https://codecov.io/gh/marco-c/gecko-dev/src/master/modules/libjar/nsJAR.cpp#L328
[Has the fix been verified in Nightly?]: built but not hand-tested
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: Beta/ESR52
[Is the change risky?]: very low risk
[Why is the change risky/not risky?]: minor changes with a proven method
[String changes made/needed]:
Flags: needinfo?(tbourvon)
Attachment #8888773 - Flags: approval-mozilla-esr52?
Attachment #8888773 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/22649352b9f4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8888773 [details] [diff] [review]
1383000.patch

uaf fix, beta55+
Attachment #8888773 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [adv-main55-]
Comment on attachment 8888773 [details] [diff] [review]
1383000.patch

Fix a security issue. Let's uplift it to ESR52.3.
Attachment #8888773 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Group: core-security → core-security-release
Whiteboard: [adv-main55-] → [adv-main55-][adv-esr52.3-]
Flags: qe-verify-
Whiteboard: [adv-main55-][adv-esr52.3-] → [adv-main55-][adv-esr52.3-][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.