Add a mechanism to scramble Mojo message IDs.

Add a mechanism to scramble Mojo message IDs.

For example, chrome/common/renderer_configuration.mojom defines:

interface RendererConfiguration {
  SetInitialConfiguration(etc);
  SetContentSettingRules(etc);
  SetFieldTrialGroup(etc);
};

The generated IDs in
gen/chrome/common/renderer_configuration.mojom-shared-internal.h, prior
to this CL, and also after this CL with an empty seed, are:

constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name = 0;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name = 1;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 2;

After this CL, with a "foo" seed, the IDs are:
    
constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name = 1257880741;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name = 631133653;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 549336076;

BUG=673417
Review-Url: https://codereview.chromium.org/2794743002
Cr-Commit-Position: refs/heads/master@{#467573}
Committed: https://chromium.googlesource.com/chromium/src/+/9c12ed217b0f67085d1ec83ab3eabdabe72c40da

[jschuh, 2017-04-02 20:10:44]

jschuh@chromium.org changed reviewers:
+ tsepez@chromium.org

[jschuh, 2017-04-02 20:10:45]

Tom is probably the best person to provide feedback and a review here.

[Tom Sepez, 2017-04-03 16:45:12]

On 2017/04/02 20:10:45, jschuh wrote:
> Tom is probably the best person to provide feedback and a review here.

This would be constant at compile-time for a given release, right?  I thought we
wanted to make these vary at run-time.

[Tom Sepez, 2017-04-04 16:43:13]

Per discussion with jschuh, it's OK to do this at compile time.
However, what does this do to deterministic builds?  Are the folks OK with this?

[nigeltao1, 2017-04-06 01:44:49]

On 2017/04/04 16:43:13, Tom Sepez wrote:
> Per discussion with jschuh, it's OK to do this at compile time.
> However, what does this do to deterministic builds?  Are the folks OK with
this?

If you supply the same seed string, you will get the same scrambled IDs and thus
should get the same build. Note that this work-in-progress CL aims to only
provide mechanism, not policy on how to set or when to change that seed across
Chromium versions.

As for whether folks are OK with this, I'm still a bit of a newbie at navigating
large parts of the Chromium community, and am not well versed with any issues re
determistic builds. Who are the right folks to ask?

[Tom Sepez, 2017-04-06 16:48:03]

Description was changed from

==========
Add a mechanism to scramble Mojo message IDs.

BUG=
==========

to

==========
Add a mechanism to scramble Mojo message IDs.

BUG=
==========

[Tom Sepez, 2017-04-06 16:48:03]

tsepez@chromium.org changed reviewers:
+ maruel@chromium.org

[Tom Sepez, 2017-04-06 16:48:47]

> As for whether folks are OK with this, I'm still a bit of a newbie at
navigating
> large parts of the Chromium community, and am not well versed with any issues
re
> determistic builds. Who are the right folks to ask?

+maruel, who I think did the determinstic build work.

[M-A Ruel, 2017-04-06 22:07:26]

On 2017/04/06 16:48:47, Tom Sepez wrote:
> > As for whether folks are OK with this, I'm still a bit of a newbie at
> navigating
> > large parts of the Chromium community, and am not well versed with any
issues
> re
> > determistic builds. Who are the right folks to ask?
> 
> +maruel, who I think did the determinstic build work.

Thank Tom for the probe, if you use a hash derived from the date produced by
https://cs.chromium.org/chromium/src/build/write_build_date_header.py
as per
https://cs.chromium.org/chromium/src/base/BUILD.gn?l=2361

please don't make it reproducible.

[nigeltao1, 2017-04-07 01:41:12]

On 2017/04/06 22:07:26, M-A Ruel wrote:
> Thank Tom for the probe, if you use a hash derived from the date produced by
> https://cs.chromium.org/chromium/src/build/write_build_date_header.py
> as per
> https://cs.chromium.org/chromium/src/base/BUILD.gn?l=2361
> 
> please don't make it reproducible.

This CL introduces a mechanism to scramble the generated integer IDs for Mojo
messages, so they're not the easily guessable sequence 0, 1, 2, etc. That
mechanism includes a flag to specify a seed string, but this CL does not dictate
a policy for selecting that seed.

tsepez's question, I think, is whether this breaks deterministic builds.

If you supply the same seed string, you will get the same scrambled IDs and thus
should get the same build. This is of course speaking only about the generated
Mojo code. If other inputs to the build vary, such as
gen/base/generated_build_date.h, then obviously you might build a different
binary.

So with the right policy for seed selection, I think we can avoid breaking
deterministic builds, but I'm not that familiar with the issues around
deterministic builds.

Your reply says that if the seed depends on the build date, then please don't
make it reproducible. I'm a little confused here. I thought that reproducibility
was the point of deterministic builds.

Or are you saying that the seed shouldn't depend on the build date. OK, then the
seed shouldn't depend on the build date. I do expect that, for production
builds, the seed will depend on the major version number such as "M57" plus at
least some salt, since the whole point of the exercise is to avoid predictible
IDs over long periods of time. But that number won't change every day.

Are there any other concerns? Or am I misunderstanding your reply?

[M-A Ruel, 2017-04-07 01:45:13]

(Sorry for the initial terse reply, having a sick day)

On 2017/04/07 01:41:12, nigeltao1 wrote:
> On 2017/04/06 22:07:26, M-A Ruel wrote:
> > Thank Tom for the probe, if you use a hash derived from the date produced by
> > https://cs.chromium.org/chromium/src/build/write_build_date_header.py
> > as per
> > https://cs.chromium.org/chromium/src/base/BUILD.gn?l=2361
> > 
> > please don't make it reproducible.

I meant the reverse. Sorry for the confusing statement.

> This CL introduces a mechanism to scramble the generated integer IDs for Mojo
> messages, so they're not the easily guessable sequence 0, 1, 2, etc. That
> mechanism includes a flag to specify a seed string, but this CL does not
dictate
> a policy for selecting that seed.
> 
> tsepez's question, I think, is whether this breaks deterministic builds.
> 
> If you supply the same seed string, you will get the same scrambled IDs and
thus
> should get the same build. This is of course speaking only about the generated
> Mojo code. If other inputs to the build vary, such as
> gen/base/generated_build_date.h, then obviously you might build a different
> binary.
> 
> So with the right policy for seed selection, I think we can avoid breaking
> deterministic builds, but I'm not that familiar with the issues around
> deterministic builds.
> 
> Your reply says that if the seed depends on the build date, then please don't
> make it reproducible. I'm a little confused here. I thought that
reproducibility
> was the point of deterministic builds.
> 
> Or are you saying that the seed shouldn't depend on the build date. OK, then
the
> seed shouldn't depend on the build date. I do expect that, for production
> builds, the seed will depend on the major version number such as "M57" plus at
> least some salt, since the whole point of the exercise is to avoid predictible
> IDs over long periods of time. But that number won't change every day.
> 
> Are there any other concerns? Or am I misunderstanding your reply?

You are not misunderstand, my reply didn't make any sense. :)

I only care about the seed determination.

Using the build date would be good to use to derive the seed from. This is
because the script changes the "build date" once per month for dev build and
once per day for prod build.

[nigeltao1, 2017-04-07 02:59:41]

Description was changed from

==========
Add a mechanism to scramble Mojo message IDs.

BUG=
==========

to

==========
Add a mechanism to scramble Mojo message IDs.

BUG=673417
==========

[nigeltao1, 2017-04-07 03:00:58]

Description was changed from

==========
Add a mechanism to scramble Mojo message IDs.

BUG=673417
==========

to

==========
Add a mechanism to scramble Mojo message IDs.

For example, chrome/common/renderer_configuration.mojom defines:

interface RendererConfiguration {
  SetInitialConfiguration(etc);
  SetContentSettingRules(etc);
  SetFieldTrialGroup(etc);
};

The generated IDs in
gen/chrome/common/renderer_configuration.mojom-shared-internal.h, prior
to this CL, and also after this CL with an empty seed, are:

constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name = 0;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name = 1;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 2;

After this CL, with a "foo" seed, the IDs are:

constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name =
7921546;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name =
315453540;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 712054529;

BUG=673417
==========

[nigeltao1, 2017-04-07 03:06:59]

On 2017/04/07 01:45:13, M-A Ruel wrote:
> Using the build date would be good to use to derive the seed from. This is
> because the script changes the "build date" once per month for dev build and
> once per day for prod build.

OK, that makes sense. :-)

