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

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 3

[Christian Holler (:decoder)]

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?

Comment 4

[Martin Thomson [:mt:]]

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.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 6

[J.C. Jones [:jcj] (he/they)]

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.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 8

[Jon Coppeard (:jonco)]

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

Comment 9

[Jon Coppeard (:jonco)]

Attachment: aes-task-init

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.

Comment 10

[Richard Barnes [:rbarnes]]

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

Redirecting r? to jcj

Comment 11

[J.C. Jones [:jcj] (he/they)]

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.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

Comment 13

[Steve Fink [:sfink] [:s:]]

The hazard analysis is missing this. Filed bug 1502132.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

I'm presuming this will be needed in ESR60.

Comment 15

[Jon Coppeard (:jonco)]

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

[Jon Coppeard (:jonco)]

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.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

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

Comment 18

[Jon Coppeard (:jonco)]

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

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Jon Coppeard (:jonco)]

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.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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