430740 - BOM characters are stripped from javascript before execution

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dave Reed]

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 6.0; SLCC1; .NET CLR 2.0.50727; InfoPath.2; .NET CLR 3.0.04506; .NET CLR 3.5.30319; WWTClient2)
Build Identifier: 2.0.0.13

Using control format unicode characters, it is possible for code that will be executed to exist inside what would otherwise appear to be a quoted javascript string. The same technique can be used to escape out of block comments.



Reproducible: Always

Steps to Reproduce:
Execute the following javascript:

function evil() {
   alert('evil');
   return 'evil';
}
alert(eval("(['a\\\u200d', '+evil()])//')]"));
Actual Results:  
alert 'evil' appears, then an aldert "a', evil"

Expected Results:  
only one alert "a\,+evil()])//" should appear.

See url for more.

Comment 1

[Brendan Eich [:brendan]]

Fixed for Firefox 3 by patch for bug 274152 -- see bug 274152 comment 43 for the checkin date (also, not quite perfect hindsight: please search closed bugs first for format control keywords).

/be

Comment 2

[Brian Crowder]

Is it possible that bug 268516 re-introduced this bug, for BOM characters?

Comment 3

[Brian Crowder]

js> alert(eval("(['a\\\ufffe', '+evil()])//')]")); 
evil
a', evil

Comment 4

[Dave Reed]

Search as I may, for 30 minutes, and could not find any bugs related to 'control format' characters. Hindsight only works if you have eyes.

Comment 5

[Brendan Eich [:brendan]]

Dave: this search is what I meant:

https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=format+control&classification=Components&product=Core&component=JavaScript+Engine&long_desc_type=substring&long_desc=&bug_file_loc_type=substring&bug_file_loc=&status_whiteboard_type=substring&status_whiteboard=&keywords_type=anywords&keywords=&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&bug_status=RESOLVED&bug_status=VERIFIED&bug_status=CLOSED&emailassigned_to1=1&emailtype1=substring&email1=&emailreporter2=1&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=

/be

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: ecma_3/extensions/regress-430740.js

Comment 7

[Brian Crowder]

How did this bug get resolved?  Are comments 2 and 3 not still issues?

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: ecma_3/extensions/regress-430740.js

I missed the second case by crowder. 1.8.1 fails the first part, 1.9.0 fails the second part.

Comment 9

[Brian Crowder]

Need a better fix here.

Comment 10

[Bob Clary [:bc] (inactive)]

is the 1.9.0 failure of the second part of the test due to bug 368516 ?

Comment 11

[Brian Crowder]

Yes, and that's what I -meant- to type in comment 2, but didn't.

Comment 12

[Brian Crowder]

brendan:  Some thoughts on this would be very helpful.  What will be the fate of randomly-occurring format characters like this, in ES4?

Comment 13

[Brendan Eich [:brendan]]

What's the problem? ES4 does not strip format control characters; it does try to cope with misplaced BOMs. The proposal to do both has been stable for a while.

/be

Comment 14

[Brian Crowder]

I guess I'm looking for a better idea of what you mean by "cope", and I'm not sure if I'm reading ecmascript.org correctly, or even looking in the right place.

Comment 15

[Brian Crowder]

Dave Reed:  If I substitute the \ufffe in my altered version of your example with just "a", the expression doesn't parse correctly.  Can you show me some more examples?

Comment 16

[Brendan Eich [:brendan]]

The write-up in http://www.ecmascript.org/es4/spec/incompatibilities.pdf says:

ES4 therefore specifies that class Cf characters should not be stripped from source text at all, with the exception of byte order marks (BOMs), which must continue to be stripped everywhere.

This bug makes the case that BOM as well as class Cf characters should not be stripped from within string literals. I'll try to get this onto the Ecma TC39 committee's radar for the next face-to-face meeting.

/be

Comment 17

[Lars T Hansen]

This has been resolved for ES4, IMO.  The resolution is that Cf characters are not stripped at all, but that a BOM is interpreted in the same way as white space between tokens.  (Thus a BOM in a string or regular expression literal has the value of the BOM, not a space value.)  The alternative is to translate BOM to U+0020 also within string and regular expression literals, but IMO that is an inferior solution, as the former solution does not translate at all -- it just gives a compatible-with-ES3 interpretation to BOM in certain contexts.