Let's continue the discussion on how to pick and update the seed on
https://crbug.com/673417

In the meantime, who should review the code itself? tsepez??

[nigeltao1, 2017-04-07 03:10:40]

Description was changed from

==========
Add a mechanism to scramble Mojo message IDs.

For example, chrome/common/renderer_configuration.mojom defines:

interface RendererConfiguration {
  SetInitialConfiguration(etc);
  SetContentSettingRules(etc);
  SetFieldTrialGroup(etc);
};

The generated IDs in
gen/chrome/common/renderer_configuration.mojom-shared-internal.h, prior
to this CL, and also after this CL with an empty seed, are:

constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name = 0;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name = 1;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 2;

After this CL, with a "foo" seed, the IDs are:

constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name =
7921546;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name =
315453540;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 712054529;

BUG=673417
==========

to

==========
Add a mechanism to scramble Mojo message IDs.

For example, chrome/common/renderer_configuration.mojom defines:

interface RendererConfiguration {
  SetInitialConfiguration(etc);
  SetContentSettingRules(etc);
  SetFieldTrialGroup(etc);
};

The generated IDs in
gen/chrome/common/renderer_configuration.mojom-shared-internal.h, prior
to this CL, and also after this CL with an empty seed, are:

constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name = 0;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name = 1;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 2;

