1415443 - Crash in js::frontend::IsIdentifier

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect, P3)

Description

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

This bug was filed from the Socorro interface and is 
report bp-a640f79f-a61e-439e-9271-1bf110171102.
=============================================================

UAF going back to at least 52ESR and likely further.  Crashes in this function go back to at least FF22.

Comment 1

[Jason Orendorff [:jorendorff]]

Yoric, would you please take a look? The stacks for this crash signature are very consistent.

If nothing else, ExpressionDecompiler::loadAtom should delegate directly to a JSScript method that has the same signature, and asserts that the pc is in range for the script and that the opcode actually has the JOF_ATOM flag. The more assertions, the better the fuzzers work, and a fuzzbug is a reproducible bug...

Comment 2

[David Teller [:Yoric] - still alive but not very active]

At a quick glance:

- looks OS-independent (so pretty certainly our fault);
- all the stacks I have looked at but one look like

  js::frontend::IsIdentifier(JSLinearString*) 	js/src/vm/Unicode.h:155
  (anonymous namespace)::ExpressionDecompiler::decompilePC(unsigned char*, unsigned char) 	js/src/jsopcode.cpp:1762
  js::DecompileValueGenerator(JSContext*, int, JS::Handle<JS::Value>, JS::Handle<JSString*>, int)
  ...

I'll add a few assertions and see if that helps.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Can't find any obvious reason for the crash, so I'll adding a few asserts as suggested by jorendorff. If you can think of more stuff to assert, I'll be happy to add some more.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Adding a few assertions around ExpressionDecompiler::decompilePC

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8929430 [details] [diff] [review]
Adding a few assertions around ExpressionDecompiler::decompilePC

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

Very nice, thanks.

::: js/src/frontend/TokenStream.cpp
@@ +179,5 @@
> +    MOZ_ASSERT(str);
> +    if (str->hasLatin1Chars())
> +        return ::IsIdentifier(str->latin1Chars(nogc), str->length());
> +    else
> +        return ::IsIdentifierMaybeNonBMP(str->twoByteChars(nogc), str->length());

Style nit: We have a hard rule against else-after-return. I'm not crazy about it, but it's one of the points where we actually agree with Gecko, so...

Comment 6

[Jason Orendorff [:jorendorff]]

Christian, if we can tell you which existing tests exercise the expression decompiler, could we get some focused fuzz-testing on this code?

Comment 7

[Christian Holler (:decoder)]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> Christian, if we can tell you which existing tests exercise the expression
> decompiler, could we get some focused fuzz-testing on this code?

Yes, if you show me code examples that hit this code, I can try to add some focused code to the fuzzing driver.

Comment 8

[Jason Orendorff [:jorendorff]]

David, would you please find out which tests use the expression decompiler?

(I would probably do this by adding a MOZ_CRASH() in DecompileValueGenerator or decompilePC and running the tests and jit-tests. Tests that use the decompiler will fail.)

No guarantee the fuzzers will find this. Sure would be great if they did, though.

Comment 9

[Jason Orendorff [:jorendorff]]

Note for Christian: The decompiler is invoked only when reporting an error; for example, "foo.toString is not a function". So most tests avoid it. And, very normal-looking code can trigger it.

Comment 10

[Christian Holler (:decoder)]

Here is a recent fuzzing coverage report for js/src/jsopcode.cpp (the file in question):

https://fuzzmanager.fuzzing.mozilla.org/covmanager/collections/833/browse/#p=js/src/jsopcode.cpp&s=1667

It seems to me that the decompiler is pretty well covered in general. If anyone wants to look through it to find interesting spots/holes and needs access for that, please ping me.

Comment 11

[Christian Holler (:decoder)]

When trying to build with the attached patch applied to mozilla-central, I got this error:

> js/src/jsscript.h:1874:40: error: ‘JOF_OPTYPE’ was not declared in this scope
>         MOZ_ASSERT(JOF_OPTYPE((JSOp)*pc) == JOF_ATOM);

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Mmmh... Might be a case of unified builds? I'll check this out tomorrow.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: Adding a few assertions around ExpressionDecompiler::decompilePC

Same one, minus typo, and plus requested layout fix.

Do I need sec-approval for this?

Also, currently attempting to list the tests requested in comment 8. Given how long our test suites last, I guess I'll have the list tomorrow.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: List of tests involving ExpressionDecompiler::decompilePC

