Add a JSON sanitizer.

Add a JSON sanitizer.

On Android, the JSON sanitizer parses the untrusted JSON in Java and checks it for wellformedness before serializing it again. On desktop platforms it uses SafeJsonParser to parse the JSON out of process, then serializes it again.

Also, on Android (where JsonSanitizer is not implemented in terms of the SafeJsonParser) the SafeJsonParser now uses the JsonSanitizer to sanitize its input before parsing it directly in process, instead of starting a new process, which is very costly on Android.

BUG=501333
Committed: https://crrev.com/b1704d0c90a6eb1e4d5eb1e83976e5964ee378b7
Cr-Commit-Position: refs/heads/master@{#338495}

[Bernhard Bauer, 2015-07-07 12:12:16]

bauerb@chromium.org changed reviewers:
+ rsesek@chromium.org

[Bernhard Bauer, 2015-07-07 12:12:18]

Please review.

This CL looks pretty big, but a lot of it is boilerplate code (build files, JNI
registration). The interesting files are JsonSanitizer.java and
json_sanitizer_android.cc.

Thanks!

[Robert Sesek, 2015-07-07 21:54:16]

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/a...
File components/safe_json/android/component_jni_registrar.h (right):

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/a...
components/safe_json/android/component_jni_registrar.h:11: 
nit: don't need blank lines between non-anonymous nested namespaces

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/a...
File
components/safe_json/android/java/src/org/chromium/components/safejson/JsonSanitizer.java
(right):

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/a...
components/safe_json/android/java/src/org/chromium/components/safejson/JsonSanitizer.java:81:
writer.value(Double.parseDouble(value));
Can this throw as well?

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/a...
components/safe_json/android/java/src/org/chromium/components/safejson/JsonSanitizer.java:142:
if (Character.isLowSurrogate(c)) return false;
Is this one-line if stylelistic? It seems terribly unreadable coming from C++.

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/a...
components/safe_json/android/java/src/org/chromium/components/safejson/JsonSanitizer.java:142:
if (Character.isLowSurrogate(c)) return false;
This code is security critical. Please add some documentation about the order of
these checks.

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
File components/safe_json/json_sanitizer.h (right):

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
components/safe_json/json_sanitizer.h:26: class JsonSanitizer {
Does this class need a Create/Start split, or can it have the same interface as
SafeJsonParser, with just one static Sanitize method?

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
File components/safe_json/json_sanitizer_android.cc (right):

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
components/safe_json/json_sanitizer_android.cc:22: // 1. The input string is
checked for whether it is well-formed UTF-8. Malformed
Is this step required, or does (2) have unexpected results if the input is
invalid UTF-8?

[Bernhard Bauer, 2015-07-08 11:54:29]

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/a...
File components/safe_json/android/component_jni_registrar.h (right):

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/a...
components/safe_json/android/component_jni_registrar.h:11: 
On 2015/07/07 21:54:15, Robert Sesek wrote:
> nit: don't need blank lines between non-anonymous nested namespaces

Done.

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/a...
File
components/safe_json/android/java/src/org/chromium/components/safejson/JsonSanitizer.java
(right):

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/a...
components/safe_json/android/java/src/org/chromium/components/safejson/JsonSanitizer.java:81:
writer.value(Double.parseDouble(value));
On 2015/07/07 21:54:16, Robert Sesek wrote:
> Can this throw as well?

It shouldn't, because if the value is not an integer or floating-point number,
we shouldn't have a NUMBER token here. There might be edge cases, but those will
cause an exception, so we'll hopefully learn about them.

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/a...
components/safe_json/android/java/src/org/chromium/components/safejson/JsonSanitizer.java:142:
if (Character.isLowSurrogate(c)) return false;
On 2015/07/07 21:54:16, Robert Sesek wrote:
> Is this one-line if stylelistic? It seems terribly unreadable coming from C++.

I know! It does look weird if you're used to C++ style, but this is definitely
in accordance with the style guide.

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/a...
components/safe_json/android/java/src/org/chromium/components/safejson/JsonSanitizer.java:142:
if (Character.isLowSurrogate(c)) return false;
On 2015/07/07 21:54:16, Robert Sesek wrote:
> This code is security critical. Please add some documentation about the order
of
> these checks.

Done. Also reordered the checks slightly so they're hopefully more readable.

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
File components/safe_json/json_sanitizer.h (right):

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
components/safe_json/json_sanitizer.h:26: class JsonSanitizer {
On 2015/07/07 21:54:16, Robert Sesek wrote:
> Does this class need a Create/Start split, or can it have the same interface
as
> SafeJsonParser, with just one static Sanitize method?

As it is right now, Create() returns a scoped_ptr which allows the caller to
destroy the object. I could change the whole interface to be fire-and-forget
like SafeJsonParser, but I like having control over the lifetime of the involved
objects. (I would have changed the interface for SafeJsonParser to allow clients
to own it, but that would have been more complicated because of the existing
clients.) But if you feel strongly, I can change it.

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
File components/safe_json/json_sanitizer_android.cc (right):

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
components/safe_json/json_sanitizer_android.cc:22: // 1. The input string is
checked for whether it is well-formed UTF-8. Malformed
On 2015/07/07 21:54:16, Robert Sesek wrote:
> Is this step required, or does (2) have unexpected results if the input is
> invalid UTF-8?

Step 2 will replace any invalid characters or multi-byte sequences with the
replacement character, whereas the JSON parser will outright reject it, so this
makes sure both implementations are equivalent.

[Robert Sesek, 2015-07-08 19:24:07]

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
File components/safe_json/json_sanitizer.h (right):

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
components/safe_json/json_sanitizer.h:26: class JsonSanitizer {
On 2015/07/08 11:54:29, Bernhard Bauer wrote:
> On 2015/07/07 21:54:16, Robert Sesek wrote:
> > Does this class need a Create/Start split, or can it have the same interface
> as
> > SafeJsonParser, with just one static Sanitize method?
> 
> As it is right now, Create() returns a scoped_ptr which allows the caller to
> destroy the object. I could change the whole interface to be fire-and-forget
> like SafeJsonParser, but I like having control over the lifetime of the
involved
> objects. (I would have changed the interface for SafeJsonParser to allow
clients
> to own it, but that would have been more complicated because of the existing
> clients.) But if you feel strongly, I can change it.

What is the use for controlling this pointer lifetime? Would clients call
Start() multiple times on the same object with different data? If so, then maybe
Start() isn't the best name since it sounds one-off. If not, then I think
mirroring the interface of SafeJsonParser is best.

[Bernhard Bauer, 2015-07-08 21:56:48]

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
File components/safe_json/json_sanitizer.h (right):

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
components/safe_json/json_sanitizer.h:26: class JsonSanitizer {
On 2015/07/08 19:24:07, Robert Sesek wrote:
> On 2015/07/08 11:54:29, Bernhard Bauer wrote:
> > On 2015/07/07 21:54:16, Robert Sesek wrote:
> > > Does this class need a Create/Start split, or can it have the same
interface
> > as
> > > SafeJsonParser, with just one static Sanitize method?
> > 
> > As it is right now, Create() returns a scoped_ptr which allows the caller to
> > destroy the object. I could change the whole interface to be fire-and-forget
> > like SafeJsonParser, but I like having control over the lifetime of the
> involved
> > objects. (I would have changed the interface for SafeJsonParser to allow
> clients
> > to own it, but that would have been more complicated because of the existing
> > clients.) But if you feel strongly, I can change it.
> 
> What is the use for controlling this pointer lifetime? Would clients call
> Start() multiple times on the same object with different data? If so, then
maybe
> Start() isn't the best name since it sounds one-off. If not, then I think
> mirroring the interface of SafeJsonParser is best.

The use is... it's easier to reason about the lifetimes of objects if they don't
self-destruct. It is in fact possible to call Start() multiple times with
different data, but I don't have any plans right now to actually do that. I
renamed the method to Sanitize() for now; let me know if I should make it
fire-and-forget.

[Robert Sesek, 2015-07-08 22:00:44]

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
File components/safe_json/json_sanitizer.h (right):

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
components/safe_json/json_sanitizer.h:26: class JsonSanitizer {
On 2015/07/08 21:56:47, Bernhard Bauer wrote:
> On 2015/07/08 19:24:07, Robert Sesek wrote:
> > On 2015/07/08 11:54:29, Bernhard Bauer wrote:
> > > On 2015/07/07 21:54:16, Robert Sesek wrote:
> > > > Does this class need a Create/Start split, or can it have the same
> interface
> > > as
> > > > SafeJsonParser, with just one static Sanitize method?
> > > 
> > > As it is right now, Create() returns a scoped_ptr which allows the caller
to
> > > destroy the object. I could change the whole interface to be
fire-and-forget
> > > like SafeJsonParser, but I like having control over the lifetime of the
> > involved
> > > objects. (I would have changed the interface for SafeJsonParser to allow
> > clients
> > > to own it, but that would have been more complicated because of the
existing
> > > clients.) But if you feel strongly, I can change it.
> > 
> > What is the use for controlling this pointer lifetime? Would clients call
> > Start() multiple times on the same object with different data? If so, then
> maybe
> > Start() isn't the best name since it sounds one-off. If not, then I think
> > mirroring the interface of SafeJsonParser is best.
> 
> The use is... it's easier to reason about the lifetimes of objects if they
don't
> self-destruct. It is in fact possible to call Start() multiple times with
> different data, but I don't have any plans right now to actually do that. I
> renamed the method to Sanitize() for now; let me know if I should make it
> fire-and-forget.

Unless there were a reason for multiple Sanitize() calls, I think it should just
be a static. That matches the SafeJsonParser, and then there's no object (for
the caller) to even reason about.

[Bernhard Bauer, 2015-07-09 12:34:49]

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
File components/safe_json/json_sanitizer.h (right):

https://codereview.chromium.org/1203083002/diff/350001/components/safe_json/j...
components/safe_json/json_sanitizer.h:26: class JsonSanitizer {
On 2015/07/08 22:00:43, Robert Sesek wrote:
> On 2015/07/08 21:56:47, Bernhard Bauer wrote:
> > On 2015/07/08 19:24:07, Robert Sesek wrote:
> > > On 2015/07/08 11:54:29, Bernhard Bauer wrote:
> > > > On 2015/07/07 21:54:16, Robert Sesek wrote:
> > > > > Does this class need a Create/Start split, or can it have the same
> > interface
> > > > as
> > > > > SafeJsonParser, with just one static Sanitize method?
> > > > 
> > > > As it is right now, Create() returns a scoped_ptr which allows the
caller
> to
> > > > destroy the object. I could change the whole interface to be
> fire-and-forget
> > > > like SafeJsonParser, but I like having control over the lifetime of the
> > > involved
> > > > objects. (I would have changed the interface for SafeJsonParser to allow
> > > clients
> > > > to own it, but that would have been more complicated because of the
> existing
> > > > clients.) But if you feel strongly, I can change it.
> > > 
> > > What is the use for controlling this pointer lifetime? Would clients call
> > > Start() multiple times on the same object with different data? If so, then
> > maybe
> > > Start() isn't the best name since it sounds one-off. If not, then I think
> > > mirroring the interface of SafeJsonParser is best.
> > 
> > The use is... it's easier to reason about the lifetimes of objects if they
> don't
> > self-destruct. It is in fact possible to call Start() multiple times with
> > different data, but I don't have any plans right now to actually do that. I
> > renamed the method to Sanitize() for now; let me know if I should make it
> > fire-and-forget.
> 
> Unless there were a reason for multiple Sanitize() calls, I think it should
just
> be a static. That matches the SafeJsonParser, and then there's no object (for
> the caller) to even reason about.

OK, done.

[Robert Sesek, 2015-07-09 15:04:14]

LGTM

[Bernhard Bauer, 2015-07-09 15:19:12]

bauerb@chromium.org changed reviewers:
+ sdefresne@chromium.org, thakis@chromium.org

[Bernhard Bauer, 2015-07-09 15:19:14]

Adding OWNERS (Nico for chrome/, Sylvain for components).

[sdefresne, 2015-07-10 14:01:19]

lgtm

https://codereview.chromium.org/1203083002/diff/450001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1203083002/diff/450001/components/BUILD.gn#ne...
components/BUILD.gn:352: "//components/safe_json:unit_tests",
I don't understand this addition. At line 298, you're adding
"//components/safe_json:unit_tests" to the list of deps, so there is no need to
add it again here when target is not iOS. Either remove it from here or from
line 298 (if the tests should not be run on iOS). Since safe_json depends on
//content, I would guess you should do the later.

https://codereview.chromium.org/1203083002/diff/450001/components/safe_json/DEPS
File components/safe_json/DEPS (right):

https://codereview.chromium.org/1203083002/diff/450001/components/safe_json/D...
components/safe_json/DEPS:5: "+grit/components_strings.h",  # For generated
headers
nit: this line is redundant with the previous one

[Bernhard Bauer, 2015-07-10 14:09:09]

Thanks for the review!

https://codereview.chromium.org/1203083002/diff/450001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1203083002/diff/450001/components/BUILD.gn#ne...
components/BUILD.gn:352: "//components/safe_json:unit_tests",
On 2015/07/10 14:01:19, sdefresne wrote:
> I don't understand this addition. At line 298, you're adding
> "//components/safe_json:unit_tests" to the list of deps, so there is no need
to
> add it again here when target is not iOS. Either remove it from here or from
> line 298 (if the tests should not be run on iOS). Since safe_json depends on
> //content, I would guess you should do the later.

You're right, the tests shouldn't be run on iOS. Removed the line above.

https://codereview.chromium.org/1203083002/diff/450001/components/safe_json/DEPS
File components/safe_json/DEPS (right):

https://codereview.chromium.org/1203083002/diff/450001/components/safe_json/D...
components/safe_json/DEPS:5: "+grit/components_strings.h",  # For generated
headers
On 2015/07/10 14:01:19, sdefresne wrote:
> nit: this line is redundant with the previous one

Removed.

[Bernhard Bauer, 2015-07-13 09:13:24]

bauerb@chromium.org changed reviewers:
+ jochen@chromium.org
- thakis@chromium.org

[Bernhard Bauer, 2015-07-13 09:13:25]

Jochen, could I get an OWNERS review for chrome/test/base/chrome_test_suite.cc
and chrome/chrome.gyp?

[jochen (gone - plz use gerrit), 2015-07-13 09:16:17]

lgtm with comment

https://codereview.chromium.org/1203083002/diff/470001/components/safe_json/DEPS
File components/safe_json/DEPS (right):

https://codereview.chromium.org/1203083002/diff/470001/components/safe_json/D...
components/safe_json/DEPS:3: "+content/public",
please restrict to c/p/browser

[Bernhard Bauer, 2015-07-13 09:41:40]

https://codereview.chromium.org/1203083002/diff/470001/components/safe_json/DEPS
File components/safe_json/DEPS (right):

https://codereview.chromium.org/1203083002/diff/470001/components/safe_json/D...
components/safe_json/DEPS:3: "+content/public",
On 2015/07/13 09:16:17, jochen wrote:
> please restrict to c/p/browser

components/safe_json/safe_json_parser_messages.h needs c/p/common. Is it okay if
I just list those two?

[Bernhard Bauer, 2015-07-13 09:43:25]

https://codereview.chromium.org/1203083002/diff/470001/components/safe_json/DEPS
File components/safe_json/DEPS (right):

https://codereview.chromium.org/1203083002/diff/470001/components/safe_json/D...
components/safe_json/DEPS:3: "+content/public",
On 2015/07/13 09:41:40, Bernhard Bauer wrote:
> On 2015/07/13 09:16:17, jochen wrote:
> > please restrict to c/p/browser
> 
> components/safe_json/safe_json_parser_messages.h needs c/p/common. Is it okay
if
> I just list those two?

Oh, and SafeJsonParserMessageFilter needs c/p/utility :-/

[jochen (gone - plz use gerrit), 2015-07-13 09:44:38]

On 2015/07/13 at 09:43:25, bauerb wrote:
>
https://codereview.chromium.org/1203083002/diff/470001/components/safe_json/DEPS
> File components/safe_json/DEPS (right):
> 
>
https://codereview.chromium.org/1203083002/diff/470001/components/safe_json/D...
> components/safe_json/DEPS:3: "+content/public",
> On 2015/07/13 09:41:40, Bernhard Bauer wrote:
> > On 2015/07/13 09:16:17, jochen wrote:
> > > please restrict to c/p/browser
> > 
> > components/safe_json/safe_json_parser_messages.h needs c/p/common. Is it
okay if
> > I just list those two?
> 
> Oh, and SafeJsonParserMessageFilter needs c/p/utility :-/

then that's missing from the gn/gyp files as well, no?

but yes, list all you need (because you don't want to accidentally include
c/p/renderer)

[Bernhard Bauer, 2015-07-13 09:55:11]

On 2015/07/13 09:44:38, jochen wrote:
> On 2015/07/13 at 09:43:25, bauerb wrote:
> >
>
https://codereview.chromium.org/1203083002/diff/470001/components/safe_json/DEPS
> > File components/safe_json/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/1203083002/diff/470001/components/safe_json/D...
> > components/safe_json/DEPS:3: "+content/public",
> > On 2015/07/13 09:41:40, Bernhard Bauer wrote:
> > > On 2015/07/13 09:16:17, jochen wrote:
> > > > please restrict to c/p/browser
> > > 
> > > components/safe_json/safe_json_parser_messages.h needs c/p/common. Is it
> okay if
> > > I just list those two?
> > 
> > Oh, and SafeJsonParserMessageFilter needs c/p/utility :-/
> 
> then that's missing from the gn/gyp files as well, no?

Hm. It definitely works like this (presumably because the dependency is added
when linking later), so I guess the question is, are BUILD files supposed to
list all dependencies, or just the ones that are immediately necessary? If the
former, it seems a bit redundant with DEPS files.

> but yes, list all you need (because you don't want to accidentally include
> c/p/renderer)

Meh, I'm pretty sure I'd get a linking error in that case, but yeah, listing all
the deps makes sense.

[Bernhard Bauer, 2015-07-13 10:56:16]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2015-07-13 10:56:16]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
sdefresne@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1203083002/#ps530001
(title: "build files")

[commit-bot: I haz the power, 2015-07-13 10:56:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1203083002/530001

[commit-bot: I haz the power, 2015-07-13 11:05:53]

Committed patchset #28 (id:530001)

[commit-bot: I haz the power, 2015-07-13 11:06:49]

Patchset 28 (id:??) landed as
https://crrev.com/b1704d0c90a6eb1e4d5eb1e83976e5964ee378b7
Cr-Commit-Position: refs/heads/master@{#338495}