After this CL, with a "foo" seed, the IDs are:

// The 7921546 value is based on crc32(seed + "RendererConfiguration1" + seed).
constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name =
7921546;
// The 315453540 value is based on crc32(seed + "RendererConfiguration2" +
seed).
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name =
315453540;
// The 712054529 value is based on crc32(seed + "RendererConfiguration3" +
seed).
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 712054529;

BUG=673417
==========

[Tom Sepez, 2017-04-07 17:32:53]

Sorry, using the build date would not be acceptable from a security perspective.
 The advesary could then generate all the possibilities for the near future, and
intercept anything matching them.  So it truly needs to be unugessable.

[M-A Ruel, 2017-04-07 17:37:27]

On 2017/04/07 17:32:53, Tom Sepez wrote:
> Sorry, using the build date would not be acceptable from a security
perspective.
>  The advesary could then generate all the possibilities for the near future,
and
> intercept anything matching them.  So it truly needs to be unugessable.

To be clear: I don't mind about the seed used for official builds, as long as
dev builds are reproducible. It's up to security folks to determine if
reproducible official builds are valuable.

I'll assume your comment was specifically about official builds.

[Tom Sepez, 2017-04-11 16:42:23]

> To be clear: I don't mind about the seed used for official builds, as long as
> dev builds are reproducible. It's up to security folks to determine if
> reproducible official builds are valuable.
> 
> I'll assume your comment was specifically about official builds.

Ok, no concern beyond what you desire for official builds.  So I think we can go
with this.

[Tom Sepez, 2017-04-11 16:43:24]

https://codereview.chromium.org/2794743002/diff/20001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom_bindings_generator.py (right):

https://codereview.chromium.org/2794743002/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom_bindings_generator.py:226: global_crc =
binascii.crc32(seed)
Can we do sha256 instead, just because CRC isn't really appropriate for
generating secrets.

[nigeltao1, 2017-04-20 04:35:04]

On 2017/04/11 16:43:24, Tom Sepez (OOO Wed - Thu) wrote:
>
https://codereview.chromium.org/2794743002/diff/20001/mojo/public/tools/bindi...
> File mojo/public/tools/bindings/mojom_bindings_generator.py (right):
> 
>
https://codereview.chromium.org/2794743002/diff/20001/mojo/public/tools/bindi...
> mojo/public/tools/bindings/mojom_bindings_generator.py:226: global_crc =
> binascii.crc32(seed)
> Can we do sha256 instead, just because CRC isn't really appropriate for
> generating secrets.