Followed jorendorff's idea in comment 8, here is a list of tests involving ExpressionDecompiler::decompilePC. This does not include the tests that failed by timeout, which may or may not be related to decompilePC.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

May I checkin-needed or do I need sec-approval for this?

Comment 16

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

al: comment 15...

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8930976 [details] [diff] [review]
Adding a few assertions around ExpressionDecompiler::decompilePC

[Security approval request comment]
This patch is just adding assertions to help us pinpoint where this crash comes from and the impact on security.

Comment 18

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

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #15)
> May I checkin-needed or do I need sec-approval for this?

Hard to know since no one has answered sec-approval template questions. It is a sec-high. If it affects more than trunk, it needs sec-approval. That's now been asked for in a later comment but I need the template questions answered.

Comment 19

[Christian Holler (:decoder)]

I'm back from PTO now and will look into fuzzing this patch before it lands. Once it is clean on fuzzing, we should be able to land it. But if it is not, then it has potential to reveal a sec bug to others, so we should at least wait until that. I don't think this is a sec-approval case because this isn't a security fix we're doing here, we don't even know what the bug is, yet.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Ok, so not landing.

Comment 21

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

So can you clear the sec-approval and request again when you want to land this?

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8930976 [details] [diff] [review]
Adding a few assertions around ExpressionDecompiler::decompilePC

Done

Comment 23

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

Doesn't Christian need to fuzz this first?

Comment 24

[Christian Holler (:decoder)]

(In reply to Al Billings [:abillings] from comment #23)
> Doesn't Christian need to fuzz this first?

I have been fuzzing this for 24 hours now but not found anything. Will keep going, but I assume we can land this already.

Comment 25

[Frederik Braun [:freddy]]

What are the next steps here? It seems we could get this shipped, no?

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Let's do that.

Comment 27

[Frederik Braun [:freddy]]

Yoric, comment #21 tells me you still require sec-approval.
Can you please request sec-approval for the attachment and answer all the questions in the template?
This is required before checkin.

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8930976 [details] [diff] [review]
Adding a few assertions around ExpressionDecompiler::decompilePC

[Security approval request comment]
This is just a diagnosis patch, which we hope to use to help fuzz out the actual issue.

Comment 29

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

Comment on attachment 8930976 [details] [diff] [review]
Adding a few assertions around ExpressionDecompiler::decompilePC

sec-approval+

Comment 30

[David Teller [:Yoric] - still alive but not very active]

I can't seem to be able to mark this one as checkin-needed, probably because I'm not in security group.

Comment 31

[Andrew McCreight [:mccr8]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #30)
> I can't seem to be able to mark this one as checkin-needed, probably because
> I'm not in security group.

There is no such restriction. I'm not sure why you weren't able to.

Comment 32

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

diagnostic asserts: https://hg.mozilla.org/integration/mozilla-inbound/rev/35b81e0007ab42a5c0e7ec037ee2295d6708607e

Comment 33

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

diagnostic asserts: https://hg.mozilla.org/mozilla-central/rev/35b81e0007ab

Comment 34

[Frederik Braun [:freddy]]

Any news since we landed the diagnostic assert?

Comment 35

[David Teller [:Yoric] - still alive but not very active]

For the moment, if I read Socorro correctly, we have exactly one re;evant signature from a build that contains this patch: https://crash-stats.mozilla.com/report/index/f72cff56-3d56-4607-98e7-1a4870180317 – and that signature neatly sidesteps all the assertions.

For the moment, I have no clue where the crash could be coming from. If anybody has ideas, I'm interested.

Comment 36

[David Teller [:Yoric] - still alive but not very active]

A few more crashes have shown up with the relevant signature, and they all show that `str` is non-null.

Apparently, all the crashes on recent builds (60a1+) are under Windows.

Crash addresses:
- 0xffffffffffffffff (most)
- 0x6b465346 (a few)
- 0xc5212076 (one)
- 0xa5 (one, what is that?)
- 0x226 (one, what is that?)

So, my best guess is that we're overwriting that memory. No clue where, though.

Comment 37

[Jason Orendorff [:jorendorff]]

This is looking unactionable now.

Comment 38

[Ted Campbell [:tcampbell]]

Stalled. We did some digging, but it is currently unactionable.

Comment 39

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 40

[Andrew McCreight [:mccr8]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #39)

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

This is still a stalled sec bug. The leave-open was because it had a diagnostic patch landed.

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 42

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit auto_nag documentation.