1478573 - AddressSanitizer: heap-use-after-free [@ IsArray] with READ of size 4 with IndexedDB

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 63.0a1-20180726001822-https://hg.mozilla.org/mozilla-central/rev/7ba07ef0e4532b644b812942aa38af4510dbc74f (originally reported by %client%).

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Daniel Veditz [:dveditz]]

Jan: can you take a look please.

Comment 3

[Jan Varga [:janv]]

Ok, this looks like bug 1459383, we thought it was fixed by bug 1404274.
I'll investigate it more.

Comment 4

[Jan Varga [:janv]]

Ok, it took me a while, but I finally know what's going on here.

Comment 5

[Jan Varga [:janv]]

Attachment: patch

I think we should add tests for this, will do in a new bug if you don't mind.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8995366 [details] [diff] [review]
patch

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

Love the comment!

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

And yes, agreed new bug for the tests.

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Triggering this condition requires a length getter that's overridden -- and changes. Is that right?

I don't write much Javascript these days, but is there a use case for that in every day programming? Is there any chance we captured a real exploit? (Normally I'm not inclined to such hypothesis, but it seems like a strange code pattern to occur non-maliciously).

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

The idiom is either malicious or bug-hunting/fuzzing.  A getter that deletes an index makes no sense, especially since it only works inside a versionchange transaction.

Given that we thought we fixed bug 1459383, but didn't quite, and the ASAN build is explicitly to report such things, I'd expect the bug reporter there or someone testing the repro on that bug and triggering the test.  I know I've manually triggered the test accidentally, but it wasn't in my ASAN profile.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #8)
> Triggering this condition requires a length getter that's overridden -- and
> changes. Is that right?

Not a length getter.  Any getter.

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Thanks for confirming (and correcting!) Using ASAN-reporter nightly for vuln researcher definitely seems like something someone might do, though I'm a bit uncomfortable assuming that without knowing who the reporter was (I'm assuming "%client%" means we didn't have an email for them).

Dan, do you have an opinion here? Is my spidey-sense overly sensitive?

Comment 12

[Christian Holler (:decoder)]

Note the "file:// Content" here:

> READ of size 4 at 0x60700012f3b0 thread T0 (file:// Content)

So someone was running this on a local file. But why would an exploit researcher use a reporter build?

Comment 13

[Alex Gaynor [:Alex_Gaynor]]

Good catch, I totally missed that. Thanks Christian.

I assume because it was a convenient way to get an ASAN. Maybe they were just testing a PoC out locally without thinking about it.

Comment 14

[Jan Varga [:janv]]

Comment on attachment 8995366 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch fixes an additional problem/regression that bug 1404274 introduced, it landed only several days ago on m-c (only). However the original security problem in bug 1459383 existed for long time and the fix for it landed as a normal bug fix in 1404274. So we fixed the security problem by landing a correctness fix that makes some web platform tests pass. This makes it much less obvious that there was a security problem.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Tests will be added separately, but the comment in the patch can point to a possible security problem. However, the cloning landed just recently on m-c and we are fixing this additional problem very quickly.

Which older supported branches are affected by this flaw?
None

If not all supported branches, which bug introduced the flaw?
bug 1404274

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
There are no affected branches.

How likely is this patch to cause regressions; how much testing does it need?
It's a simple and safe patch, the patch was tested locally.

Comment 15

[Jan Varga [:janv]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #8)
> Triggering this condition requires a length getter that's overridden -- and
> changes. Is that right?

See the first attachment in bug 1459383.

Comment 16

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

If this is nightly only, it doesn't require sec-approval. See https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 17

[Jan Varga [:janv]]

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

Comment 18

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

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

Comment 19

[Liz Henry (:lizzard)]

Jan, since we uplifted the patch from bug 1404274 to beta, should we also uplift this?

Comment 20

[Jan Varga [:janv]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #19)
> Jan, since we uplifted the patch from bug 1404274 to beta, should we also
> uplift this?

No, we provided a special uplift patch in bug 1404274 which contains changes from this bug.

Comment 21

[Liz Henry (:lizzard)]

OK. Thanks!

Comment 22

[Christian Holler (:decoder)]

When you find some time, it would be very helpful if you could answer the following questions so I can work on improving ASan Nightly further. In particular, many of the reports we saw about use-after-free were sent only once, not multiple times. So I'm asking myself what changes to ASan Nightly I can make to improve overall uaf detection rate:

1) Do you think more frequent GC/CC would have helped detecting this problem more frequently?

2) Do you think GC/CC on specific DOM events (e.g. beforeunload, pagehide, DOMContentLoaded) would have helped?

3) Do you think triggering the event loop in the specific child processes would have helped here? (e.g. dummy XMLHttpRequest onbeforeunload, useful in fuzzing).

4) Do you have any other ideas to reproduce this particular problem better or do you think any of the above points (or other approaches) might help to find other use-after-free issues in areas that you work in?

Any feedback would be greatly appreciated. Thanks!

Comment 23

[Jan Varga [:janv]]

(In reply to Christian Holler (:decoder) from comment #22)

When you find some time, it would be very helpful if you could answer the
following questions so I can work on improving ASan Nightly further. In
particular, many of the reports we saw about use-after-free were sent only
once, not multiple times. So I'm asking myself what changes to ASan Nightly
I can make to improve overall uaf detection rate:

1. Do you think more frequent GC/CC would have helped detecting this problem
   more frequently?

Probably not in this case.

2. Do you think GC/CC on specific DOM events (e.g. beforeunload, pagehide,
   DOMContentLoaded) would have helped?

Probably not in this case.

3. Do you think triggering the event loop in the specific child processes
   would have helped here? (e.g. dummy XMLHttpRequest onbeforeunload, useful in
   fuzzing).

Probably not in this case.

4. Do you have any other ideas to reproduce this particular problem better
   or do you think any of the above points (or other approaches) might help to
   find other use-after-free issues in areas that you work in?

No.

Any feedback would be greatly appreciated. Thanks!

I think the attached crash information was enough in this case.