SHA-256 is done.

As for using the build date as the seed for official builds, I was imagining
basing the seed on the build date but not solely on the build date. For example,
given a suitably long secret, the actual seed used would be something like
"aSuitablyLongSecret20170420", and the adversary would not be able to
reverse-enginer that "aSuitablyLongSecret" prefix from the generated IDs even
though the build dates are easily guessable.

[nigeltao1, 2017-04-20 04:36:07]

Description was changed from

==========
Add a mechanism to scramble Mojo message IDs.

For example, chrome/common/renderer_configuration.mojom defines:

interface RendererConfiguration {
  SetInitialConfiguration(etc);
  SetContentSettingRules(etc);
  SetFieldTrialGroup(etc);
};

The generated IDs in
gen/chrome/common/renderer_configuration.mojom-shared-internal.h, prior
to this CL, and also after this CL with an empty seed, are:

constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name = 0;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name = 1;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 2;

After this CL, with a "foo" seed, the IDs are:

// The 7921546 value is based on crc32(seed + "RendererConfiguration1" + seed).
constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name =
7921546;
// The 315453540 value is based on crc32(seed + "RendererConfiguration2" +
seed).
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name =
315453540;
// The 712054529 value is based on crc32(seed + "RendererConfiguration3" +
seed).
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 712054529;

BUG=673417
==========

to

==========
Add a mechanism to scramble Mojo message IDs.

For example, chrome/common/renderer_configuration.mojom defines:

interface RendererConfiguration {
  SetInitialConfiguration(etc);
  SetContentSettingRules(etc);
  SetFieldTrialGroup(etc);
};

The generated IDs in
gen/chrome/common/renderer_configuration.mojom-shared-internal.h, prior
to this CL, and also after this CL with an empty seed, are:

constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name = 0;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name = 1;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 2;

After this CL, with a "foo" seed, the IDs are:
    
constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name =
1257880741;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name =
631133653;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 549336076;

BUG=673417
==========

[nigeltao1, 2017-04-20 04:36:07]

nigeltao@chromium.org changed reviewers:
+ yzshen@chromium.org

[nigeltao1, 2017-04-20 04:38:35]

I also added a test, although I'm not fluent in Python. I'm not confident that
it's idiomatic.

yzhen, can you review the Mojo generator code, particularly as you are in
mojo/OWNERS.

tsepez, can you review for security.

maruel, I presume that you are happy wrt reproducibility.

[nigeltao1, 2017-04-20 04:50:06]

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

[commit-bot: I haz the power, 2017-04-20 04:50:31]

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, 2017-04-20 05:47:52]

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

[commit-bot: I haz the power, 2017-04-20 05:47:54]

Dry run: This issue passed the CQ dry run.

[M-A Ruel, 2017-04-20 08:31:45]

On 2017/04/20 04:38:35, nigeltao1 wrote:
> maruel, I presume that you are happy wrt reproducibility.

Yes, lgtm for this.

https://codereview.chromium.org/2794743002/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom_bindings_generator.py (right):

https://codereview.chromium.org/2794743002/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom_bindings_generator.py:360:
"--scrambled_message_id_seed", type=str,
type=str is the default, so not needed.

Technically speaking, this is a salt, not a seed, so please rename.
https://en.wikipedia.org/wiki/Salt_(cryptography)

[yzshen1, 2017-04-20 17:21:49]

LGTM

https://codereview.chromium.org/2794743002/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom_bindings_generator.py (right):

https://codereview.chromium.org/2794743002/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom_bindings_generator.py:119: # harder for a
compromised renderer process to send fake Mojo messages.
Nit: please remove "renderer". There are many kinds of processes and this is not
a problem specific to renderers.

https://codereview.chromium.org/2794743002/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom_bindings_generator.py:120: s =
hashlib.sha256(seed)
Nit: please use a more descriptive variable name than "s".

[nigeltao1, 2017-04-20 23:39:13]

