Bug 176114 – semicolon is being interpreted as an = in the LiteralParser

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Oliver Hunt]

The following test cases fail

x = 0;
x;5
assert(x != 5)

var y;5
assert(y!=5)

etc

[Saam Barati]

*** Bug 176115 has been marked as a duplicate of this bug. ***

[Saam Barati]

(In reply to Oliver Hunt from comment #0)
> The following test cases fail
> 
> x = 0;
> x;5
> assert(x != 5)
> 
> var y;5
> assert(y!=5)
> 
> etc
This is not right. You need code like this:

// test.js
x=undefined;
load("test2.js");
print(x); // prints 123
// test2.js
x;123


It appears to be related to the literal parser. If you run the above example with JSC_dumpGeneratedBytecodes=1, we don't even generate bytecode for the second program in test2.js

[Saam Barati]

I've verified commenting out the literal parser for programs inside Interpreter.cpp removes this bug.

[Oliver Hunt]

have there been any changes in that code recently? because this seems to be a regression from shipping. Very strange.

[Saam Barati]

I have a vague memory of Yusuke doing some work there. It's really bizarre to me that the literal parser is causing a variable assignment. Is that expected behavior?

I don't even really understand what the literal parser is for, I always though it was a way to quickly create objects that are in JSON format.

[Saam Barati]

Maybe it was this bug?
https://bugs.webkit.org/show_bug.cgi?id=156953

I believe this is the last change there that isn't related to basic refactoring.

[Oliver Hunt]

(In reply to Saam Barati from comment #5)
> I have a vague memory of Yusuke doing some work there. It's really bizarre
> to me that the literal parser is causing a variable assignment. Is that
> expected behavior?
> 
> I don't even really understand what the literal parser is for, I always
> though it was a way to quickly create objects that are in JSON format.

LiteralParser is used for JSON and for JSONP.

To do JSONP quickly we have magic logic to handle

(a(.b)*=json | a(.b)*\(json\))+

which we parse and call directly to avoid jumping through the interpreter. We need this because people still insist on sending megs of data as executable script. because reasons.

[Saam Barati]

It's not related to:
https://bugs.webkit.org/show_bug.cgi?id=156953

[Oliver Hunt]

(In reply to Saam Barati from comment #6)
> Maybe it was this bug?
> https://bugs.webkit.org/show_bug.cgi?id=156953
> 
> I believe this is the last change there that isn't related to basic
> refactoring.

I suspect change would only occur in the jsonp handling. Which used to be in interpreter.cpp but I don't have a webkit checkout handy.

[Saam Barati]

(In reply to Oliver Hunt from comment #4)
> have there been any changes in that code recently? because this seems to be
> a regression from shipping. Very strange.

I can reproduce it in a shipping macOS. So if it's a regression, it is from further back than the last macOS.

[Saam Barati]

Ima work on this.

[Saam Barati]

This code looks suspicious inside the lex() function:
        

        if (*m_ptr == ';') {
            token.type = TokSemi;
            token.end = ++m_ptr;
            return TokAssign;
        }

[Oliver Hunt]

(In reply to Saam Barati from comment #12)
> This code looks suspicious inside the lex() function:
>         
> 
>         if (*m_ptr == ';') {
>             token.type = TokSemi;
>             token.end = ++m_ptr;
>             return TokAssign;
>         }

that does indeed seem questionable. I'm so sad I wrote it :(

[Saam Barati]

Patch forcoming. I'm adding an assertion to next() for this.

[Saam Barati]

Created attachment 319392 [details]
patch

[WebKit Commit Bot]

Comment on attachment 319392 [details]
patch

Clearing flags on attachment: 319392

Committed r221400: <http://trac.webkit.org/changeset/221400>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34173912>

[LiJunPan(evil7)]

It was a nice XSS victor for me. And it's a just little mistake not even a bug.
So, thx for Ur mistake & don't write more like "OMG I forget change the return when I paste codes".
Have nice day! XD

[Oliver Hunt]

(In reply to LiJunPan(evil7) from comment #19)
> It was a nice XSS victor for me. And it's a just little mistake not even a
> bug.
> So, thx for Ur mistake & don't write more like "OMG I forget change the
> return when I paste codes".
> Have nice day! XD

The XSS vector is mercifully limited as it requires the right hand side of the assignment to be side effect free (it's not arbitrary expressions, at least it shouldn't be), basically the right hand side has to be a slightly more accepting version of JSON: "s and 's are are allowed for string literals, and a few other minor differences in that vein.

[LiJunPan(evil7)]

(In reply to Oliver Hunt from comment #20)
> (In reply to LiJunPan(evil7) from comment #19)
> > It was a nice XSS victor for me. And it's a just little mistake not even a
> > bug.
> > So, thx for Ur mistake & don't write more like "OMG I forget change the
> > return when I paste codes".
> > Have nice day! XD
> 
> The XSS vector is mercifully limited as it requires the right hand side of
> the assignment to be side effect free (it's not arbitrary expressions, at
> least it shouldn't be), basically the right hand side has to be a slightly
> more accepting version of JSON: "s and 's are are allowed for string
> literals, and a few other minor differences in that vein.

Yep.I just found it from a CTF-game at last week. That's a strangely POC but OMG it's worked. SO I tryed to input some others like "foo;test;" or "var test;'string'" wanna make it working but NOT，It ONLY worked with "location;'(String:)str'" and ONLY at macOS. So I realized it's hardly to make a XSS in web framework. Just maybe a little mistake in the sourses of Webkit.