Christian Holler (:decoder) Reporter
	Description • 6 years ago

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.

	Christian Holler (:decoder) Reporter
	Comment 3 • 6 years ago

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?

	Martin Thomson [:mt:]
	Comment 4 • 6 years ago

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.

	Christoph Kerschbaumer [:ckerschb]
	Comment 5 • 6 years ago

sec-critical web-crypto bug - Dana, JC, can either of you take a look please?

	J.C. Jones [:jcj] (he/him)
	Comment 6 • 6 years ago

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.

	Christoph Kerschbaumer [:ckerschb]
	Comment 7 • 6 years ago

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

	Jon Coppeard (:jonco) Assignee
	Comment 8 • 6 years ago

I'm suspicious of AesTask::Init() which seems to have unrooted GC pointers on the stack in the form of Aes*Params objects.

	Jon Coppeard (:jonco) Assignee
	Comment 9 • 6 years ago

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.

	Richard Barnes [:rbarnes]
	Comment 10 • 6 years ago

Comment on attachment 9020063 [details] [diff] [review]
aes-task-init

Redirecting r? to jcj

	J.C. Jones [:jcj] (he/him)
	Comment 11 • 6 years ago

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.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 12 • 6 years ago

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

	Steve Fink [:sfink] [:s:]
	Comment 13 • 6 years ago

The hazard analysis is missing this. Filed bug 1502132.

	Randell Jesup [:jesup] (needinfo me)
	Comment 14 • 6 years ago

I'm presuming this will be needed in ESR60.

	Jon Coppeard (:jonco) Assignee
	Comment 15 • 6 years ago

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

	Jon Coppeard (:jonco) Assignee
	Comment 16 • 6 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 17 • 6 years ago

sec-approval+ for checkin on November 6, two weeks into the new cycle.

	Jon Coppeard (:jonco) Assignee
	Comment 18 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e721c3e314bda10fe918e919d6ca9af8ad1a56ca

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 19 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/e721c3e314bd

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 6 years ago

Please request Beta and ESR60 approval on this when you get a chance. It grafts cleanly to both as-landed.

	Jon Coppeard (:jonco) Assignee
	Comment 21 • 6 years ago

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.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 22 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/3cbc2340b051
https://hg.mozilla.org/releases/mozilla-esr60/rev/5e1a9644aeef