Bug 90667 – [JSC] HTML extensions to String.prototype should escape " as " in argument values

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

[Mathias Bynens]

Currently:

> '_'.link('a"b')
'<a href="a"b">_</a>'

Expected result:

> '_'.link('a"b')
'<a href="a&quot;b">_</a>'

The problem here is JSC doesn’t escape " into &quot; at the moment, which is a potential security risk (XSS vector).

For this reason, Chrome/V8 escapes " into &quot;. Firefox/Spidermonkey is going to change its behavior to do the same: https://bugzilla.mozilla.org/show_bug.cgi?id=352437 Opera/Carakan will change its behavior too, as soon as other browsers change (bug DSK-369206).

http://mathias.html5.org/specs/javascript/#escapeattributevalue requires escaping the ".

Tests: http://mathias.html5.org/tests/javascript/string/

Here’s a list of the methods that have this issue:

* String.prototype.anchor(name)
* String.prototype.fontcolor(color)
* String.prototype.fontsize(size)
* String.prototype.link(href)

See http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/StringPrototype.cpp.

[Mathias Bynens]

(In reply to comment #0)
> For this reason, Chrome/V8 escapes " into ". Firefox/Spidermonkey is going to change its behavior to do the same: https://bugzilla.mozilla.org/show_bug.cgi?id=352437

Update: Firefox/Spidermonkey just landed this change.
https://bugzilla.mozilla.org/show_bug.cgi?id=352437#c16

[Adam Barth]

> '_'.link('a"b')
"<a href="a&quot;b">_</a>"

Seems to work as expected in V8.

[Mathias Bynens]

(In reply to comment #2)
> > '_'.link('a"b')
> "<a href="a&quot;b">_</a>"
> 
> Seems to work as expected in V8.


That’s what I said in comment #0, no?

V8 does the right thing and escapes " as &quot; correctly, but it also (needlessly) escapes some other characters, i.e. U+0027 APOSTROPHE and the < and > characters. These other escapes aren’t needed for security, and no other engine applies them. For this reason, I’ve filed a bug and submitted a patch against that behavior here: http://code.google.com/p/v8/issues/detail?id=2217

[Chris Dumez]

Created attachment 172367 [details]
Patch

[Benjamin Poulain]

Comment on attachment 172367 [details]
Patch

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

No clue if the patch is correct, but some comments regarding performance:

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1391
> +    String color = a0.toString(exec)->value(exec);

JSValue has toWTFString(), it is more efficient for some types when the value is not a string.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1392
> +    // Escape quotation mark.

The Comment is not adding anything.

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1393
> +    color.replace('"', ASCIILiteral("""));

I don't like the idea of creating a new String every time. ASCIILiteral() will save you some time, but we can do better.
What about adding StringImpl::replace() for string literals?

[Chris Dumez]

Comment on attachment 172367 [details]
Patch

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

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1391
>> +    String color = a0.toString(exec)->value(exec);
> 
> JSValue has toWTFString(), it is more efficient for some types when the value is not a string.

Ok

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1392
>> +    // Escape quotation mark.
> 
> The Comment is not adding anything.

Ok

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1393
>> +    color.replace('"', ASCIILiteral("""));
> 
> I don't like the idea of creating a new String every time. ASCIILiteral() will save you some time, but we can do better.
> What about adding StringImpl::replace() for string literals?

I think this is a good idea this should probably be done in a separate patch, right?

[Chris Dumez]

Created attachment 172380 [details]
Patch

Take Benjamin's feedback into consideration.

I haven't added a StringImpl::replace() that takes a literal yet though.

[kov's GTK+ EWS bot]

Comment on attachment 172380 [details]
Patch

Attachment 172380 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14722812

[Benjamin Poulain]

> >> Source/JavaScriptCore/runtime/StringPrototype.cpp:1393
> >> +    color.replace('"', ASCIILiteral("""));
> > 
> > I don't like the idea of creating a new String every time. ASCIILiteral() will save you some time, but we can do better.
> > What about adding StringImpl::replace() for string literals?
> 
> I think this is a good idea this should probably be done in a separate patch, right?

Yep, that would be better in a separate patch.

[Chris Dumez]

Created attachment 172431 [details]
Patch

Stop using ASCIILiteral(). Calling String::replace() with a literal replacement string will no longer construct a WTF::String object after Bug 101257 is closed.

[Chris Dumez]

Created attachment 172746 [details]
Patch

Use replaceWithLiteral() instead of replace() for efficiency (now that the dependency patch landed).

Any feedback on this patch?

[Benjamin Poulain]

Comment on attachment 172746 [details]
Patch

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

The patch itself looks good. I think the tests could be better.

> LayoutTests/ChangeLog:14
> +        * fast/js/script-tests/string-prototype-escape-attribute.js: Added.
> +        * fast/js/string-prototype-escape-attribute-expected.txt: Added.
> +        * fast/js/string-prototype-escape-attribute.html: Added.

I think the test names are not descriptive enough.
Wouldn't it be better to create one test for each of those functions? A quick grep in fast/js tells me their coverage may be bad, but maybe they are tested elsewhere?

> LayoutTests/fast/js/script-tests/string-prototype-escape-attribute.js:5
> +shouldBe("'_'.anchor('\x22')", '"<a name=\\"&quot;\\">_</a>"');

Why use \x22 instead of "? it looks more confusing.
You should also have tests with the quote being with some text and pathologically bad examples, e.g:
"_".anchor('" href="http://evil.com"')

[Chris Dumez]

Created attachment 172946 [details]
Patch

Take Benjamin's feedback into consideration. I added a lot of more tests.

[WebKit Review Bot]

Comment on attachment 172946 [details]
Patch

Attachment 172946 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14762736

New failing tests:
fast/js/string-fontsize.html
fast/js/string-anchor.html
fast/js/string-link.html
fast/js/string-fontcolor.html

[Chris Dumez]

It appears V8 does not throw for null / undefined arguments.

According to [1] it is supposed to throw a type error when CheckObjectCoercible is called for null or undefined.

According to [2], CheckObjectCoercible should be called for those methods.

This is causing the new tests to fail on Chromium port.

[1] http://ecma-international.org/ecma-262/5.1/#sec-9.10
[2] http://mathias.html5.org/specs/javascript/#string.prototype.anchor

[Benjamin Poulain]

(In reply to comment #15)
> It appears V8 does not throw for null / undefined arguments.
> 
> According to [1] it is supposed to throw a type error when CheckObjectCoercible is called for null or undefined.
> 
> According to [2], CheckObjectCoercible should be called for those methods.
> 
> This is causing the new tests to fail on Chromium port.
> 
> [1] http://ecma-international.org/ecma-262/5.1/#sec-9.10
> [2] http://mathias.html5.org/specs/javascript/#string.prototype.anchor

That shows the test coverage has already improved :)

Just in case, check what the other browsers are doing, sometime a spec is ignored by everyone. If JSC is not the only engine with that behavior, I guess we'll just add a failure in Chromium's test expectations.

[Chris Dumez]

(In reply to comment #16)
> (In reply to comment #15)
> > It appears V8 does not throw for null / undefined arguments.
> > 
> > According to [1] it is supposed to throw a type error when CheckObjectCoercible is called for null or undefined.
> > 
> > According to [2], CheckObjectCoercible should be called for those methods.
> > 
> > This is causing the new tests to fail on Chromium port.
> > 
> > [1] http://ecma-international.org/ecma-262/5.1/#sec-9.10
> > [2] http://mathias.html5.org/specs/javascript/#string.prototype.anchor
> 
> That shows the test coverage has already improved :)
> 
> Just in case, check what the other browsers are doing, sometime a spec is ignored by everyone. If JSC is not the only engine with that behavior, I guess we'll just add a failure in Chromium's test expectations.

It appears there is already a bug filed for that:
http://code.google.com/p/v8/issues/detail?id=2218

It seems that Opera and Firefox throw, IE doesn't.

Should I skip these new test cases for Chromium port? Or should I remove those checks from the test cases?

[Benjamin Poulain]

> Should I skip these new test cases for Chromium port? Or should I remove those checks from the test cases?

Don't remove the checks, it is great you added them. You can just add a failure expectation for Chromium.

[Chris Dumez]

Created attachment 173076 [details]
Patch

Skip new tests for chromium port.

[Benjamin Poulain]

Comment on attachment 173076 [details]
Patch

Looks good to me, good tests too.

[WebKit Review Bot]

Comment on attachment 173076 [details]
Patch

Clearing flags on attachment: 173076

Committed r133966: <http://trac.webkit.org/changeset/133966>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.