Basic Mojo message header validation for JavaScript.

Basic Mojo message header validation for JavaScript.

This is the the second part of the original CL for "Validate incoming JS Message Headers", per https://codereview.chromium.org/406993002#msg2. The first part is https://codereview.chromium.org/411553003.

TBR=jochen@chromium.org
BUG=395801
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286819

[hansmuller, 2014-07-28 21:07:35]

This CL reintroduces the second part of 
https://codereview.chromium.org/406993002/

[Matt Perry, 2014-07-28 23:59:04]

https://codereview.chromium.org/424463003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/424463003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:64: (numFields == 2 && numBytes ==
codec.kMessageHeaderSize) ||
indent += 2

[hansmuller, 2014-07-29 00:10:43]

https://codereview.chromium.org/424463003/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/424463003/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:64: (numFields == 2 && numBytes ==
codec.kMessageHeaderSize) ||
On 2014/07/28 23:59:03, Matt Perry wrote:
> indent += 2

Done.

[yzshen1, 2014-07-29 05:59:53]

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
File mojo/public/js/bindings/codec.js (right):

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/codec.js:363: var kNumBytesOffset = 0;
Maybe we could consider define these in the "constants" section. They are
constants for general struct header, not just for message header.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/codec.js:367: var kFlagsOffset = 4 + 4 + 4;
Some of those 4's (or all of them) could be replaced by constants defined above?

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/codec.js:370: var kRequestIDOffset = 4 + 4 + 4 + 4;
ditto.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:32: Validator.prototype.claimMemory =
function(numBytes) {
The objects are not guaranteed to be stored in the message continuously.
(Alignment requirement; unrecognized region from newer versions, etc.)

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:33: if (numBytes < 0 || this.offset +
numBytes > this.offsetLimit)
Is it possible for the addition to overflow?
If not, is it useful to add a comment explaining that this operation is safe?

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:44: if (offset + codec.kStructHeaderSize >
this.offsetLimit)
This is inadequate: the offset may overlap a previous object, or violate the
depth-first layout order.
Please see the Mojo Archive Format and the Validate Algorithm.

It seems better to make this a separate method, just like claimMemory(). (Or
even group them as a separate class.)

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:53: if (!this.claimMemory(numBytes))
It should claim the specified size starting from |offset| .

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:66: if (!validNumFieldsAndNumBytes)
This is incorrect. In the future, the message header may evolve to have more
fields than 3. An older implementation should ignore fields that it doesn't
understand.

(Please see the validation algorithm.)

[yzshen1, 2014-07-29 06:04:36]

+Tom who is our security reviewer. (Thanks Tom!)

[Tom Sepez, 2014-07-29 16:32:54]

I'll defer to yzshen on the correctness of the algorithm.  We don't have exactly
the same problems as in C, since we don't construct the objects 'in-place',
right?  Plus a few nits.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
File mojo/public/js/bindings/codec.js (right):

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/codec.js:21: return offset >= 0 && (offset % kAlignment)
== 0;
nit: purists might insist on === througout.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
File mojo/public/js/bindings/router.js (right):

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/router.js:65: if (v.validateMessage() !=
validator.ValidationError.NONE)
nit: purists might insist on !==

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:9: var ValidationError = {
nit: In theory, this probably shouldn't be capitalized; its not the name of an
object you can construct with new.  But other chrome code violates this
convention.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:10: NONE: 'VALIDATION_ERROR_NONE',
nit: if you use '' as the value for none (or some other fasly value), you get to
write things later on like:  if (err) { ... } instead of if (err !=
validationError.none) { ... }

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:33: if (numBytes < 0 || this.offset +
numBytes > this.offsetLimit)
> If not, is it useful to add a comment explaining that this operation is safe?
Addition in JS is safe. No need to comment.

[hansmuller, 2014-07-29 19:06:23]

Changes per the review feedback.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
File mojo/public/js/bindings/codec.js (right):

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/codec.js:21: return offset >= 0 && (offset % kAlignment)
== 0;
On 2014/07/29 16:32:54, Tom Sepez wrote:
> nit: purists might insist on === througout.

Done.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/codec.js:363: var kNumBytesOffset = 0;
On 2014/07/29 05:59:53, yzshen1 wrote:
> Maybe we could consider define these in the "constants" section. They are
> constants for general struct header, not just for message header.

Done.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/codec.js:367: var kFlagsOffset = 4 + 4 + 4;
On 2014/07/29 05:59:53, yzshen1 wrote:
> Some of those 4's (or all of them) could be replaced by constants defined
above?

Done.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/codec.js:370: var kRequestIDOffset = 4 + 4 + 4 + 4;
On 2014/07/29 05:59:53, yzshen1 wrote:
> ditto.

Done.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
File mojo/public/js/bindings/router.js (right):

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/router.js:65: if (v.validateMessage() !=
validator.ValidationError.NONE)
On 2014/07/29 16:32:54, Tom Sepez wrote:
> nit: purists might insist on !==

Done.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:9: var ValidationError = {
On 2014/07/29 16:32:54, Tom Sepez wrote:
> nit: In theory, this probably shouldn't be capitalized; its not the name of an
> object you can construct with new.  But other chrome code violates this
> convention.

OK, will use validationError.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:10: NONE: 'VALIDATION_ERROR_NONE',
On 2014/07/29 16:32:54, Tom Sepez wrote:
> nit: if you use '' as the value for none (or some other fasly value), you get
to
> write things later on like:  if (err) { ... } instead of if (err !=
> validationError.none) { ... }

A good suggestion and I tried it but then reverted. I liked the reminder about
the return value that's apparent in:

    if (v.validateMessage() != validator.validationError.NONE)
      this.close();

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:32: Validator.prototype.claimMemory =
function(numBytes) {
On 2014/07/29 05:59:53, yzshen1 wrote:
> The objects are not guaranteed to be stored in the message continuously.
> (Alignment requirement; unrecognized region from newer versions, etc.)

Sorry, about that, I've written this incorrectly. Two parameters are needed: the
offset from which the allocation is to start, and numBytes.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:33: if (numBytes < 0 || this.offset +
numBytes > this.offsetLimit)
On 2014/07/29 05:59:53, yzshen1 wrote:
> Is it possible for the addition to overflow?
> If not, is it useful to add a comment explaining that this operation is safe?

Good point. Only positive JS integers that are less than 2^53
(Number.MAX_SAFE_INTEGER) are represented exactly, so overflow from integer to
floating point is a problem.

I've added checks to verify that the arithmetic doesn't overflow and a comment
explaining as much.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:44: if (offset + codec.kStructHeaderSize >
this.offsetLimit)
On 2014/07/29 05:59:53, yzshen1 wrote:
> This is inadequate: the offset may overlap a previous object, or violate the
> depth-first layout order.
> Please see the Mojo Archive Format and the Validate Algorithm.
> 
> It seems better to make this a separate method, just like claimMemory(). (Or
> even group them as a separate class.)

I've made isValidRange() a separate method and corrected the validity test.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:53: if (!this.claimMemory(numBytes))
On 2014/07/29 05:59:53, yzshen1 wrote:
> It should claim the specified size starting from |offset| .

Yes. I've added a start parameter to claimMemory().

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:66: if (!validNumFieldsAndNumBytes)
On 2014/07/29 05:59:53, yzshen1 wrote:
> This is incorrect. In the future, the message header may evolve to have more
> fields than 3. An older implementation should ignore fields that it doesn't
> understand.
> 
> (Please see the validation algorithm.)

I've added a numFields > 3 case. However:

From "Message Header Validation" (and per our conversation)

...
  else if (header.num_fields > 3) {
    if (header.num_bytes < sizeof(MessageHeaderWithRequestID)) {
      Report(VALIDATION_ERROR_UNEXPECTED_STRUCT_HEADER);
      return false;
    }
  }

If there are more than 3 message header fields, the header's length would have
to be > sizeof(MessageHeaderWithRequestID). In other words, this test should
probably be |header.num_bytes <= sizeof(MessageHeaderWithRequestID)|.

For now I've made the JS version consistent with existing test.

[yzshen1, 2014-07-30 17:14:56]

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:33: if (numBytes < 0 || this.offset +
numBytes > this.offsetLimit)
Does it make sense to add your comment to the code so that it is more obvious
what the code does?
"""Only positive JS integers that are less than 2^53
(Number.MAX_SAFE_INTEGER) are represented exactly, so overflow from integer to
floating point is a problem."""

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:66: if (!validNumFieldsAndNumBytes)
Yes, you are right. In this case, we could tighten the check if we want.

The reason why I am okay keeping it the current way: adding more fields to a
struct could remain the same size, if there is padding and the new fields could
fit into the padding according to our packing algorithm. So here the check is
consistent with the generic header check that we will do for structs in general.
(We haven't done that part because we haven't support backward compatibility.)

https://codereview.chromium.org/424463003/diff/90001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/424463003/diff/90001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:32: // True if we can safely allocate a
block of bytes from offset + start
Do you mean |start| is relative to the current |offset| (because offset + start
is used)? That seems confusing.

It might be easier if |start| is relative to a fixed point, such as the
beginning of the message. What do you think?

https://codereview.chromium.org/424463003/diff/90001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:35: if (start < this.offset || numBytes < 0
||
start < this.offset: it indicates start and offset are relative to the same
origin. That is inconsistent with start + offset, which indicates start is
relative to offset.

numBytes < 0: The documented algorithm regards an empty range as invalid. It is
better to be consistent.

[hansmuller, 2014-07-30 18:32:30]

I've made the suggested corrections.

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/424463003/diff/50001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:33: if (numBytes < 0 || this.offset +
numBytes > this.offsetLimit)
On 2014/07/30 17:14:56, yzshen1 wrote:
> Does it make sense to add your comment to the code so that it is more obvious
> what the code does?
> """Only positive JS integers that are less than 2^53
> (Number.MAX_SAFE_INTEGER) are represented exactly, so overflow from integer to
> floating point is a problem."""
Yes, I'll do that.

https://codereview.chromium.org/424463003/diff/90001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/424463003/diff/90001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:32: // True if we can safely allocate a
block of bytes from offset + start
On 2014/07/30 17:14:56, yzshen1 wrote:
> Do you mean |start| is relative to the current |offset| (because offset +
start
> is used)? That seems confusing.
> 
> It might be easier if |start| is relative to a fixed point, such as the
> beginning of the message. What do you think?

I agree, it should be relative to the beginning of the message, offset = 0.

https://codereview.chromium.org/424463003/diff/90001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:35: if (start < this.offset || numBytes < 0
||
On 2014/07/30 17:14:56, yzshen1 wrote:
> start < this.offset: it indicates start and offset are relative to the same
> origin. That is inconsistent with start + offset, which indicates start is
> relative to offset.

Right, that's my mistake (again). The check just needs to verify that
start+offset first between this.offset and this.offsetLimit. In addition to
guarding against overflow.

> 
> numBytes < 0: The documented algorithm regards an empty range as invalid. It
is
> better to be consistent.

Yes. The check is now numBytes <= 0.

[yzshen1, 2014-07-30 18:36:07]

https://codereview.chromium.org/424463003/diff/110001/mojo/public/js/bindings...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/424463003/diff/110001/mojo/public/js/bindings...
mojo/public/js/bindings/validator.js:51: this.offset += start + numBytes;
This line needs to be changed, too.

[hansmuller, 2014-07-30 19:13:31]

https://codereview.chromium.org/424463003/diff/110001/mojo/public/js/bindings...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/424463003/diff/110001/mojo/public/js/bindings...
mojo/public/js/bindings/validator.js:51: this.offset += start + numBytes;
On 2014/07/30 18:36:07, yzshen1 wrote:
> This line needs to be changed, too.

Yes. Sorry to have missed that.

[yzshen1, 2014-07-30 19:14:11]

LGTM

Thanks!

[hansmuller, 2014-07-30 19:16:12]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-07-30 19:17:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hansmuller@chromium.org/424463003/130001

[hansmuller, 2014-07-30 19:36:34]

jochen - I've updated the following files because I added a public Mojo Java
module (validator.js). The presubmit check says I need an LGTM from
src/content/OWNERS. Can you take a look?

    content/browser/webui/web_ui_data_source_impl.cc
    content/browser/webui/web_ui_mojo_browsertest.cc
    content/content_resources.grd

[hansmuller, 2014-07-30 19:46:21]

asargent, scheib - I've updated the following file because I added a public Mojo
Java module (validator.js). The presubmit check says I need an LGTM from
src/extensions/OWNERS. Can you take a look?

    extensions/renderer/dispatcher.cc

[scheib, 2014-07-30 20:17:26]

extensions/renderer/dispatcher.cc LGTM

[commit-bot: I haz the power, 2014-07-30 21:20:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/35476)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/40507)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-07-30 21:24:17]

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

[commit-bot: I haz the power, 2014-07-30 21:24:18]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hansmuller, 2014-07-31 14:38:11]

On 2014/07/30 19:36:34, hansmuller wrote:
> jochen - I've updated the following files because I added a public Mojo Java
> module (validator.js). The presubmit check says I need an LGTM from
> src/content/OWNERS. Can you take a look?
> 
>     content/browser/webui/web_ui_data_source_impl.cc
>     content/browser/webui/web_ui_mojo_browsertest.cc
>     content/content_resources.grd

I TBR'd you on these changes, since they're essentially the same (just adding a
JS module) as the ones you reviewed for
https://codereview.chromium.org/411553003.

[hansmuller, 2014-07-31 14:38:22]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-07-31 14:39:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hansmuller@chromium.org/424463003/130001

[jochen (gone - plz use gerrit), 2014-07-31 14:50:14]

lgtm

[hansmuller, 2014-07-31 15:08:19]

On 2014/07/31 14:50:14, jochen wrote:
> lgtm

Thanks!

[commit-bot: I haz the power, 2014-07-31 16:44:47]

Change committed as 286819