Improve JsonDoubleQuoteT to do script escapes

Improve the underlying escaping function JsonDoubleQuoteT to escape < and > characters BY DEFAULT to prevent script execution.
BUG=40147
TEST=StringEscapeTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=43695

[inferno, 2010-04-02 03:47:52]

simple fix, but took a little more time to come up with a nice presentable
solution :) user will see no differences.

testcase
1.
ftp://alt.swiecki.net/%3c/script%3e%3cimg%20src=p%20onerror=alert%28document.domain+document.cookie%29%3e/
(title xss)
2. ftp://alt.swiecki.net/ (name xss probably)

i made sure that all instances of JsonDoubleQuote use encoded input so that
noone can come out of script tags.

[inferno, 2010-04-02 04:58:27]

added a unit test case. fixed existing ones. changed from escapepath to
urlescape since it handles more chars (although anyone would be fine).

[inferno, 2010-04-02 05:07:22]

url escape also escaped + which we dont want as it is not unescaped by unescape
function. that is what i get for thinking too much. coming back to escapepath.

[jschuh, 2010-04-02 17:53:23]

SGTM, but I'm not familiar with this code. Chris, you mind singing off before
commit?

[cevans, 2010-04-02 17:55:46]

On Fri, Apr 2, 2010 at 10:53 AM, <jschuh@chromium.org> wrote:

> SGTM, but I'm not familiar with this code. Chris, you mind singing off
> before
> commit?


I'd prefer someone on the network stack took a quick look. Suggest
eroman@as particularly helpful.


>
>
> http://codereview.chromium.org/1512013
>

[inferno, 2010-04-02 17:57:00]

Including Eric for review.

[eroman, 2010-04-03 00:50:24]

I think it would be better to push the fix into the base library function
JsonDoubleQuote, rather than doing a on-off for the FTP case.

How about changing JsonDoubleQuote() so that it replaces:

"<"    with   "\u003c"

and

">"   with "\u003e"


This will avoid other consumers from hitting this problem, and at the same time
simplify the change, since you don't need to path escape/unescape throughout the
directory lister.

[inferno, 2010-04-03 01:18:36]

That is a great suggestion Eric. I will work on doing that.

[inferno, 2010-04-05 03:32:22]

Hi Eric,

I have implemented the suggested solution. Please review.

1. added a boolean flag in underlying hidden template JsonDoubleQuoteT function
to tell if script escaping is required.
2. Did not modify JsonDoubleQuote Function since it is used in generic function
- jsonwriter. i checked json spec http://www.json.org/ and it does not say about
escaping <, >. so we cannot break the spec by doing for default.
3. i checked use of JsonDoubleQuote function in chrome source. all of its use in
net_util.cc for ftp. I changed its name with JsonDoubleQuoteScriptEscape to make
sure script chars are escaped.
4. Added unittest to verify behavior of JsonDoubleQuoteScriptEscape.
5. on a unrelated note, there was an xss that used unsanitized document.location
input into .innerHTML. i corrected that by using encodeuri func.

[cevans, 2010-04-05 04:30:57]

On Sun, Apr 4, 2010 at 8:32 PM, <inferno@chromium.org> wrote:

> Hi Eric,
>
> I have implemented the suggested solution. Please review.
>
> 1. added a boolean flag in underlying hidden template JsonDoubleQuoteT
> function
> to tell if script escaping is required.
> 2. Did not modify JsonDoubleQuote Function since it is used in generic
> function
> - jsonwriter. i checked json spec http://www.json.org/ and it does not say
> about
> escaping <, >. so we cannot break the spec by doing for default.
>

FWIW: we changed some of our low-level google3 JSON outputting routines to
avoid XSS via an unescaped </script> within a Javascript string.

I seem to remember a couple of complaints about a moderately common JSON
parsing library not liking what we did, but it was pretty quickly fixed.

Would it be possible to adopt a more security-conservative approach and
escape < and > in JSON strings by default? It would defend against
accidental incorrect API usage in the future.
In terms of risk:
- All common JSON reading libraries should handle escaping.
- Do we expose JSON as an external-to-Chromium interface anywhere, anyway?
We serialize to and from the filesystem, or use the escaping functions to
actually escape Javascript strings, but I've not seen much else?


Cheers
Chris

3. i checked use of JsonDoubleQuote function in chrome source. all of its
> use in
> net_util.cc for ftp. I changed its name with JsonDoubleQuoteScriptEscape to
> make
> sure script chars are escaped.
> 4. Added unittest to verify behavior of JsonDoubleQuoteScriptEscape.
> 5. on a unrelated note, there was an xss that used unsanitized
> document.location
> input into .innerHTML. i corrected that by using encodeuri func.
>
>
> http://codereview.chromium.org/1512013
>

[Peter Kasting, 2010-04-05 04:48:49]

On 2010/04/05 04:30:57, cevans_google.com wrote:
> - Do we expose JSON as an external-to-Chromium interface anywhere, anyway?
> We serialize to and from the filesystem, or use the escaping functions to
> actually escape Javascript strings, but I've not seen much else?

We read JSON off the wire for some things, e.g. search provider suggestions (can
come from any search provider).  I dunno about writing, though.

[Peter Kasting, 2010-04-05 04:50:12]

On 2010/04/05 03:32:22, inferno wrote:
> 2. Did not modify JsonDoubleQuote Function since it is used in generic
function
> - jsonwriter. i checked json spec http://www.json.org/ and it does not say
about
> escaping <, >. so we cannot break the spec by doing for default.

I think we should escape in JsonDoubleQuote.  I don't think this will cause us
any problems.

[inferno, 2010-04-05 05:25:22]

Thanks a lot Chris and Peter.

I have now enabled Script escaping < > for JsonDoubleQuoteT by default and also
corrected/added some unittests. let me know if it looks ok to you.

btw, what are your thoughts for merging this to 249. i think it will be risky,
so we should just stick to v5 trunk for this.

[eroman, 2010-04-06 01:43:11]

lgtm.

I can't imagine this causing any problems... but if it does, then that is is
good since it has discovered broken code! (and we can address it once
discovered).

http://codereview.chromium.org/1512013/diff/29003/25007
File base/json/string_escape.cc (right):

http://codereview.chromium.org/1512013/diff/29003/25007#newcode61
base/json/string_escape.cc:61: if (c < 32 || c > 126 || c == 60 || c == 62) {
nit: can you '<' instead of 60, and '>' instead of 62 ?

http://codereview.chromium.org/1512013/diff/29003/25009
File net/base/dir_header.html (right):

http://codereview.chromium.org/1512013/diff/29003/25009#newcode63
net/base/dir_header.html:63: box.innerHTML = box.innerHTML.replace("LOCATION",
encodeURI(document.location)
nit: this seems like a separate issue, I suggest breaking it out into a
different CL.

[inferno, 2010-04-06 03:45:10]

Thanks Eric. i did as you recommended and committed in two seperate cls.