Comment 18

[Daniel Veditz [:dveditz]]

Brendan, Brian: is this something we'd fix on the 1.8 branch, too?

Comment 19

[Brian Crowder]

I'm actually not convinced anymore that this is that serious a bug, and the original poster has not provided an exploitable example, in which the enclosing code does not itself contain a syntax error.  We should probably WONTFIX or morph this bug to be the "make SM compatible with ES4" bug, which means doing what Lars documents in comment #17.

Comment 20

[Dave Reed]

My original example does not result in a syntax error. The code within the supposed string isn't valid by itself, but that would be necessary by an attacker to make the unquoted string form valid code in the context in which it is used. Hence, the string ends in // so that remaining 'actual' code is considered commented out. 

Comment 21

[Brian Crowder]

Attachment: the fix

Actually, yeah.  You're right.  Here's an attempt at a patch; it needs more testing to be ready for review, though.

Comment 22

[Brian Crowder]

Comment on attachment 329544 [details] [diff] [review]
the fix

I have -no idea- why mercurial has so much difficulty generating a minimized diff of this, I can't get it to do -b or -w.  I'll have a manually-generated diff shortly.

Comment 23

[Brian Crowder]

Attachment: for review purposes

jsscan.cpp versus the fixed jsscan.cpp generated by |diff -bpU8|

Comment 24

[Brian Crowder]

Comment on attachment 329544 [details] [diff] [review]
the fix

This passes js/tests and Mochitests.

Comment 25

[Brian Crowder]

Comment on attachment 329544 [details] [diff] [review]
the fix

Passing review to Igor.

Comment 26

[Igor Bukanov]

