Last Comment Bug 352437 - string.link does not escape url
: string.link does not escape url
Status: RESOLVED FIXED
[sg:want]
: dev-doc-complete, sec-want, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla17
Assigned To: :Ms2ger
:
Mentors:
: 437618 536837 649818 (view as bug list)
Depends on:
Blocks: xss
  Show dependency treegraph
 
Reported: 2006-09-12 18:20 PDT by Jesse Ruderman
Modified: 2012-12-11 11:28 PST (History)
17 users (show)
sayrer: wanted1.9.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part a: Use StringBuffer (2.52 KB, patch)
2012-07-06 08:14 PDT, :Ms2ger
jorendorff: review+
Details | Diff | Review
Part b: Get rid of some strlen() calls (4.59 KB, patch)
2012-07-06 08:15 PDT, :Ms2ger
no flags Details | Diff | Review
Part c: Escape quotes (1.73 KB, patch)
2012-07-06 08:16 PDT, :Ms2ger
jorendorff: review+
Details | Diff | Review

Description Jesse Ruderman 2006-09-12 18:20:41 PDT
js> "foo".link("http://www.google.com/search?q=foo&num=100")
<a href="http://www.google.com/search?q=foo&num=100">foo</a>

In HTML, & in links needs to be escaped as &amp;
Comment 1 Jesse Ruderman 2006-10-23 20:32:46 PDT
This is worse than I thought: the double-quote character (") isn't escaped either.  This caused an XSS hole in mozilla.com (bug 357750), where the developer assumed that String.link was an API for creating a link from any link text and URL.  I think we should meet that assumption (rather than telling developers not to use String.link or to escape or sanitize text passed to it), but we'd need to get other browser vendors to fix their String.link implementations too for that problem to really go away.
Comment 2 Brian Crowder 2008-06-09 14:10:28 PDT
*** Bug 437618 has been marked as a duplicate of this bug. ***
Comment 3 Daniel Veditz [:dveditz] 2008-06-17 00:40:31 PDT
As pointed out in bug 437618 the problem is not limited to .link(), it's all the HTML-helper string methods. e.g.

> javascript:"foo".anchor("hello\"><script>alert(1)</script>")
> javascript:"foo".fontcolor("hello\"><script>alert(1)</script>")
> javascript:"foo".fontsize("hello\"><script>alert(1)</script>")
> javascript:"foo".link("hello\"><script>alert(1)</script>")
Comment 4 Robert Sayre 2008-06-18 10:10:24 PDT
It's not clear that we can fix this, but we'll mark it for investigation.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2009-12-26 18:35:11 PST
*** Bug 536837 has been marked as a duplicate of this bug. ***
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-13 15:01:24 PDT
*** Bug 649818 has been marked as a duplicate of this bug. ***
Comment 7 Mathias Bynens 2012-07-04 04:49:16 PDT
Here’s a spec for the non-standard string extensions; an attempt to get
browsers to align their implementations:
http://mathias.html5.org/specs/javascript/#escapeattributevalue

It only requires escaping U+0022 QUOTATION MARK as &quot; (not U+0026
AMPERSAND as &amp;) — see note below for the reasoning behind this.

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

(In reply to Robert Sayre from comment #4)
> It's not clear that we can fix this, but we'll mark it for investigation.

Note that WebKit does escape U+0022 QUOTATION MARK as &quot;. It also escapes
U+0027 APOSTROPHE as &#039;, although that seems pointless. It doesn’t escape
U+0026 AMPERSAND, though. No browser does.

As it doesn’t seem to cause any compatibility problems for WebKit, it seems
possible to fix the &quot; issue. I’m afraid the &amp; “issue” is harder to
fix, as there’s no precedent.
Comment 8 Simon Pieters 2012-07-05 03:32:54 PDT
I agree we shouldn't change &. There may be sites that "do the right thing" and escape both & and " before using these methods, that would break if we start escaping &. It also means it is harder for sites to plug their XSS hole for legacy browsers if new browsers start escaping &.

Escaping " seems like a good thing to do, IMHO.
Comment 9 Mathias Bynens 2012-07-06 02:44:13 PDT
My previous comment (comment 7) should’ve said V8 instead of WebKit.

For some reason I’m not allowed to add these URLs to the “See Also” list, so here goes:

* Safari/JavaScriptCore: https://bugs.webkit.org/show_bug.cgi?id=90667
* IE/Chakra: https://connect.microsoft.com/IE/feedback/details/752391
* Opera/Carakan: https://bugs.opera.com/browse/CORE-47348 (DSK-369206/CORE-47348)
Comment 10 :Ms2ger 2012-07-06 08:14:59 PDT
Created attachment 639678 [details] [diff] [review]
Part a: Use StringBuffer

For nice code.
Comment 11 :Ms2ger 2012-07-06 08:15:59 PDT
Created attachment 639680 [details] [diff] [review]
Part b: Get rid of some strlen() calls

For ever so slightly faster code
Comment 12 :Ms2ger 2012-07-06 08:16:51 PDT
Created attachment 639681 [details] [diff] [review]
Part c: Escape quotes

For better conforming code.
Comment 13 Mathias Bynens 2012-07-09 01:59:53 PDT
See also: https://bugs.ecmascript.org/show_bug.cgi?id=406
Comment 14 Jason Orendorff [:jorendorff] 2012-07-20 13:34:21 PDT
Comment on attachment 639680 [details] [diff] [review]
Part b: Get rid of some strlen() calls

We shouldn't optimize stuff like this. It's better to do the slow thing than to have the compiler spit out a dozen instantiations of a template.

If you really want to eliminate the strlen calls, make tagify_impl take beginLen and endLen as arguments (it can assert that those arguments are actually equal to strlen(begin) and strlen(end)), then make an inline function template that uses this trick to pass them in. Then measure the perf difference. This doesn't seem worth the effort to me though.
Comment 15 Jason Orendorff [:jorendorff] 2012-07-20 13:35:00 PDT
Comment on attachment 639678 [details] [diff] [review]
Part a: Use StringBuffer

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

r=me with or without the changes.

::: js/src/jsstr.cpp
@@ +2791,1 @@
>          taglen += 2 + parlen + 1;                       /* '="param"' */

Feel free to eliminate the temporary entirely (and then the curly braces here).

@@ +2799,5 @@
>          return false;
> +
> +    sb.infallibleAppend('<');
> +
> +    MOZ_ALWAYS_TRUE(sb.appendInflated(begin, beglen));

Feel free to add methods to vm/StringBuffer.h:
    inline void infallibleAppendInflated(const char *cstr, size_t len);
    inline void infallibleAppend(JSLinearString *str);
that do this assertion.
Comment 17 Ed Morley [:emorley] 2012-08-04 11:29:53 PDT
Backed out with the mass tree revert to get rid of the OS X M5 orange:
https://hg.mozilla.org/mozilla-central/rev/c801b99d726f

Note You need to log in before you can comment on or make changes to this bug.