Closed Bug 1500759 Opened 10 months ago Closed 10 months ago

AddressSanitizer: use-after-poison [@ getClass] with READ of size 8 through mozilla::dom::WebCryptoTask

Categories

(Core :: DOM: Security, defect, P1, critical)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 64+ fixed
firefox63 --- wontfix
firefox64 + fixed
firefox65 + fixed

People

(Reporter: decoder, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, sec-high, Whiteboard: [domsecurity-active][post-critsmash-triage][adv-main64+][adv-esr60.4+])

Attachments

(2 files)

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 64.0a1-20181020102231-https://hg.mozilla.org/mozilla-central/rev/f88ebf2720c875520d2de1cf4104a3b550c73ad8.

For detailed crash information, see attachment.
Flags: sec-bounty?
Not sure what is going on here exactly, but the use-after-poison suggests that maybe the underlying data buffer has been GCed. As far as I remember, the JS team recently made changes to poison GCed memory to detect more use-after-frees, so this could be such a case.

Martin, can you investigate this trace and check if you see anything that could cause us to use-after-gc here?
Flags: needinfo?(martin.thomson)
This definitely looks like the C++ code is being passed a freed ArrayBufferView.  There is nothing in the path in to the code that might release the argument - it is all correctly treated as being immutable.  This just happens to be the first place that the argument is read.  The JS side of this is a black box to me; if there is a problem I can't see one on the WebCrypto side of things.
Flags: needinfo?(martin.thomson)
Group: core-security → dom-core-security
Component: DOM → DOM: Security
Keywords: sec-high
sec-critical web-crypto bug - Dana, JC, can either of you take a look please?
Flags: needinfo?(jjones)
Flags: needinfo?(dkeeler)
I agree with Martin here. 

That said, this looks an awful lot like Bug 1499020, including the crash happening in AES-GCM (since we're assigning AAD on WebCryptoTask.cpp:581). In that, it looks like JS is GC'ing the AAD buffer before we use it. Which... yeah. 

We probably need someone from the JS team to help us out here.
Flags: needinfo?(jjones)
(In reply to J.C. Jones [:jcj] (he/him) from comment #6)
> We probably need someone from the JS team to help us out here.

Jason, can you help connect us to the right people? Probably we do indeed need some help from the JS folks.
Flags: needinfo?(jorendorff)
Flags: needinfo?(jorendorff)
Flags: needinfo?(jcoppeard)
See Also: → 1501647, 1499020
I'm suspicious of AesTask::Init() which seems to have unrooted GC pointers on the stack in the form of Aes*Params objects.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attached patch aes-task-initSplinter Review
I put a call to JS_GC() at WebCryptoTask.cpp line 581 and when I ran the WebCrypto tests the browser crashed with this exact stack.

The patch roots the parameter objects in AesTask::Init().  This fixed the crash with the additional GC call.
Attachment #9020063 - Flags: review?(rlb)
Comment on attachment 9020063 [details] [diff] [review]
aes-task-init

Redirecting r? to jcj
Attachment #9020063 - Flags: review?(rlb) → review?(jjones)
Comment on attachment 9020063 [details] [diff] [review]
aes-task-init

redirecting from Richard to Keeler, though this LGTM but it's late and I'm exhausted.
Attachment #9020063 - Flags: review?(jjones) → review?(dkeeler)
Comment on attachment 9020063 [details] [diff] [review]
aes-task-init

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

Thanks! Do we also need to do this for e.g. instances of JsonWebKey? ( https://searchfox.org/mozilla-central/search?q=JsonWebKey%5Cs&case=true&regexp=true&path=dom%2Fcrypto%2F ) or various KeyAlgorithms? ( https://searchfox.org/mozilla-central/search?q=KeyAlgorithm%5Cs&case=true&regexp=true&path=dom%2Fcrypto%2F ) (basically there are other dictionary types we're using defined in SubtleCrypto.webidl that I'm concerned about having the same problem...)
Attachment #9020063 - Flags: review?(dkeeler) → review+
Flags: needinfo?(dkeeler)
The hazard analysis is missing this. Filed bug 1502132.
I'm presuming this will be needed in ESR60.
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]
(In reply to Dana Keeler [:keeler] (she/her) (use needinfo) from comment #12)
> Thanks! Do we also need to do this for e.g. instances of JsonWebKey?

It's necessary for anything that can contain a JS GC thing (e.g. a JSObject).  In this case it's the OwningArrayBufferViewOrArrayBuffer that is the problem.  I had a quick look at those other two classes and they only seem to contain nsStrings, which is fine.
Comment on attachment 9020063 [details] [diff] [review]
aes-task-init

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: I think it would be moderately easy to trigger a crash based on this but not trivial to exploit it.

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

Which older supported branches are affected by this flaw?: Back to FF 32

If not all supported branches, which bug introduced the flaw?: Bug 998802

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: Same fix should apply.

How likely is this patch to cause regressions; how much testing does it need?: Unlikely.
Attachment #9020063 - Flags: sec-approval?
sec-approval+ for checkin on November 6, two weeks into the new cycle.
Whiteboard: [domsecurity-active] → [domsecurity-active][checkin on 11/6]
Attachment #9020063 - Flags: sec-approval? → sec-approval+
Blocks: 1499020
Whiteboard: [domsecurity-active][checkin on 11/6] → [domsecurity-active]
https://hg.mozilla.org/mozilla-central/rev/e721c3e314bd
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Please request Beta and ESR60 approval on this when you get a chance. It grafts cleanly to both as-landed.
Flags: needinfo?(jcoppeard)
Comment on attachment 9020063 [details] [diff] [review]
aes-task-init

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 998802

User impact if declined: Possible crash / security vulnerability.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a very simple fix to root data structures on the stack.

String changes made/needed: None.

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bug.

User impact if declined: Possible crash / security vulnerability.

Fix Landed on Version: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a very simple fix to root data structures on the stack.

String or UUID changes made by this patch: None.
Flags: needinfo?(jcoppeard)
Attachment #9020063 - Flags: approval-mozilla-esr60?
Attachment #9020063 - Flags: approval-mozilla-beta?
Comment on attachment 9020063 [details] [diff] [review]
aes-task-init

Fix for sec-high issue, has test coverage; let's take this for beta 64 and esr60.
Attachment #9020063 - Flags: approval-mozilla-esr60?
Attachment #9020063 - Flags: approval-mozilla-esr60+
Attachment #9020063 - Flags: approval-mozilla-beta?
Attachment #9020063 - Flags: approval-mozilla-beta+
Flags: sec-bounty? → sec-bounty+
Flags: qe-verify-
Whiteboard: [domsecurity-active] → [domsecurity-active][post-critsmash-triage]
Whiteboard: [domsecurity-active][post-critsmash-triage] → [domsecurity-active][post-critsmash-triage][adv-main64+][adv-esr60.4+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.