Limit Mojo messages recursion depth

Limit Mojo messages recursion depth

This prevents a malicious sender from sending deeply nested messages
that cause the receiver to blow the stack during message validation.

BUG=640298
Committed: https://crrev.com/fe0adee770041fc6ab06582579c16e165346f22a
Cr-Commit-Position: refs/heads/master@{#417151}

[tibell, 2016-09-06 04:07:58]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 04:08:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-09-06 04:08:36]

tibell@chromium.org changed reviewers:
+ dcheng@chromium.org, yzshen@chromium.org

[tibell, 2016-09-06 04:48:08]

Rename test interface for clarity

[tibell, 2016-09-06 04:48:13]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 04:48:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 05:43:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 05:43:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tibell, 2016-09-06 07:04:03]

Correctly decrement the recursion limit count

[tibell, 2016-09-06 07:04:08]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 07:04:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-09-06 07:23:15]

Update comment to match arg name change

[tibell, 2016-09-06 07:23:19]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 07:23:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 08:35:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 08:35:07]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-09-06 17:03:10]

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/interfaces/...
File
mojo/public/interfaces/bindings/tests/data/validation/req_max_recursion_depth.data
(right):

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/req_max_recursion_depth.data:21:
[dist4]struct_a2  // num_bytes
style nit: please align the comments.

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/interfaces/...
File mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom
(right):

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom:95:
interface RecursionDepthTestInterface {
I think it may make sense to add it to ConformanceTestInterface. And we need to
make JavaScript and Java validation pass this test, too.

WDYT?

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl
(right):

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:8:
mojo::internal::ValidationContext::ScopedDepthTracker depth_tracker(
This is not the only place: we need to consider things like:

EXAMPLE:
Union A {
  B b;
};

Struct B {
  A a;
};

EXAMPLE 2:
Struct A {
  array<array<array<A>>> a;
};

This will result in a 4-time deeper stack than we expect.


I think the best place to insert these checks is probably in validation_util.h:
- ValidateContainer
- ValidateStruct
- Validate{Inlined, NonInlined}Union

What do you think?

[dcheng, 2016-09-06 21:37:03]

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_context.h (right):

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_context.h:15: static const int
kMaxRecursionDepth = 100;
I'd suggest a more conservative limit. Maybe 50, since that's what the current
ParamTraits for base::Value limits it to.

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_context.h:93:
ScopedDepthTracker(ValidationContext* ctx) : ctx_(ctx) {
Nit: explicit

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_context.h:104:
DISALLOW_COPY_AND_ASSIGN(ScopedDepthTracker);
Nit: include base/compiler_specific.h

[tibell, 2016-09-07 05:40:38]

Address review comments

[tibell, 2016-09-07 05:40:46]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 05:40:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-09-07 05:41:21]

PTAL

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_context.h (right):

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_context.h:15: static const int
kMaxRecursionDepth = 100;
On 2016/09/06 21:37:03, dcheng wrote:
> I'd suggest a more conservative limit. Maybe 50, since that's what the current
> ParamTraits for base::Value limits it to.

I took this from the old IPC limit at
https://cs.chromium.org/chromium/src/ipc/ipc_message_utils.cc?q=kMaxRecursion...
in order to not have it get in the way of some conversion.

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_context.h:93:
ScopedDepthTracker(ValidationContext* ctx) : ctx_(ctx) {
On 2016/09/06 21:37:02, dcheng wrote:
> Nit: explicit

Done.

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_context.h:104:
DISALLOW_COPY_AND_ASSIGN(ScopedDepthTracker);
On 2016/09/06 21:37:02, dcheng wrote:
> Nit: include base/compiler_specific.h

Done.

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/interfaces/...
File
mojo/public/interfaces/bindings/tests/data/validation/req_max_recursion_depth.data
(right):

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/req_max_recursion_depth.data:21:
[dist4]struct_a2  // num_bytes
On 2016/09/06 17:03:10, yzshen1 wrote:
> style nit: please align the comments.

Done.

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/interfaces/...
File mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom
(right):

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom:95:
interface RecursionDepthTestInterface {
On 2016/09/06 17:03:10, yzshen1 wrote:
> I think it may make sense to add it to ConformanceTestInterface. And we need
to
> make JavaScript and Java validation pass this test, too.
> 
> WDYT? 

Moved it into ConformanceTestInterface.

I agree with should look at Java and JS too. I'd prefer to do that in a separate
CL.

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl
(right):

https://codereview.chromium.org/2312813002/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_definition.tmpl:8:
mojo::internal::ValidationContext::ScopedDepthTracker depth_tracker(
On 2016/09/06 17:03:10, yzshen1 wrote:
> This is not the only place: we need to consider things like:
> 
> EXAMPLE:
> Union A {
>   B b;
> };
> 
> Struct B {
>   A a;
> };
> 
> EXAMPLE 2:
> Struct A {
>   array<array<array<A>>> a;
> };
> 
> This will result in a 4-time deeper stack than we expect.
> 
> 
> I think the best place to insert these checks is probably in
validation_util.h:
> - ValidateContainer
> - ValidateStruct
> - Validate{Inlined, NonInlined}Union
> 
> What do you think?

Done.

I was aware of this. The reason I didn't do it for unions etc is that an
attacker can only increase the recursion depth by a small constant factor,
dependent on the message definition rather than the runtime message, even if we
don't check unions/arrays/maps (unless unions can be recursive?). The limit
(100) times the stack frame size is still quite small compare to e.g. a O(1MB)
total stack size limit.

In either case, it's cleaner to do it everywhere and the functions you listed
above gives us a nice place to do so.

[commit-bot: I haz the power, 2016-09-07 06:56:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 06:56:31]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-09-07 16:51:20]

One more comment. Thanks!

https://codereview.chromium.org/2312813002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/validation_unittest.cc (right):

https://codereview.chromium.org/2312813002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/validation_unittest.cc:436:
RunValidationTests("req_max_recursion_depth", &validators);
Does it make sense to use the same prefix as other conformance tests (So it will
be handled by line 383 above), and then change the validation tests of JS/Java
to skip this test case and add TODOs?

[dcheng, 2016-09-07 22:20:51]

I'm not super familiar with the mojo guts, but at a high-level, this lgtm from a
security perspective.

[yzshen1, 2016-09-07 22:52:57]

On 2016/09/07 16:51:20, yzshen1 wrote:
> One more comment. Thanks!
> 
>
https://codereview.chromium.org/2312813002/diff/80001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/tests/validation_unittest.cc (right):
> 
>
https://codereview.chromium.org/2312813002/diff/80001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/tests/validation_unittest.cc:436:
> RunValidationTests("req_max_recursion_depth", &validators);
> Does it make sense to use the same prefix as other conformance tests (So it
will
> be handled by line 383 above), and then change the validation tests of JS/Java
> to skip this test case and add TODOs?

LGTM with the one comment above.

[tibell, 2016-09-08 00:03:32]

Address more review comments

[tibell, 2016-09-08 00:03:38]

https://codereview.chromium.org/2312813002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/validation_unittest.cc (right):

https://codereview.chromium.org/2312813002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/validation_unittest.cc:436:
RunValidationTests("req_max_recursion_depth", &validators);
On 2016/09/07 16:51:20, yzshen1 wrote:
> Does it make sense to use the same prefix as other conformance tests (So it
will
> be handled by line 383 above), and then change the validation tests of JS/Java
> to skip this test case and add TODOs?

Done.

[tibell, 2016-09-08 00:03:38]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 00:03:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 00:07:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 00:07:15]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tibell, 2016-09-08 00:27:24]

The CQ bit was checked by tibell@chromium.org

[tibell, 2016-09-08 00:27:25]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2312813002/#ps100001
(title: "Address more review comments")

[commit-bot: I haz the power, 2016-09-08 00:27:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 01:36:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-08 01:39:44]

Description was changed from

==========
Limit Mojo messages recursion depth

This prevents a malicious sender from sending deeply nested messages
that cause the receiver to blow the stack during message validation.

BUG=640298
==========

to

==========
Limit Mojo messages recursion depth

This prevents a malicious sender from sending deeply nested messages
that cause the receiver to blow the stack during message validation.

BUG=640298

Committed: https://crrev.com/fe0adee770041fc6ab06582579c16e165346f22a
Cr-Commit-Position: refs/heads/master@{#417151}
==========

[commit-bot: I haz the power, 2016-09-08 01:39:45]

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