(In reply to comment #25)
> (From update of attachment 329544 [details] [diff] [review])
> Passing review to Igor.
> 

From the first impression the patch replaces BOM by whytespace in regexp literals, doesn't it?

Comment 27

[Brian Crowder]

> From the first impression the patch replaces BOM by whytespace in regexp
> literals, doesn't it?

String literals, not regexp literals, unless I did something terribly wrong.

Comment 28

[Brendan Eich [:brendan]]

Brian, see comment 17. BOM in string or regexp must be preserved, BOM outside quotes or slashes becomes space.

/be

Comment 29

[Igor Bukanov]

(In reply to comment #28)
> Brian, see comment 17. BOM in string or regexp must be preserved, BOM outside
> quotes or slashes becomes space.

I interpret that idea that BOM between tokens means whitespace as preserving BOM not only inside string and regexps but also inside numbers and names. That is, 1<bom>e7 should not mean 1 e7 but rather lead to a syntax error. 

Comment 30

[Igor Bukanov]

(In reply to comment #29)
> I interpret that idea that BOM between tokens means whitespace as preserving
> BOM not only inside string and regexps but also inside numbers and names. That
> is, 1<bom>e7 should not mean 1 e7 but rather lead to a syntax error. 

Here is another example:

-<bom>-a

With the stripping rule that becomes "--a" while with bom-as-whitespace that would be "- -a" or just "+a". This is not good in my book.

Comment 31

[Brendan Eich [:brendan]]

Igor: problem is IE set a precedent. Can someone test IE8 beta to confirm it does turn BOM outside of string or regexp into a space?

/be

Comment 32

[Brian Crowder]

IE8 and IE7 seem to actually error out on BOMs anywhere except inside regexp and string literals, from my testing.  Both 7 and 8 throw an "invalid character" exception if the BOM occurs anywhere but at the location of the first character in the script (string, in eval), or in a regexp or string literal.  Also, IE throws an invalid character error for non-BOM format control characters (\u200d, as in the example from comment #1) occuring -anywhere- except inside of string/regexp literals.

Comment 33

[Brian Crowder]

My comment #32 is wrong.  IE7/8 handle BOMs differently inside strings being eval()d than inside <script> tags or code included from <script> tags.  Does eval() lose the utf-8 encoding attribute, perhaps?

Comment 34

[Brian Crowder]

At any rate, what IE does for BOMs inside <script> tags is to convert them to space (rather than eliding them as we currently do).  This means that my "the fix" is relatively close to their handling, except that I must now make eval() break to match them.

Comment 35

[Brian Crowder]

Also, I need to add the same exemption for regexp literals.

Comment 36

[Igor Bukanov]

(In reply to comment #33)
> My comment #32 is wrong.  IE7/8 handle BOMs differently inside strings being
> eval()d than inside <script> tags or code included from <script> tags.  Does
> eval() lose the utf-8 encoding attribute, perhaps?

What would MSIE alerts for the following case:

<script>
    var a = 0;
    -<bom>-a;
    alert(a);
<script>

??

Comment 37

[Brian Crowder]

> What would MSIE alerts for the following case:
> 
> <script>
>     var a = 0;
>     -<bom>-a;
>     alert(a);
> <script>

IE alerts 0, not -1

Comment 38

[Igor Bukanov]

(In reply to comment #37)
> > What would MSIE alerts for the following case:
> > 
> > <script>
> >     var a = 0;
> >     -<bom>-a;
> >     alert(a);
> > <script>
> 
> IE alerts 0, not -1

On more test, what about

<script>
    var a = eval('"<bom>"')
    alert(a.charCodeAt(0));
</script>

That is, does that bom->whitespace replacement happens on HTML parser or script level?

Comment 39

[Brian Crowder]

For 0xfeff, this alerts 65279.

Comment 40

[Igor Bukanov]

(In reply to comment #39)
> For 0xfeff, this alerts 65279.

Ok , the last curiosity attempt:

<script>
function f() {<bom>}

var s = f.toString().replace('\ufeff', 'T');
alert(s);
</script>

I.e. the question is what exactly character MSIE uses to replace the bom? Or perhaps it does not replace it with WS at all but rather treats BOM as a whitespace in the lexer outside of eval context?  

Comment 41

[Brian Crowder]

function f() {T}

Comment 42

[Igor Bukanov]

(In reply to comment #41)
> function f() {T}
> 

So MSIE simply treats BOM as a whitespace for the purpose of parsing. Which suggests to do this in SM to fix the bug: treat BOM as one of Unicode whitespace characters in the scanner avoiding any character skipping or patching.

Comment 43

[Daniel Veditz [:dveditz]]

Is the patch only for the BOM? What about \u200d from the original example, and the other :Cf: characters listed in the URI?

Comment 44

[Brian Crowder]

Non BOM Cf characters aren't stripped anymore (note that comment 0 references an Fx2 build-id), so they get treated appropriately (typically, we'll throw an "illegal character" exception).  That has been the case since mrbkap's original fix for bug 274152 landed.  I excepted BOM characters in bug 368516 because of concerns over web-compability, but stripping them out wasn't the Right Thing.  The Right Thing (or close to it, I think, given the situation), is to treat them as whitespace.

The 1.8 patch for this will likely be at least a little different.

Comment 45

[Brian Crowder]

Pratap:  I'm hoping you (or another engineer) can shed some light on the difference in behavior for IE7/8's eval() here, as opposed to the handling of <script> tags.

Comment 46

[Brian Crowder]

Attachment: Here's another swing at emulating IE.

We still don't do the same thing IE does for eval cases (we'll continue to treat BOMs as whitespace inside eval.  This saves us from the exploit, but doesn't precisely emulated IE.

Comment 47

[Brian Crowder]

Attachment: diff -w of previous

For convenience in review.

Comment 48

[Igor Bukanov]

Comment on attachment 329910 [details] [diff] [review]
Here's another swing at emulating IE.

>--- mozilla.4b761ffed330/js/src/jsscan.cpp	2008-07-16 15:11:37.000000000 -0700
>+++ /Users/crowder/mozilla/js/src/jsscan.cpp	2008-07-16 14:56:37.000000000 -0700
>+static JS_INLINE JSBool
>+ScanAsSpace(jschar ch)
>+{
>+    if (JS_ISSPACE(ch) || ch == 0xfffe || ch == 0xfeff)
>+        return JS_TRUE;
>+    return JS_FALSE;
>+}

Nit: comment that 0xfeff and 0xfffe are BOMs treated as whitespace for compatibility.

Comment 49

[Brendan Eich [:brendan]]

Comment on attachment 329911 [details] [diff] [review]
diff -w of previous

Sorry, drive-by: IsSpace would be canonical name (someone pointed out that BOM code points are Unicode space chars going by attributes), instead of ScanAsSpace. Likewise, c not ch.

/be

Comment 50

[Brendan Eich [:brendan]]

Come to think of it, does JS_ISSPACE treat BOMs as space already?

/be

Comment 51

[Igor Bukanov]

(In reply to comment #50)
> Come to think of it, does JS_ISSPACE treat BOMs as space already?

BOMs are zero-width space characters which is different from the space character attribute.

Comment 52

[Brian Crowder]

Attachment: the patch to land

Includes the ch => c rewrite, but retains ScanAsSpace as the function name, per discussion with brendan on IRC.

Comment 53

[Brian Crowder]

Attachment: 1.9 branch patch

Applies without fuzz on the branch, other than the module name (jsscan.cpp => jsscan.c)

Comment 54

[Brian Crowder]

I think this bug is a legit XSS bug and rightly deserves the blocking statuses requested for it for both branches.

Comment 55

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/ddb85d02a8f5

Comment 56

[Shawn Wilsher :sdwilsh]

OK, now it's really there (there were some issues before)

Comment 57

[Daniel Veditz [:dveditz]]

Is the "branch patch" for both branches, or is that only the 1.9 "branch"?

Comment 58

[Brian Crowder]

(In reply to comment #57)
> Is the "branch patch" for both branches, or is that only the 1.9 "branch"?

Only for 1.9, yeah.  I'll roll a 1.8 patch next.

Comment 59

[Brian Crowder]

Attachment: 1.8 branch patch

Fx2 is vulnerable to -all- Cf characters, not just BOMs.  This fixes that.  Asking for re-review, since the patch didn't apply trivially.  If 1.8.1.17 is not the branch I should be seeking approval for, please fix.

Comment 61

[Bob Clary [:bc] (inactive)]

verified m-c:1.9.1

Comment 62

[Chris Weber]

Sorry for the trivial question but I'm new to bugzilla - is there a nightly build (Windows preferably) with the fix included that I can grab to run my test cases against?

Comment 63

[Brian Crowder]

The builds here should have the latest and greatest, including the patch landed on the 17th.

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

Comment 64

[Samuel Sidler (old account; do not CC)]

Comment on attachment 330105 [details] [diff] [review]
1.9 branch patch

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 65

[Daniel Veditz [:dveditz]]

Comment on attachment 330318 [details] [diff] [review]
1.8 branch patch

Approved for 1.8.1.17, a=dveditz for release-drivers

Comment 66

[Bob Clary [:bc] (inactive)]

Did this cause ecma_3/extensions/regress-368516.js to regress or is the test no longer valid?

Comment 67

[Brian Crowder]

Yes, that test is invalid now; BOMs aren't stripped anymore, but are treated as whitespace in parsing.

Comment 68

[Igor Bukanov]

(In reply to comment #66)
> Did this cause ecma_3/extensions/regress-368516.js to regress or is the test no
> longer valid?
> 

The test contains:

eval("hi" + bomchars[i] + "there = 'howdie';");

With the patch this equivalent to eval("hi there = 'howdie';") which is not a valid code. So the test is no longer applicable. Ideally it should be changed to verify that boms are treated precizely as a whitespace, like in 

eval("var"+bom+"x;")

Comment 69

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/ecma_3/extensions/regress-368516.js,v  <--  regress-368516.js
new revision: 1.2; previous revision: 1.1

mozilla-central: 16398:4da7cad4b133

Comment 70

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.1, 1.9.0, 1.9.1 linux/mac/win shell/browser

Comment 71

[Alexander Sack]

Comment on attachment 330318 [details] [diff] [review]
1.8 branch patch

a=asac for 1.8.0.15

taking for distros

Comment 72

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/ecma_3/extensions/regress-430740.js,v  <--  regress-430740.js
initial revision: 1.1

http://hg.mozilla.org/mozilla-central/rev/2d7d00ef041e

Comment 73

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ddb85d02a8f5

Comment 74

[Bob Clary [:bc] (inactive)]

v 1.9.1