Bug 87980 – Fix access beyond array end when parsing the empty string

• Component: JavaScriptCore
• Status: NEW

[Andy Wingo]

The patch to be attached fixes an OOB array access in the lexer.

[Andy Wingo]

Created attachment 145095 [details]
Patch

[Andy Wingo]

Adding more potential reviewers :)

[Geoffrey Garen]

Comment on attachment 145095 [details]
Patch

Our policy is to require a regression test for a code change that fixes an observable bug (http://www.webkit.org/coding/contributing.html). Can you make a regression test for this?

[José Dapena Paz]

With this patch, I'm getting the assert

==31670== Invalid write of size 4
==31670==    at 0xB4E5578: JSC::Identifier const& JSC::IdentifierArena::makeIdentifier<unsigned short>(JSC::JSGlobalData*, unsigned short const*, unsigned long) (ParserArena.h:74)
==31670==    by 0xB4E2FAA: JSC::Lexer<unsigned short>::makeIdentifier(unsigned short const*, unsigned long) (Lexer.h:236)
==31670==    by 0xB4E217A: JSC::Lexer<unsigned short>::scanRegExp(JSC::Identifier const*&, JSC::Identifier const*&, unsigned short) (Lexer.cpp:1623)
==31670==    by 0xB531EC0: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parsePrimaryExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1456)
==31670==    by 0xB52F5B6: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parseMemberExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1519)
==31670==    by 0xB52DE05: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parseUnaryExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1608)
==31670==    by 0xB52C0DC: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parseBinaryExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1150)
==31670==    by 0xB52A20E: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parseConditionalExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1112)
==31670==    by 0xB527DCE: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parseAssignmentExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1054)
==31670==    by 0xB524B34: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parseExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1028)

Seems similar changes are needed in other usages of makeIdentifier.

[Andy Wingo]

I appear to have borked the patch :)  It was a valgrind run from José that caught the bug.

Geoffrey; are you asking me to make an exploit for this bug?  It seems tricky: the extra byte would have to reside on a page that is not mapped into memory, and it would just yield a segfault.  There is of course the possibility of poisoning the recent identifiers cache, but that doesn't seem to be exploitable, merely an efficiency loss.  I'll submit an updated patch, but can you please clarify that you think a regression test is necessary in this case.

[Geoffrey Garen]

> Geoffrey; are you asking me to make an exploit for this bug?

I'm not sure -- maybe you can explain what the symptom is that you're trying to fix, and what conditions might cause it, and then we can talk about the best way to add a test.

If this is a read past the end of a malloc buffer, one option is to create a test that crashes if and only if you are running under GuardMalloc.

[Andy Wingo]

(In reply to comment #6)
> > Geoffrey; are you asking me to make an exploit for this bug?
> 
> I'm not sure -- maybe you can explain what the symptom is that you're trying to fix, and what conditions might cause it, and then we can talk about the best way to add a test.

The story is that José was doing a valgrind run and noticed this read beyond the end of the array.  It seems to be worth fixing, as it could cause a segfault at some point.  Admittedly, we have not seen a crash due to this.

Though, hummm: what if the OOB byte is greater than 127.  In that case we would be writing beyond the bounds of m_recentIdentifiers.

> If this is a read past the end of a malloc buffer, one option is to create a test that crashes if and only if you are running under GuardMalloc.

I will look into this, thanks.

[Darin Adler]

(In reply to comment #7)
> (In reply to comment #6)
> > If this is a read past the end of a malloc buffer, one option is to create a test that crashes if and only if you are running under GuardMalloc.
> 
> I will look into this, thanks.

Another good option is to add an assertion that we aren’t reading past the end of the buffer, then write a test that hits the assertion, then fix the bug.

[Andy Wingo]

Created attachment 145582 [details]
Patch

[Andy Wingo]

The patch I pushed does include tests for the codepaths that hit this assertion.  Without the fix they do indeed abort.

[Andy Wingo]

Comment on attachment 145582 [details]
Patch

Clearing review flag, as I am seeing some test suite failures.

[Andy Wingo]

Created attachment 145596 [details]
Patch

[Andy Wingo]

Attachment 145582 [details] had a bug.  Sorry for the noise.  I fixed it and ran all the layout tests on the one I just uploaded, attachment 145596 [details].

[Geoffrey Garen]

Comment on attachment 145596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145596&action=review

> Source/JavaScriptCore/parser/Lexer.cpp:605
> +    if (!length)
> +        return;

This function and all its callees seem safe vs zero length. What happens if you remove this check?

If something bad happens, why doesn't append16 need this check?

> Source/JavaScriptCore/parser/ParserArena.h:74
> +        ASSERT(length);

I'm not a fan of the design pattern that has a callee ASSERT a condition and then requires all callers to ensure the condition with extra lines of code. This is only a good tradeoff if it's critical to performance in some way. But even then, I prefer to have a "fast path" function that ASSERTs the condition and a "slow path" convenience function that ensures the condition. That way, there's only one place in the code that needs to get this right, instead of many.

I'd suggest the following design change:
(1) Remove all instances of "stringStart != currentCharacter() && ", "currentCharacter() != stringStart &&", "identifierStart != currentCharacter()", etc.
(2) Add a test for zero length inside Lexer::makeIdentifier and Lexer::makeIdentifierLCharFromUChar, which returns the empty identifier.
(3) Put an ASSERT(length) inside IdentifierArena::makeIdentifier and IdentifierArena::makeIdentifierLCharFromUChar.

To test the *16 conditions, you need a separate .js file encoded as UTF-16.