1385112 - Assertion failure: length != 0, at js/src/frontend/TokenStream.cpp:59

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

found this while investigating bug 1383951 (where only the number of repeat is different),
not yet sure if that's the same issue tho.

revision:
  m-i 433540884241

code:
  eval(`let \\u{${"0".repeat(0x8000)}65} = 123;`);

configure flag:
  --enable-warnings-as-errors --disable-optimize

command line option:
  <nothing>

Comment 1

[Tooru Fujisawa [:arai]]

looks like, the value of userbuf.addressOfNextRawChar() is not expected.
>       case '\\': {
>         uint32_t escapeLength = matchUnicodeEscapeIdStart(&qc);
>         if (escapeLength > 0) {
>             identStart = userbuf.addressOfNextRawChar() - escapeLength - 1;
>             hadUnicodeEscape = true;
>             goto identifier;
>         }
>         goto badchar;
>       }

it points `userbuf.base_ + 10`, and escapeLength is 32773, so identStart points an address before userbuf.base_.

anyway, this is from bug 1314037.
(not adding to blocking bugs to avoid hint for attackers)

Comment 2

[Tooru Fujisawa [:arai]]

> uint32_t
> TokenStream::matchUnicodeEscapeIdStart(uint32_t* codePoint)
> {
>     uint32_t length = peekUnicodeEscape(codePoint);
>     if (length > 0 && unicode::IsIdentifierStart(*codePoint)) {
>         skipChars(length);
>         return length;
>     }
>     return 0;
> }

> void skipChars(uint8_t n) {

oh...
it's an overflow...

Comment 3

[Tooru Fujisawa [:arai]]

I could create a patch by tomorrow.
if anyone are willing to, feel free to take over.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Make TokenStream::skipChars a template method.

So, in TokenStream::matchUnicodeEscapeIdStart and TokenStream::matchUnicodeEscapeIdent the argument of TokenStream::skipChars is uint32_t.
We could make TokenStream::skipChars either to take uint32_t or an template (if uint8_t is preferred for any reason).

This patch makes it template.
anba, how do you think about this change?


(btw, I've tried reproducing bug 1383951, but cannot reproduce the exact same crash.  it crashes with null-deref in other place, that gets fixed by this patch, and it runs without crash)

Comment 5

[André Bargull [:anba]]

Comment on attachment 8891167 [details] [diff] [review]
Make TokenStream::skipChars a template method.

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

(In reply to Tooru Fujisawa [:arai] from comment #4)
> So, in TokenStream::matchUnicodeEscapeIdStart and
> TokenStream::matchUnicodeEscapeIdent the argument of TokenStream::skipChars
> is uint32_t.
> We could make TokenStream::skipChars either to take uint32_t or an template
> (if uint8_t is preferred for any reason).
> 
> This patch makes it template.
> anba, how do you think about this change?

I think I'd go with uint32_t, so we don't have potentially up to three different instantiations of this method (uint32_t when called from matchUnicodeEscape..., uint8_t when called from getDirective, and int32_t when called with an integer literal). (Except I hope skipChars gets inlined when called from getStringOrTemplateToken. :-)

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Use uint32_t in TokenStream::skipChars.

okay, here's the patch that changes the signature to uint32_t

I'll post testcase as different patch, that should be landed later, since the testcase clearly describes how to trigger the issue.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add testcase.

here's the testcase from comment #0.

Comment 8

[André Bargull [:anba]]

Comment on attachment 8891360 [details] [diff] [review]
Use uint32_t in TokenStream::skipChars.

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

Great. Thank you!

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8891360 [details] [diff] [review]
Use uint32_t in TokenStream::skipChars.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

it should be easy to create a case that causes assertion failure on debug build.
not sure about more critical exploit.
at least I haven't yet figured out how to cause crash or other thing on release build.

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

Testcase describes how to cause the assertion failure, so it should be landed later.
the commit message and the change will also give some hint about overflow.

> Which older supported branches are affected by this flaw?

52

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

bug 1314037

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

not yet, but it should be easy.
hopefully the same patch should be applicable.

> How likely is this patch to cause regressions; how much testing does it need?

less likely to regress semantics/behavior,
but there might be some chance of performance regression.

Comment 10

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #9)
> > If not all supported branches, which bug introduced the flaw?
> 
> bug 1314037

Actually it's bug 1326454 (it changed the parameter type from |int| to |uint8_t|: http://searchfox.org/mozilla-central/diff/306c6680e35acd2fa4b488794fe761195f941b5a/js/src/frontend/TokenStream.h#999). So Firefox52 (ESR) isn't affected by this bug (https://hg.mozilla.org/releases/mozilla-esr52/file/tip/js/src/frontend/TokenStream.h#l983).

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Any idea on a sec rating for this?

Comment 12

[Tooru Fujisawa [:arai]]

it basically reads string from semi-random address, and in very limited case it might be able to get security-sensitive information as error message or something, if the information matches to JavaScript identifier syntax.
(in the comment #0 case locally, the memory doesn't match any identifier syntax and it results in 0-length identifier, and it results in the assertion failure)

so sec-moderate or sec-low.

Comment 13

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #12)
> it basically reads string from semi-random address, and in very limited case
> it might be able to get security-sensitive information as error message or
> something, if the information matches to JavaScript identifier syntax.

For example this test case prints "\u9098\uCE08\u9098\uD50C\u1990\uFF96 is not defined" for me:
---
"0123456789".repeat(2**10);
try {
eval(`
  {
  \\u{${"0".repeat(512)}65};
`);
} catch(e) {
  print(e.message);
}
---

Comment 14

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

This is a sec-low. It can go onto trunk but I'm clearing sec-approval.

Comment 15

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dea5dba2f49cec533c8e943c9d5569bda1912f2
Bug 1385112 - Use uint32_t in TokenStream::skipChars. r=anba

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5dea5dba2f49

Comment 17

[Tooru Fujisawa [:arai]]

Comment on attachment 8891360 [details] [diff] [review]
Use uint32_t in TokenStream::skipChars.

Approval Request Comment
> [Feature/Bug causing the regression]:

bug 1326454

> [User impact if declined]:

possible disclosure of security sensitive information

> [Is this code covered by automated tests?]:

the testcase is not yet landed
(I'll land it after fixed version is released)

> [Has the fix been verified in Nightly?]:

yes

> [Needs manual test from QE? If yes, steps to reproduce]:

no

> [List of other uplifts needed for the feature/fix]:

none

> [Is the change risky?]:

no

> [Why is the change risky/not risky?]:

this changes the signature of function from uint8_t to uint32_t,
so it shouldn't cause any regression, except performance regression.

> [String changes made/needed]:

none

Comment 18

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

Comment on attachment 8891360 [details] [diff] [review]
Use uint32_t in TokenStream::skipChars.

Let's uplift this now for the 56 beta 1 build.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c743648bf55b

Comment 20

[Tooru Fujisawa [:arai]]

ni? myself to land testcase after next beta (btw, when is it?)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Technically I don't think we can land the test until after Fx56 ships in late September since Fx55 was affected by this bug too.

Comment 22

[Tooru Fujisawa [:arai]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)
> Technically I don't think we can land the test until after Fx56 ships in
> late September since Fx55 was affected by this bug too.

ah, indeed.
clearing ni? for now.

Comment 23

[Tooru Fujisawa [:arai]]

we can now land testcase

Comment 24

[Tooru Fujisawa [:arai]]

Comment on attachment 8891361 [details] [diff] [review]
Part 2: Add testcase.

sec-approval? for part 2, testcase.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Very easy.

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

Yes.

> Which older supported branches are affected by this flaw?

None.
All branches are fixed.

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

-

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

this is test only.

> How likely is this patch to cause regressions; how much testing does it
> need?

Not likely.
this is test.

Comment 25

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

Comment on attachment 8891361 [details] [diff] [review]
Part 2: Add testcase.

sec-approval+ to check in the tests now.

Comment 26

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdfd46134a2406c196edd3341eaa71fbffab487c
Bug 1385112 - Part 2: Add testcase. r=anba

Comment 27

[Tooru Fujisawa [:arai]]

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