Mojo::JSON::to_json no longer produces output safe for inline JavaScript

[2shortplanks]

Mojo::JSON::to_json does not produce output safe for embedding inside inline JavaScript with arbitrary inputs. For example, this construct isn't safe:

<script>foo = <%= to_json($foo) %>;</script>

The problem comes with input containing a </script> sequence. For example, If $foo is { foo => "</script><script>alert('xss');" } the output will be produced as:

<script>foo = {"foo":"</script><script>alert('xss');"};</script>

Most browsers will execute the alert statement that was intended to be encoded data not executable code.

Ironically, this would have been protected had the encoding of / had not been removed in 5.28, as that would have given us output such as

<script>foo = {"foo":"<\/script><script>alert('xss');"};</script>

[kraih]

Some more things to consider, a) this was changed in 5.28 because of regular complaints from users, b) at the time Mojo:JSON::to_json and therefore this use case did not exist, c) escaping / as \/ is optional in the RFC, and d) other CPAN modules like JSON::XS do not support this, so if you monkey patch them into Mojo::JSON for better performance, you might potentially lose a security feature.

[jberger]

The fact that users might remove a security feature by monkey patching is a sad reason not to have it in the first place :/ . I would think a notice in the doc should take care of that.

I would have supported this as a configurable option on the oo interface but I know that @sri doesn't like to emphasize that interface anymore.

[kraih]

@jberger It's not just that i don't like to emphasize that interface, it is actually deprecated. e9e69af

[jberger]

Ex post facto
On Oct 23, 2014 1:28 PM, "Sebastian Riedel" notifications@github.com
wrote:

@jberger https://github.com/jberger It's not just that i don't like to
emphasize that interface, it is actually deprecated. e9e69af
e9e69af

—
Reply to this email directly or view it on GitHub
#693 (comment).

[kraih]

It's been a month, did this proposal fail?

[marcusramberg]

I am actually in favor of this, it seems that when we protect against xss in the template we should do so in the json helper too. Sorry for the late reply.

[kraih]

@marcusramberg You'd still be the only one. I don't think that's enough, but we can of course leave this issue open for another month.

[kraih]

For the record, this does not actually need to be implemented in Mojo::JSON.

perl -Mojo -E 'helper encode_js => sub { Mojo::JSON::to_json($_[1]) =~ s/\//\\\//gr }; say app->encode_js({foo => "test</script>123"})'

[2shortplanks]

It seems to me that the argument comes down to what is the correct thing to do by default since as you point out it's easy enough to put in any behavior you want without too much.

I'd like to make the argument still that the correct thing is the safest possible thing. A little uglyness in the JSON output is worth it for a little less security problems.

[kraih]

This proposal has been accepted with a vote from @marcusramberg.

[powerman]

It's two different tasks - generate JSON as per spec and inject Javascript data structure into html template. Format of Javascript data in html isn't the same as JSON and there are a lot of use cases when you need to generate JSON not for embedding into html.

So, if someone assume it's safe to embed JSON into html without extra escaping - it's security issue in his mind and code, but not a bug in module which generate JSON. Using helpers (or specific delimiters in html template language) for escaping is the right way to embed anything into html.

But these helpers should be ready to get both escaped and not escaped slash in input (because escaping them is optional and module which generate JSON may work in both ways) and avoid double-escaping them.

Moreover, when we talking about embedding data into Javascript inside html we have two different things to escape, and they should be escaped in different ways: complex data structures mentioned above, but sometimes user need to inject only one scalar value (string, numbers isn't an issue).

Here is example of implementation for both helpers from Text::MiniTmpl:

=item encode_js( $scalar )

Encode $scalar (string or number) for inserting into JavaScript code
(usually inside HTML templates).

Example:

    <script>
    var int_from_perl =  #~ encode_js($number) ~#;
    var str_from_perl = '#~ encode_js($string) ~#';
    </script>

Return encoded string.

=cut

sub encode_js :Export {
    my ($s) = @_;
    $s = quotemeta $s;
    $s =~ s/\n/n/xmsg;
    return $s;
}

=item encode_js_data( $complex )

Encode $complex data structure (HASHREF, ARRAYREF, etc. - any data type
supported by JSON::XS) for inserting into JavaScript code (usually inside
HTML templates).

Example:

    <script>
    var hash_from_perl  = #~ encode_js_data( \%hash ) ~#;
    var array_from_perl = #~ encode_js_data( \@array ) ~#;
    </script>

Return encoded string.

=cut

sub encode_js_data :Export {
    my ($s) = @_;
    $s = encode_json($s);
    $s =~ s{</script}{<\\/script}xmsg;
    return $s;
}

Bottomline: if we anyway need helpers for escaping, then it doesn't makes much sense to make generated json larger and uglier by escaping optional things like slashes.

[kraih]

@powerman You've had over a month to comment on this issue, now it's too late. If you want to propose another change, please open a new issue, so there can be a new vote.

[powerman]

@kraih I didn't monitor all changes on github, I've just noticed this when you post about it in maillist. Anyway, it's not "too late" because I didn't ask to undo this commit, spec allow to escape slashes so it doesn't harm to do this. I was talking about needs to provide helpers for doing right escaping, no matter is current Mojo::JSON output safe to embed into html or not (both because in general JSON isn't safe to embed into html and because people use alternative implementations like JSON::XS to replace Mojo::JSON and this - I hope - is supported use case). But you right, this is probably should be discussed in separate issue.

@shadowcat-mst I'm sorry to harm you so much with my code! I hope you're well now. BTW, these attributes are from Perl6::Export::Atts, developed by Damian Conway and recommended to use in his "Perl Best Practices" book… so while you probably right in general, I think you slightly overreact in this case.