XSS vulnerability: serialization of props, exposed on the client

[alex35mil]

As we discussed a few days ago, there is an issue with serialization of props, exposed on the client. Since these props may contain user's input and they will be exposed as javascript variable in the browser, we must escape it to prevent harmful script injection.

Here is the lib that I'm using in Node environment to serialize javascript before it will be exposed on the client: https://github.com/yahoo/serialize-javascript/

We should implement escaping of JS the same way, but on the Ruby side.

/cc: @justin808 @samnang

[justin808]

Let's get to this by the weekend, @alexfedoseev, @samnang.

[samnang]

@alexfedoseev let's have some discussion on monday when you get up.

[alex35mil]

@samnang 👍

[justin808]

This is probably NOT an issue for the following reason:

We use Rails to_json which does the encoding.

https://github.com/rails/rails/blob/master/activesupport/lib/active_support/json/encoding.rb#L51-51

private
          # Rails does more escaping than the JSON gem natively does (we
          # escape \u2028 and \u2029 and optionally >, <, & to work around
          # certain browser problems).
          ESCAPED_CHARS = {
            "\u2028" => '\u2028',
            "\u2029" => '\u2029',
            '>'      => '\u003e',
            '<'      => '\u003c',
            '&'      => '\u0026',
            }

          ESCAPE_REGEX_WITH_HTML_ENTITIES = /[\u2028\u2029><&]/u
          ESCAPE_REGEX_WITHOUT_HTML_ENTITIES = /[\u2028\u2029]/u

          # This class wraps all the strings we see and does the extra escaping
          class EscapedString < String #:nodoc:
            def to_json(*)
              if Encoding.escape_html_entities_in_json
                super.gsub ESCAPE_REGEX_WITH_HTML_ENTITIES, ESCAPED_CHARS
              else
                super.gsub ESCAPE_REGEX_WITHOUT_HTML_ENTITIES, ESCAPED_CHARS
              end
            end

            def to_s
              self
            end
          end

HOWEVER, we do support passing a string as a param of the JSON.

1. Would a rails user convert an object to JSON without using the rails way?
2. Should we require the passing of a string to pass an extra param saying that it's escaped.

I'm researching this a bit more.

The big hazard is that somebody will use JSON.generate (Ruby stdlib) rather than Rails to_json.

[justin808]

I just pushed shakacode/react-webpack-rails-tutorial#108, which shows how jBuilder will create the JSON string.

This is a sample result. Note how symbols like < are encoded.

//<![CDATA[
(function() {
  window.__appData0__ = [{"id":63,"author":"aaa","text":"dddd","created_at":"2015-09-21T22:44:33.602Z","updated_at":"2015-09-21T22:44:33.602Z"},{"id":64,"author":"afaaf","text":"ffffff","created_at":"2015-09-27T18:20:20.713Z","updated_at":"2015-09-27T18:20:20.713Z"},{"id":65,"author":"afaaf","text":"**afafafa**","created_at":"2015-09-27T18:20:29.251Z","updated_at":"2015-09-27T18:20:29.251Z"},{"id":66,"author":"Justin","text":"\u003c/script\u003e\u003cscript\u003ealert(\"WTF\");\u003c/script\u003e","created_at":"2015-10-04T22:53:12.373Z","updated_at":"2015-10-04T22:53:12.373Z"},{"id":67,"author":"JG","text":"' some \" \" ' etc.","created_at":"2015-10-04T23:09:51.375Z","updated_at":"2015-10-04T23:09:51.375Z"},{"id":68,"author":"aaa","text":"dddd","created_at":"2015-10-05T01:49:23.188Z","updated_at":"2015-10-05T01:49:23.188Z"},{"id":69,"author":"ddd","text":"xxxxxxx","created_at":"2015-10-05T01:50:03.228Z","updated_at":"2015-10-05T01:50:03.228Z"},{"id":70,"author":"ddd","text":"cccccccc","created_at":"2015-10-05T01:50:22.336Z","updated_at":"2015-10-05T01:50:22.336Z"},{"id":71,"author":"addddd","text":"aaaa","created_at":"2015-10-05T01:59:15.763Z","updated_at":"2015-10-05T01:59:15.763Z"}];
  ReactOnRails.clientRenderReactComponent({
    componentName: 'App',
    domId: 'App-react-component-0',
    propsVarName: '__appData0__',
    props: window.__appData0__,
    trace: true,
    generatorFunction: true,
    expectTurboLinks: true
  });
})();

//]]>

[justin808]

So the question is whether or not we should check the String (not Hash) to see if there might be some non-escaped characters. Maybe we should always escape unless an option is set? That seems like a reasonable solution, as we could put the option in the global configuration file for a default.

Maybe the option could be:

escape_javascript: true

Or we can always run this escape:

          ESCAPED_CHARS = {
            "\u2028" => '\u2028',
            "\u2029" => '\u2029',
            '>'      => '\u003e',
            '<'      => '\u003c',
            '&'      => '\u0026',
            }

          ESCAPE_REGEX_WITH_HTML_ENTITIES = /[\u2028\u2029><&]/u

          json_string.gsub(ESCAPE_REGEX_WITH_HTML_ENTITIES, ESCAPED_CHARS)

I'm just against doing this every time unless we can turn this off.

As I mentioned, somebody would have to be using a method like JSON.generate to get non-escaped JSON. Jbuilder, rails to_json all escape. Rails tries to keep everybody safe!

[justin808]

Example of safe and non-safe json conversion

[24] (pry) main: 0> aa
{
        :hello => "world",
         :free => "of charge",
      "script" => "<script>alert(\"yo\");</script>",
    "<script>" => "<script>alert(\"yo\");</script>"
}
[25] (pry) main: 0> non_safe = JSON.generate(aa)
"{\"hello\":\"world\",\"free\":\"of charge\",\"script\":\"<script>alert(\\\"yo\\\");</script>\",\"<script>\":\"<script>alert(\\\"yo\\\");</script>\"}"

[27] (pry) main: 0> safe = aa.to_json
"{\"hello\":\"world\",\"free\":\"of charge\",\"script\":\"\\u003cscript\\u003ealert(\\\"yo\\\");\\u003c/script\\u003e\",\"\\u003cscript\\u003e\":\"\\u003cscript\\u003ealert(\\\"yo\\\");\\u003c/script\\u003e\"}"

[alex35mil]

My vote is for "always escape", this way we won't let the user shoot in his leg (and I don't see a reason why it might be turned off).

[justin808]

@alexfedoseev My only concern is that it might be a slight performance disadvantage if sending over lots of data, and the developer is using the rails ways of creating the JSON strings.

Do you agree on the escaping rules I posted?

[alex35mil]

Yep. Well in this case we can:

• set escape_javascript: true by default
• in docs for this setting explain the differences between JSON methods and put a warning: don't turn this off unless you know what you're doing.

[samnang]

I still feel having an option on and off performance is early optimization for me. I vote for "always scape" and use Rails ActiveSupport::JSON::encode, so that we will take advantage from latest Rails security concern when they found something and they fix it.

[justin808]

@mapreal19 Maybe you can take a crack at this? Let's consider @samnang's suggestion on the encoding. We'll put off making this optional until needed.

@samnang is this the method that we need:
https://github.com/rails/activesupport-json_encoder/blob/master/lib/active_support/json/encoding/active_support_encoder.rb#L76

It seems to do more than Rails is doing by default for to_json. Maybe a good thing?