escape_javascript helper corrupts strings #5127

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

benmmurphy commented Feb 22, 2012

  <%= javascript_tag do %>

     window.myvar = '<%= j "\u2028\\\r\n\"'<>&" %>';
         alert(JSON.stringify(window.myvar));
  <% end %>

this produces the following output:

"&amp;#x2028;\\\n&quot;'&lt;&gt;&amp;"

this happens because the content of a script tag is not interpreted as html except for certain sequences which look like a closing </script> tag to the browser. but rails escapes &,>,<," which means the values end up getting escaped too many times.

I think this is quite unexpected behaviour but may be a feature because it stops xss attacks caused by the user doing something like:

my_element.innerHtml = window.myvar;

i've added a new method to the helper called escape_javascript_string that doesn't cause the value of the string to be modified. it does this by changing html characters to unicode javascript escapes. example:

  <%= javascript_tag do %>

     window.myvar2 = '<%= escape_javascript_string "\u2028\\\r\n\"'<>&" %>';

     alert(JSON.stringify(window.myvar2));
  <% end %>

which produces the following output (chrome JSON.stringify doesn't escape unicode new line):

"

\\\r\n\"'<>&"
@benmmurphy benmmurphy add escape_javascript_string helper that escapes a string safely for …
…javascript that can also be safely embedded in a script tag while not corrupting any characters.
7a92aa6
Contributor

benmmurphy commented Feb 22, 2012

thanks! that is very close to what i'm looking for.

unfortunately, it doesn't escape \u2028 or \r or \n so i get a script error

output it produces:

window.myvar = "
\
'\u003C\u003E\u0026";

1.9.3p0 :004 > ERB::Util.json_escape "\r\n"
 => "\r\n"

i think those characters (not including \u2028. i think that is javascript specific) need to be escaped in json according to the rfc as well as the other control characters

http://www.ietf.org/rfc/rfc4627

The representation of strings is similar to conventions used in the C
family of programming languages.  A string begins and ends with
quotation marks.  All Unicode characters may be placed within the
quotation marks except for the characters that must be escaped:
quotation mark, reverse solidus, and the control characters (U+0000
through U+001F). 
Member

josevalim commented Feb 22, 2012

So maybe we should fix json escape to not double escape?

Contributor

benmmurphy commented Feb 22, 2012

we also aren't escaping u2029 (similar to u2028) which javascript interprets as a new line. i'm not sure if this is an exploitable xss issue or not in current rails. i can create js errors with it but i didn't try to inject code with it. if the developer does something like the following which you might expect to be safe because you believe the function escapes all new line characters (i would hope that no-one would write code like this) :

// <%= escape_javascript user_influenced_variable %>

then it is exploitable. i don't know of any other way to exploit it.

see section 7.8.4 for string literals and section 7.3 for line terminators:
http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf

i'm not sure what other characters we should be escaping. the standard says they should all be safe except line terminators and reverse solidus () but maybe control characters in 0000->0001F range might do funny things in some browsers.

Contributor

benmmurphy commented Feb 23, 2012

ah. i think i understand the json_escape method now. you can do:

json_escape(escape_javascript(myvar))

and it works correctly. a bit closer anyway :) it seems to remove the " character and doesn't properly handle \u2029

Contributor

isaacsanders commented May 5, 2012

Is this still an issue?

Member

steveklabnik commented Sep 14, 2012

We should certainly fix json_escape rather than making a new method that also does escaping.

@benmmurphy are you interested in doing that? if not, we should close this, and let someone else take a crack at it.

Member

steveklabnik commented Nov 17, 2012

Since I haven't heard from you in 9 months, @benmmurphy, I'm giving this a close. If you're willing to fix json_escape, please submit a new pull request. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment