UAF in nsMIMEHeaderParamImpl::DecodeRFC5987Param in nsMIMEHeaderParamImpl.cpp

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: tbourvon, Assigned: tbourvon, Mentored)

Tracking

({sec-critical})

unspecified
mozilla56
Points:
---
Dependency tree / graph
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 nsMIMEHeaderParamImpl::DecodeRFC5987Param in nsMIMEHeaderParamImpl.cpp on line 824. It detects the use of pointers owned by temporaries after they are deleted (https://bugzilla.mozilla.org/show_bug.cgi?id=1374024).

> mozilla-central/netwerk/mime/nsMIMEHeaderParamImpl.cpp:824:55: error: calling `get` on a temporary, potentially allowing use after free of the raw pointer [mozilla-dangling-on-temporary]
>   const char *encoded = PromiseFlatCString(aParamVal).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). |encoded| now points to freed memory.
Assignee

Comment 1

2 years ago
Posted patch 1383002.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 #8888778 - Flags: review?(mcmanus)
Comment on attachment 8888778 [details] [diff] [review]
1383002.patch

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

thanks!
Attachment #8888778 - Flags: review?(mcmanus) → review+
Given that this bug goes back to Gecko 15 AFAICT and UAFs are almost always sec-high/crit, this should get sec-approval before landing.
https://wiki.mozilla.org/Security/Bug_Approval_Process
Assignee

Comment 4

2 years ago
Comment on attachment 8888778 [details] [diff] [review]
1383002.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It's always hard to construct an attack on a UAF. However, this method decodes user content and performs allocations based on user content (when calling |charset.Append(tc)| for example), so it is especially at risk.

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?
I'm guessing the patch can very easily be backported.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely. It runs fine on try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=539a94e1e172f271bfe0107ccd96a5bda1e4ddfa&selectedJob=116383455) and this should probably be enough to get it landed.
Flags: needinfo?(tbourvon)
Attachment #8888778 - Flags: sec-approval?
sec-approval+ for trunk.

Ritu, can we take this on Beta? It is a two line change.
Flags: needinfo?(rkothari)
Keywords: sec-critical
Attachment #8888778 - Flags: sec-approval? → sec-approval+
Tristan, we need a backported patch nominated for ESR52 and Beta ASAP if we're to get this into the next release.
Flags: needinfo?(tbourvon)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a444c40f63b397dbc67e71ab187fd4110841ecc

This grafts cleanly to Beta and ESR52. Also, we still have 2 more betas left this cycle, so there should be plenty of time to get this uplifted around.
Flags: needinfo?(tbourvon)
We still need the patch to be nominated. :-)
https://hg.mozilla.org/mozilla-central/rev/4a444c40f63b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Please request Beta & ESR52 approval on this when you get a chance.
Flags: needinfo?(tbourvon)
Assignee

Comment 11

2 years ago
Comment on attachment 8888778 [details] [diff] [review]
1383002.patch

Approval Request Comment
[Feature/Bug causing the regression]: UAF in nsMIMEHeaderParamImpl::DecodeRFC5987Param
[User impact if declined]: security vulnerability left open
[Is this code covered by automated tests?]: Not sure, but after looking at the tree I couldn't find any
[Has the fix been verified in Nightly?]: It builds in Nightly, but hasn't been tested
[Needs manual test from QE? If yes, steps to reproduce]: Probably doesn't need tests
[List of other uplifts needed for the feature/fix]: ESR52
[Is the change risky?]: No
[Why is the change risky/not risky?]: This is a very minor code change using a pattern which is proven to work by being used in many places
[String changes made/needed]:
Flags: needinfo?(tbourvon)
Attachment #8888778 - Flags: approval-mozilla-esr52?
Attachment #8888778 - Flags: approval-mozilla-beta?
> [Is this code covered by automated tests?]: Not sure, but after looking at
> the tree I couldn't find any
It is covered:
https://codecov.io/gh/marco-c/gecko-dev/src/master/netwerk/mime/nsMIMEHeaderParamImpl.cpp#L824
Comment on attachment 8888778 [details] [diff] [review]
1383002.patch

uaf, sec-crit, beta55+ and esr52.3+
Flags: needinfo?(rkothari)
Attachment #8888778 - Flags: approval-mozilla-esr52?
Attachment #8888778 - Flags: approval-mozilla-esr52+
Attachment #8888778 - Flags: approval-mozilla-beta?
Attachment #8888778 - Flags: approval-mozilla-beta+
Whiteboard: [adv-main55+][adv-esr52.3+]
Group: core-security → core-security-release
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.