https://codereview.chromium.org/2794743002/diff/40001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom_bindings_generator.py (right):

https://codereview.chromium.org/2794743002/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom_bindings_generator.py:119: # harder for a
compromised renderer process to send fake Mojo messages.
On 2017/04/20 17:21:49, yzshen1 wrote:
> Nit: please remove "renderer". There are many kinds of processes and this is
not
> a problem specific to renderers.

Done.

https://codereview.chromium.org/2794743002/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom_bindings_generator.py:120: s =
hashlib.sha256(seed)
On 2017/04/20 17:21:49, yzshen1 wrote:
> Nit: please use a more descriptive variable name than "s".

Done.

https://codereview.chromium.org/2794743002/diff/40001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom_bindings_generator.py:360:
"--scrambled_message_id_seed", type=str,
On 2017/04/20 08:31:45, M-A Ruel wrote:
> type=str is the default, so not needed.

There's other unnecessary uses of "type=str" in this file, so I'll clean this
all up in a follow-up CL.


> Technically speaking, this is a salt, not a seed, so please rename.
> https://en.wikipedia.org/wiki/Salt_(cryptography)

Done.

[nigeltao1, 2017-04-20 23:39:44]

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

[nigeltao1, 2017-04-20 23:39:57]

tsepez, PTAL.

[commit-bot: I haz the power, 2017-04-20 23:40:02]

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

[yzshen1, 2017-04-21 00:45:07]

lgtm

[Tom Sepez, 2017-04-24 16:48:03]

lgtm

[nigeltao1, 2017-04-27 00:42:09]

The CQ bit was checked by nigeltao@chromium.org

[nigeltao1, 2017-04-27 00:42:10]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/2794743002/#ps80001
(title: "Add a mechanism to scramble Mojo message IDs.")

[commit-bot: I haz the power, 2017-04-27 00:43:07]

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, 2017-04-27 03:57:39]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493253729972160,
"parent_rev": "c1ad8f0a9e80c7b864f432fb4cdbb6f987de53fb", "commit_rev":
"9c12ed217b0f67085d1ec83ab3eabdabe72c40da"}

[commit-bot: I haz the power, 2017-04-27 03:57:57]

Description was changed from

==========
Add a mechanism to scramble Mojo message IDs.

For example, chrome/common/renderer_configuration.mojom defines:

interface RendererConfiguration {
  SetInitialConfiguration(etc);
  SetContentSettingRules(etc);
  SetFieldTrialGroup(etc);
};

The generated IDs in
gen/chrome/common/renderer_configuration.mojom-shared-internal.h, prior
to this CL, and also after this CL with an empty seed, are:

constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name = 0;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name = 1;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 2;

After this CL, with a "foo" seed, the IDs are:
    
constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name =
1257880741;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name =
631133653;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 549336076;

BUG=673417
==========

to

==========
Add a mechanism to scramble Mojo message IDs.

For example, chrome/common/renderer_configuration.mojom defines:

interface RendererConfiguration {
  SetInitialConfiguration(etc);
  SetContentSettingRules(etc);
  SetFieldTrialGroup(etc);
};

The generated IDs in
gen/chrome/common/renderer_configuration.mojom-shared-internal.h, prior
to this CL, and also after this CL with an empty seed, are:

constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name = 0;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name = 1;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 2;

After this CL, with a "foo" seed, the IDs are:
    
constexpr uint32_t kRendererConfiguration_SetInitialConfiguration_Name =
1257880741;
constexpr uint32_t kRendererConfiguration_SetContentSettingRules_Name =
631133653;
constexpr uint32_t kRendererConfiguration_SetFieldTrialGroup_Name = 549336076;

BUG=673417

Review-Url: https://codereview.chromium.org/2794743002
Cr-Commit-Position: refs/heads/master@{#467573}
Committed:
https://chromium.googlesource.com/chromium/src/+/9c12ed217b0f67085d1ec83ab3ea...
==========

[commit-bot: I haz the power, 2017-04-27 03:57:58]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9c12ed217b0f67085d1ec83ab3ea...