Add base::UnguessableToken

Add base::UnguessableToken

cc::SurfaceId, gpu::Mailbox and ScopedSurfaceRequestManager need an
unguessable identifier. Security recommends using 128 bits to make sure
an ID is unguessable. However, there is no conveniently serializable
way to represent 128 bits.

This change introduces base::UnguessableToken, a 128 bit class with a
cryptographically strong Create() function. UnguessableToken can be
used by themselves, or as part of an aggregate ID.

An empty UnguessableToken is a valid value. It is however illegal to
send empty UnguessableToken across processes (because the resource that
is supposed to be protected by the token would now be guessable).
Sending empty tokens across processes is a security issue, and should
be handled as such.

This change also introduces the appropriate code to send tokens
over IPC and Mojo. base::Optional should be used in cases where it may
be valid to send no token (rather than sending an empty token).

TEST=Added unittests. Also tested in a prototype that uses IPC and Mojo.
BUG=643857
Committed: https://crrev.com/4a5ac609f7ea52f2eb69fe871ba6b00c463b9b25
Cr-Commit-Position: refs/heads/master@{#419550}

[tguilbert, 2016-09-10 09:09:52]

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

[commit-bot: I haz the power, 2016-09-10 09:10:01]

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-10 10:29:54]

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

[commit-bot: I haz the power, 2016-09-10 10:29:55]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-09-12 20:41:56]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2016-09-12 20:41:57]

A few comments on the base impl

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode21
base/nonce.h:21: // PLEASE MAKE SURE YOUR USAGE OF THIS CLASS IS SECURE!
You should explain how to make sure or this comment is scary but not actionable.

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode24
base/nonce.h:24: Nonce() : high(0), low(0){};
 = default;

move the defn to a .cc file

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode25
base/nonce.h:25: Nonce(uint64_t high, uint64_t low) : high(high), low(low){};
dittos

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode28
base/nonce.h:28: static Nonce Generate();
maybe this should go first in the class so people find and use this.

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode30
base/nonce.h:30: bool is_null() const { return high == 0 && low == 0; };
no ; after a function body

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode33
base/nonce.h:33: return base::StringPrintf("(%" PRIu64 ":%" PRIu64 ")", high,
low);
move this fn defn to a .cc file

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode38
base/nonce.h:38: size_t hash() const { return base::HashInts64(high, low); }
Think this should be part of the hash struct, rather than part of this class.

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode41
base/nonce.h:41: return high == other.high ? low < other.low : high <
other.high;
use std::tie

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode50
base/nonce.h:50: // Note: two uint64_t are used instead of uint8_t[16], in order
to have a
Two

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode59
base/nonce.h:59: // Specialize default hash for use in std::unordered_map.
You can provider a struct to do the hash if you like, but it should not be in
std::. https://google.github.io/styleguide/cppguide.html#std_hash

https://codereview.chromium.org/2333443002/diff/1/base/nonce_unittest.cc
File base/nonce_unittest.cc (right):

https://codereview.chromium.org/2333443002/diff/1/base/nonce_unittest.cc#newc...
base/nonce_unittest.cc:19: EXPECT_TRUE(nonce == nonce);
can you compare 2 different Nonces with the same internal values instead of
comparing to *this

[Fady Samuel, 2016-09-12 20:47:47]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-09-12 20:47:48]

Some non-OWNER thoughts.

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode28
base/nonce.h:28: static Nonce Generate();
On 2016/09/12 20:41:57, danakj wrote:
> maybe this should go first in the class so people find and use this.

Do we want to expose manually setting the nonce to potentially something
insecure? I'm not sure how I feel about this constructor:

Nonce(uint64_t high, uint64_t low)

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode52
base/nonce.h:52: uint64_t high;
I'm a bit worried about exposing these because that invites the possibility of
changing them in client code after creation.

Personally I think it might make more sense to make these private and only
provide readonly accessors?

I'm not sure how we'd expose this to serialization code though. We could either
make serialization code a friend (layering violation) or continue to expose
Nonce(uint64_t high, uint64_t low). WDYT?

[danakj, 2016-09-12 20:54:55]

On Mon, Sep 12, 2016 at 1:47 PM, <fsamuel@chromium.org> wrote:

> Some non-OWNER thoughts.
>
>
> https://codereview.chromium.org/2333443002/diff/1/base/nonce.h
> File base/nonce.h (right):
>
> https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode28
> base/nonce.h:28: static Nonce Generate();
> On 2016/09/12 20:41:57, danakj wrote:
> > maybe this should go first in the class so people find and use this.
>
> Do we want to expose manually setting the nonce to potentially something
> insecure? I'm not sure how I feel about this constructor:
>
> Nonce(uint64_t high, uint64_t low)
>
> https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode52
> base/nonce.h:52: uint64_t high;
> I'm a bit worried about exposing these because that invites the
> possibility of changing them in client code after creation.
>
> Personally I think it might make more sense to make these private and
> only provide readonly accessors?
>
> I'm not sure how we'd expose this to serialization code though. We could
> either make serialization code a friend (layering violation) or continue
> to expose Nonce(uint64_t high, uint64_t low). WDYT?
>

I think leaving the constructor is fine, fwiw, at which point you can
construct and replace the None. So leaving the fields visible is fine too.
Having the type know about its users is not super scaling.


>
> https://codereview.chromium.org/2333443002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tguilbert, 2016-09-12 23:53:32]

tguilbert@chromium.org changed reviewers:
+ dcheng@chromium.org, jam@chromium.org

[tguilbert, 2016-09-12 23:53:34]

danakj PTAL @ base/*
dcheng PTAL @ mojo/common/*
jam    PTAL @ ipc/*

Addressed comments:
- Made Nonce(uint64_t, uint64_t) private.
- Added a UT that verifies that the class is a POD according to c++11 standards
(and therefore memcpy-able). LMK if there is a better way to do this, of if this
should live in the IPC tests instead.

Thank you very much!
Thomas

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode21
base/nonce.h:21: // PLEASE MAKE SURE YOUR USAGE OF THIS CLASS IS SECURE!
On 2016/09/12 20:41:57, danakj wrote:
> You should explain how to make sure or this comment is scary but not
actionable.

Updated.

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode24
base/nonce.h:24: Nonce() : high(0), low(0){};
On 2016/09/12 20:41:57, danakj wrote:
>  = default;
> 
> move the defn to a .cc file

I have updated it to be "= default". If you have no objection, I will leave the
= default here, because moving it to the .cc file is considered to be a "user
defined" ctor, and std::is_pod<Nonce>::value would no longer be true. (Following
info I found in this post
http://stackoverflow.com/questions/4178175/what-are-aggregates-and-pods-and-h...)

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode25
base/nonce.h:25: Nonce(uint64_t high, uint64_t low) : high(high), low(low){};
On 2016/09/12 20:41:57, danakj wrote:
> dittos

Done.

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode28
base/nonce.h:28: static Nonce Generate();
On 2016/09/12 20:47:47, Fady Samuel wrote:
> On 2016/09/12 20:41:57, danakj wrote:
> > maybe this should go first in the class so people find and use this.
> 
> Do we want to expose manually setting the nonce to potentially something
> insecure? I'm not sure how I feel about this constructor:
> 
> Nonce(uint64_t high, uint64_t low) 

I agree with you and have changed the class to be harder to misuse.

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode30
base/nonce.h:30: bool is_null() const { return high == 0 && low == 0; };
On 2016/09/12 20:41:57, danakj wrote:
> no ; after a function body

Done.

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode33
base/nonce.h:33: return base::StringPrintf("(%" PRIu64 ":%" PRIu64 ")", high,
low);
On 2016/09/12 20:41:57, danakj wrote:
> move this fn defn to a .cc file

Done.

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode38
base/nonce.h:38: size_t hash() const { return base::HashInts64(high, low); }
On 2016/09/12 20:41:57, danakj wrote:
> Think this should be part of the hash struct, rather than part of this class.

Done.

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode41
base/nonce.h:41: return high == other.high ? low < other.low : high <
other.high;
On 2016/09/12 20:41:57, danakj wrote:
> use std::tie

Done.

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode50
base/nonce.h:50: // Note: two uint64_t are used instead of uint8_t[16], in order
to have a
On 2016/09/12 20:41:57, danakj wrote:
> Two

Done.

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode52
base/nonce.h:52: uint64_t high;
On 2016/09/12 20:47:47, Fady Samuel wrote:
> I'm a bit worried about exposing these because that invites the possibility of
> changing them in client code after creation.
> 
> Personally I think it might make more sense to make these private and only
> provide readonly accessors?
> 
> I'm not sure how we'd expose this to serialization code though. We could
either
> make serialization code a friend (layering violation) or continue to expose
> Nonce(uint64_t high, uint64_t low). WDYT?

I agree with you. I wasn't sure in which ways one could misuse this class, but
moving the Nonce(uint64_t, uint64_t) to private forces users to be very
deliberate about their usage of the class.

I have added a Deserialize() function that I know I will use when converting
back from JNI (where sending two longs is simpler than checking the size of byte
arrays). Otherwise, the class is a POD class (and I have added unit test that
checks that std::is_pod<Nonce>::value is true, thanks to the info found
here:http://stackoverflow.com/questions/4178175/what-are-aggregates-and-pods-and-how-why-are-they-special/7189821#7189821).
The UT makes sure it safe to simply memcpy the class for deserialization.

https://codereview.chromium.org/2333443002/diff/1/base/nonce.h#newcode59
base/nonce.h:59: // Specialize default hash for use in std::unordered_map.
On 2016/09/12 20:41:57, danakj wrote:
> You can provider a struct to do the hash if you like, but it should not be in
> std::. https://google.github.io/styleguide/cppguide.html#std_hash

Done. TY!

https://codereview.chromium.org/2333443002/diff/1/base/nonce_unittest.cc
File base/nonce_unittest.cc (right):

https://codereview.chromium.org/2333443002/diff/1/base/nonce_unittest.cc#newc...
base/nonce_unittest.cc:19: EXPECT_TRUE(nonce == nonce);
On 2016/09/12 20:41:57, danakj wrote:
> can you compare 2 different Nonces with the same internal values instead of
> comparing to *this

Done.

https://codereview.chromium.org/2333443002/diff/20001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/20001/base/nonce.h#newcode50
base/nonce.h:50: return high_ == other.high_ && low_ == other.low_;
@security reviewers
Does this need to be changed to be constant time?
Ex:
uint64_t temp = high_ ^ other.high_;
temp |= low_ ^ other.low_;
return temp == 0;

(Similar to 
https://cs.chromium.org/chromium/src/crypto/secure_util.h?type=cs&q=secure+me...)

[tguilbert, 2016-09-12 23:53:35]

danakj PTAL @ base/*
dcheng PTAL @ mojo/common/*
jam    PTAL @ ipc/*

Addressed comments:
- Made Nonce(uint64_t, uint64_t) private.
- Added a UT that verifies that the class is a POD according to c++11 standards
(and therefore memcpy-able). LMK if there is a better way to do this, of if this
should live in the IPC tests instead.

Thank you very much!
Thomas

[tguilbert, 2016-09-12 23:53:51]

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

[commit-bot: I haz the power, 2016-09-12 23:54:13]

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

[Fady Samuel, 2016-09-12 23:58:06]

I love this change, making Nonce::Deserialize explicit! LGTM! Thanks!

[dcheng, 2016-09-13 01:33:29]

Btw, should we pass by Nonce by value or by const ref? Seems small enough that
by value might be a good idea.

https://codereview.chromium.org/2333443002/diff/40001/base/nonce.cc
File base/nonce.cc (right):

https://codereview.chromium.org/2333443002/diff/40001/base/nonce.cc#newcode11
base/nonce.cc:11: namespace base {
Nit: newlines after { here and before closing }, for consistency.

https://codereview.chromium.org/2333443002/diff/40001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/40001/base/nonce.h#newcode32
base/nonce.h:32: static Nonce Deserialize(uint64_t high, uint64_t low);
This doesn't appear to actually be used outside of tests. I'd be OK with just
not having this until it's clear it's needed. That way, we don't need the big
scary warning either.

https://codereview.chromium.org/2333443002/diff/40001/base/nonce.h#newcode37
base/nonce.h:37: bool is_null() const { return high_ == 0 && low_ == 0; }
This also seems unused, but maybe this is useful to keep around despite that.

https://codereview.chromium.org/2333443002/diff/40001/base/nonce.h#newcode73
base/nonce.h:73: static_assert(std::is_pod<Nonce>::value, "base::Nonce should be
of POD type.");
Nit: newline after this for symmetry

[tguilbert, 2016-09-13 01:50:12]

I also think that by value should be fine.

Side question: Should the memory backing a Nonce be 0-ed out when that Nonce is
destroyed?

https://codereview.chromium.org/2333443002/diff/40001/base/nonce.cc
File base/nonce.cc (right):

https://codereview.chromium.org/2333443002/diff/40001/base/nonce.cc#newcode11
base/nonce.cc:11: namespace base {
On 2016/09/13 01:33:29, dcheng wrote:
> Nit: newlines after { here and before closing }, for consistency.

Done.

https://codereview.chromium.org/2333443002/diff/40001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/40001/base/nonce.h#newcode32
base/nonce.h:32: static Nonce Deserialize(uint64_t high, uint64_t low);
On 2016/09/13 01:33:29, dcheng wrote:
> This doesn't appear to actually be used outside of tests. I'd be OK with just
> not having this until it's clear it's needed. That way, we don't need the big
> scary warning either.

The ScopedSurfaceRequestManager makes use of this when going back from Java to
C++
(https://codereview.chromium.org/2285593002/diff/260001/content/browser/androi...),
but that CL is dependent on this one.

https://codereview.chromium.org/2333443002/diff/40001/base/nonce.h#newcode37
base/nonce.h:37: bool is_null() const { return high_ == 0 && low_ == 0; }
On 2016/09/13 01:33:29, dcheng wrote:
> This also seems unused, but maybe this is useful to keep around despite that.

It is indeed unused ATM, but was probably going to use it in a few DCHECKs in
the ScopedSurfaceRequestManager.

https://codereview.chromium.org/2333443002/diff/40001/base/nonce.h#newcode73
base/nonce.h:73: static_assert(std::is_pod<Nonce>::value, "base::Nonce should be
of POD type.");
On 2016/09/13 01:33:29, dcheng wrote:
> Nit: newline after this for symmetry

Done.

[jam, 2016-09-13 17:43:45]

jam@chromium.org changed reviewers:
+ tsepez@chromium.org
- jam@chromium.org

[jam, 2016-09-13 17:43:46]

Redirecting to Tom for review of ipc/

[Fady Samuel, 2016-09-13 17:45:00]

https://codereview.chromium.org/2333443002/diff/60001/mojo/common/common_cust...
File mojo/common/common_custom_types.mojom (right):

https://codereview.chromium.org/2333443002/diff/60001/mojo/common/common_cust...
mojo/common/common_custom_types.mojom:17: struct Nonce;
Sorry just noticed. Could we please make this not [Native]? Could you please
define the high and low here and write StructTraits? Thanks!

[Tom Sepez, 2016-09-13 18:20:41]

https://codereview.chromium.org/2333443002/diff/60001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/60001/base/nonce.h#newcode34
base/nonce.h:34: // Creates a null Nonce.
Actually, wouldn't this create an uninitialized nonce?  Do we want to set to
(0,0)?

https://codereview.chromium.org/2333443002/diff/60001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2333443002/diff/60001/ipc/ipc_message_utils.c...
ipc/ipc_message_utils.cc:1018: memcpy(r, data, sizeof(param_type));
Yes, this works, but I'm less thrilled with copying over the struct than having
a public ctor and passing these over the wire as two 64s.

[tguilbert, 2016-09-13 20:20:19]

Updated. PTAL! TY.

https://codereview.chromium.org/2333443002/diff/60001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/60001/base/nonce.h#newcode34
base/nonce.h:34: // Creates a null Nonce.
On 2016/09/13 18:20:41, Tom Sepez wrote:
> Actually, wouldn't this create an uninitialized nonce?  Do we want to set to
> (0,0)?

It does create a (0,0) Nonce (and I have a UT to make sure it is set to 0). I
updated the comment, and renamed is_null() to is_empty().

https://codereview.chromium.org/2333443002/diff/60001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2333443002/diff/60001/ipc/ipc_message_utils.c...
ipc/ipc_message_utils.cc:1018: memcpy(r, data, sizeof(param_type));
On 2016/09/13 18:20:41, Tom Sepez wrote:
> Yes, this works, but I'm less thrilled with copying over the struct than
having
> a public ctor and passing these over the wire as two 64s.

Done.

https://codereview.chromium.org/2333443002/diff/60001/mojo/common/common_cust...
File mojo/common/common_custom_types.mojom (right):

https://codereview.chromium.org/2333443002/diff/60001/mojo/common/common_cust...
mojo/common/common_custom_types.mojom:17: struct Nonce;
On 2016/09/13 17:45:00, Fady Samuel wrote:
> Sorry just noticed. Could we please make this not [Native]? Could you please
> define the high and low here and write StructTraits? Thanks!

Done.

I originally had StructTraits but encountered huge linking errors in the
component build, so I switched to [Native].

From looking around, it seems like a lot of the StructTraits have their
implementation in the .h file (url_gurl_struct_traits.h for instance). Is this
the proper way to go?

[Avi (use Gerrit), 2016-09-13 20:28:37]

avi@chromium.org changed reviewers:
+ avi@chromium.org

[Avi (use Gerrit), 2016-09-13 20:28:38]

https://codereview.chromium.org/2333443002/diff/100001/base/nonce.cc
File base/nonce.cc (right):

https://codereview.chromium.org/2333443002/diff/100001/base/nonce.cc#newcode24
base/nonce.cc:24: base::RandBytes(&nonce, sizeof(nonce));
This potentially could scribble across padding. Along with the static_assert for
the POD-ness of Nonce, do you want to assert that sizeof(Nonce) ==
2*sizeof(uint64_t) or do you not mind the padding scribble?

https://codereview.chromium.org/2333443002/diff/100001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/100001/base/nonce.h#newcode23
base/nonce.h:23: // Generate(), and sent accross processes as two uint64_t.
typo: across

https://codereview.chromium.org/2333443002/diff/100001/base/nonce.h#newcode27
base/nonce.h:27: // USE THIS FUNCTION TO GENERATE NEW NONCES.
use ONLY this function?

https://codereview.chromium.org/2333443002/diff/100001/base/nonce.h#newcode64
base/nonce.h:64: // For use in std::unorderer_map.
typo: unordered_map

[Fady Samuel, 2016-09-13 20:28:44]

https://codereview.chromium.org/2333443002/diff/60001/mojo/common/common_cust...
File mojo/common/common_custom_types.mojom (right):

https://codereview.chromium.org/2333443002/diff/60001/mojo/common/common_cust...
mojo/common/common_custom_types.mojom:17: struct Nonce;
On 2016/09/13 20:20:19, ThomasGuilbert wrote:
> On 2016/09/13 17:45:00, Fady Samuel wrote:
> > Sorry just noticed. Could we please make this not [Native]? Could you please
> > define the high and low here and write StructTraits? Thanks!
> 
> Done.
> 
> I originally had StructTraits but encountered huge linking errors in the
> component build, so I switched to [Native].
> 
> From looking around, it seems like a lot of the StructTraits have their
> implementation in the .h file (url_gurl_struct_traits.h for instance). Is this
> the proper way to go?

That's fine. If you want some of the StructTraits to live in a cc file you have
to specify it in the typemap:

https://cs.chromium.org/chromium/src/cc/ipc/compositor_frame.typemap

https://codereview.chromium.org/2333443002/diff/100001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2333443002/diff/100001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:1008: ParamTraits<int64_t>::Write(m, p.high());
uint64_t?

https://codereview.chromium.org/2333443002/diff/100001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:1017: if (!ParamTraits<int64_t>::Read(m, iter, &high)
||
Shouldn't this be uint64_t?

[tguilbert, 2016-09-13 21:11:45]

https://codereview.chromium.org/2333443002/diff/60001/mojo/common/common_cust...
File mojo/common/common_custom_types.mojom (right):

https://codereview.chromium.org/2333443002/diff/60001/mojo/common/common_cust...
mojo/common/common_custom_types.mojom:17: struct Nonce;
On 2016/09/13 20:28:44, Fady Samuel wrote:
> On 2016/09/13 20:20:19, ThomasGuilbert wrote:
> > On 2016/09/13 17:45:00, Fady Samuel wrote:
> > > Sorry just noticed. Could we please make this not [Native]? Could you
please
> > > define the high and low here and write StructTraits? Thanks!
> > 
> > Done.
> > 
> > I originally had StructTraits but encountered huge linking errors in the
> > component build, so I switched to [Native].
> > 
> > From looking around, it seems like a lot of the StructTraits have their
> > implementation in the .h file (url_gurl_struct_traits.h for instance). Is
this
> > the proper way to go?
> 
> That's fine. If you want some of the StructTraits to live in a cc file you
have
> to specify it in the typemap:
> 
> https://cs.chromium.org/chromium/src/cc/ipc/compositor_frame.typemap

Ok, thanks for the info.

https://codereview.chromium.org/2333443002/diff/100001/base/nonce.cc
File base/nonce.cc (right):

https://codereview.chromium.org/2333443002/diff/100001/base/nonce.cc#newcode24
base/nonce.cc:24: base::RandBytes(&nonce, sizeof(nonce));
On 2016/09/13 20:28:38, Avi wrote:
> This potentially could scribble across padding. Along with the static_assert
for
> the POD-ness of Nonce, do you want to assert that sizeof(Nonce) ==
> 2*sizeof(uint64_t) or do you not mind the padding scribble?

Since I have removed the call to memcpy and send the nonce as 2 uint64_ts
instead, the PODness is no longer all that important. I am replacing the POD
assert with the sizeof assert, and updated the comment. TY for the suggestion!

https://codereview.chromium.org/2333443002/diff/100001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/100001/base/nonce.h#newcode23
base/nonce.h:23: // Generate(), and sent accross processes as two uint64_t.
On 2016/09/13 20:28:38, Avi wrote:
> typo: across

Done.

https://codereview.chromium.org/2333443002/diff/100001/base/nonce.h#newcode27
base/nonce.h:27: // USE THIS FUNCTION TO GENERATE NEW NONCES.
On 2016/09/13 20:28:38, Avi wrote:
> use ONLY this function?

Done.

https://codereview.chromium.org/2333443002/diff/100001/base/nonce.h#newcode64
base/nonce.h:64: // For use in std::unorderer_map.
On 2016/09/13 20:28:38, Avi wrote:
> typo: unordered_map

Done.

https://codereview.chromium.org/2333443002/diff/100001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2333443002/diff/100001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:1008: ParamTraits<int64_t>::Write(m, p.high());
On 2016/09/13 20:28:44, Fady Samuel wrote:
> uint64_t?

Oops! You are right, TY. Done.

https://codereview.chromium.org/2333443002/diff/100001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:1017: if (!ParamTraits<int64_t>::Read(m, iter, &high)
||
On 2016/09/13 20:28:44, Fady Samuel wrote:
> Shouldn't this be uint64_t?

Done.

[tguilbert, 2016-09-13 21:12:30]

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

[commit-bot: I haz the power, 2016-09-13 21:13:16]

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

[Tom Sepez, 2016-09-13 21:29:39]

LGTM otherwise.

https://codereview.chromium.org/2333443002/diff/60001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/60001/base/nonce.h#newcode34
base/nonce.h:34: // Creates a null Nonce.
On 2016/09/13 20:20:19, ThomasGuilbert wrote:
> On 2016/09/13 18:20:41, Tom Sepez wrote:
> > Actually, wouldn't this create an uninitialized nonce?  Do we want to set to
> > (0,0)?
> 
> It does create a (0,0) Nonce (and I have a UT to make sure it is set to 0). I
> updated the comment, and renamed is_null() to is_empty().

Ok, yes if the ctor is called, so your test Nonce().is_empty() will pass, but
what about:

  Nonce nonce;
  nonce.is_empty()

sure you dont wan't a user-defined ctor to cover this case?

[Tom Sepez, 2016-09-13 22:11:04]

(The reason I suggest this is that uninitialized values look very much like
random values, and we may not notice that they aren't really random in
practice).

[commit-bot: I haz the power, 2016-09-13 22:40:15]

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

[commit-bot: I haz the power, 2016-09-13 22:40:16]

Dry run: This issue passed the CQ dry run.

[tguilbert, 2016-09-13 22:55:39]

On 2016/09/13 22:11:04, Tom Sepez wrote:
> (The reason I suggest this is that uninitialized values look very much like
> random values, and we may not notice that they aren't really random in
> practice).

Yes, that is very important, TY for pointing that out! I removed the '=
default'.

[Avi (use Gerrit), 2016-09-13 23:03:38]

lgtm

Nice!

[danakj, 2016-09-13 23:06:15]

On Mon, Sep 12, 2016 at 6:33 PM, <dcheng@chromium.org> wrote:

> Btw, should we pass by Nonce by value or by const ref? Seems small enough
> that
> by value might be a good idea.
>

For gfx::Size (2 ints) and friends, we concluded that we should pass by
const ref. It was faster for recursive microbenchmarks and not really
different otherwise. I think anything that isn't a primitive type should be
const& and that is an easy rule to remember.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tom Sepez, 2016-09-13 23:06:42]

++LGTM

[dcheng, 2016-09-13 23:19:06]

Mostly just nits.

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode17
base/nonce.h:17: // An Nonce is a cryptographically strong, randomly generated,
unguessable 128
A Nonce is an unguessable 128-bit token generated from a cryptographically
strong random source. Nonce should be used when <insert scenarios here>.

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode20
base/nonce.h:20: // TO CREATE NEW NONCES, ONLY USE GENERATE()!
Suggested wording:

Use Generate() for creating new unguessable nonces. Deserialize() is a helper
intended only for implementing IPC deserialization.

(I think we don't need to repeat the bit about cryptographically strong random,
since it's such a long phrase =)

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode26
base/nonce.h:26: // Generates a unique, strongly random, unguessable nonce.
Ditto: "Create an unguessable and unique nonce."

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode34
base/nonce.h:34: // Creates a 0 initiliazed Nonce.
Nit: Create a zero-initialized Nonce (so it doesn't look like an O)

https://codereview.chromium.org/2333443002/diff/140001/mojo/common/common_cus...
File mojo/common/common_custom_types.typemap (right):

https://codereview.chromium.org/2333443002/diff/140001/mojo/common/common_cus...
mojo/common/common_custom_types.typemap:29:
"mojo.common.mojom.Nonce=base::Nonce",
If we expect people to pass Nonce by value (and it sounds like we do?), then
this should be annotated with [copyable_pass_by_value]

https://codereview.chromium.org/2333443002/diff/140001/mojo/common/common_cus...
File mojo/common/common_custom_types_struct_traits.h (right):

https://codereview.chromium.org/2333443002/diff/140001/mojo/common/common_cus...
mojo/common/common_custom_types_struct_traits.h:31: static void
SetToNull(base::Nonce* out) { *out = base::Nonce(); }
Do we actually need to support nullability? I'd suggest only adding this when
there's a clear need.

[Ken Rockot(use gerrit already), 2016-09-13 23:21:12]

rockot@chromium.org changed reviewers:
+ rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-09-13 23:21:13]

A few drive-by comments.

We also use random 128-bit tokens inside the Mojo system layer, and we could
probably de-dupe some code by reusing this now.

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.cc
File base/nonce.cc (right):

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.cc#newcode21
base/nonce.cc:21: // Static
nit: lowercase

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode28
base/nonce.h:28: static Nonce Generate();
Why Generate? Why can't the default constructor just do what most consumers will
want it to do? We could add a special constructor for creating an empty Nonce
(e.g. over nullptr_t or something)?

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode34
base/nonce.h:34: // Creates a 0 initiliazed Nonce.
nit: initialized (typo)

[DaleCurtis, 2016-09-13 23:39:48]

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode71
base/nonce.h:71: // If base::Nonce is no longer 128 bits, the IPC serialization
logic and Mojo
Drive-by: Worth moving to C++ file to avoid all includes from running
static_assert? I.e. in c++ it'll only run once, versus once for each include.

[tguilbert, 2016-09-13 23:42:35]

On 2016/09/13 23:21:13, Ken Rockot wrote:
> A few drive-by comments.
> 
> We also use random 128-bit tokens inside the Mojo system layer, and we could
> probably de-dupe some code by reusing this now.
> 
> https://codereview.chromium.org/2333443002/diff/140001/base/nonce.cc
> File base/nonce.cc (right):
> 
> https://codereview.chromium.org/2333443002/diff/140001/base/nonce.cc#newcode21
> base/nonce.cc:21: // Static
> nit: lowercase
> 
> https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
> File base/nonce.h (right):
> 
> https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode28
> base/nonce.h:28: static Nonce Generate();
> Why Generate? Why can't the default constructor just do what most consumers
will
> want it to do? We could add a special constructor for creating an empty Nonce
> (e.g. over nullptr_t or something)?
> 
> https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode34
> base/nonce.h:34: // Creates a 0 initiliazed Nonce.
> nit: initialized (typo)

My reasoning for Generate() was that it differentiates between intentional use
and accidental use. This forces any future user of the class to be deliberate,
and (hopefully) think about the way they are using the Nonce. Also, I wanted to
avoid stray 'Nonce nonce;' from being used, and thought that the zeroed-out
Nonce would alert the developer that they may have made a mistake.

sandersd@ also mentioned to me offline that the Nonce() should be the generated
Nonce, and I can see why that makes sense.

Does anyone else want to weigh in on the matter?

[sandersd (OOO until July 31), 2016-09-13 23:51:52]

sandersd@chromium.org changed reviewers:
+ sandersd@chromium.org

[sandersd (OOO until July 31), 2016-09-13 23:51:53]

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode28
base/nonce.h:28: static Nonce Generate();
On 2016/09/13 23:21:13, Ken Rockot wrote:
> Why Generate? Why can't the default constructor just do what most consumers
will
> want it to do? We could add a special constructor for creating an empty Nonce
> (e.g. over nullptr_t or something)?

I do like the clarity of explicitly calling Generate(). A different option would
be to make the constructor protected. Both solutions are fine by me.

[danakj, 2016-09-13 23:52:52]

On Tue, Sep 13, 2016 at 4:42 PM, <tguilbert@chromium.org> wrote:

> On 2016/09/13 23:21:13, Ken Rockot wrote:
> > A few drive-by comments.
> >
> > We also use random 128-bit tokens inside the Mojo system layer, and we
> could
> > probably de-dupe some code by reusing this now.
> >
> > https://codereview.chromium.org/2333443002/diff/140001/base/nonce.cc
> > File base/nonce.cc (right):
> >
> > https://codereview.chromium.org/2333443002/diff/140001/
> base/nonce.cc#newcode21
> > base/nonce.cc:21: // Static
> > nit: lowercase
> >
> > https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
> > File base/nonce.h (right):
> >
> > https://codereview.chromium.org/2333443002/diff/140001/
> base/nonce.h#newcode28
> > base/nonce.h:28: static Nonce Generate();
> > Why Generate? Why can't the default constructor just do what most
> consumers
> will
> > want it to do? We could add a special constructor for creating an empty
> Nonce
> > (e.g. over nullptr_t or something)?
> >
> > https://codereview.chromium.org/2333443002/diff/140001/
> base/nonce.h#newcode34
> > base/nonce.h:34: // Creates a 0 initiliazed Nonce.
> > nit: initialized (typo)
>
> My reasoning for Generate() was that it differentiates between intentional
> use
> and accidental use. This forces any future user of the class to be
> deliberate,
> and (hopefully) think about the way they are using the Nonce. Also, I
> wanted to
> avoid stray 'Nonce nonce;' from being used, and thought that the zeroed-out
> Nonce would alert the developer that they may have made a mistake.
>
> sandersd@ also mentioned to me offline that the Nonce() should be the
> generated
> Nonce, and I can see why that makes sense.
>
> Does anyone else want to weigh in on the matter?
>

It's not POD then anymore right? Does that matter?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tguilbert, 2016-09-14 00:00:43]

On 2016/09/13 23:51:53, sandersd wrote:
> https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
> File base/nonce.h (right):
> 
> https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode28
> base/nonce.h:28: static Nonce Generate();
> On 2016/09/13 23:21:13, Ken Rockot wrote:
> > Why Generate? Why can't the default constructor just do what most consumers
> will
> > want it to do? We could add a special constructor for creating an empty
Nonce
> > (e.g. over nullptr_t or something)?
> 
> I do like the clarity of explicitly calling Generate(). A different option
would
> be to make the constructor protected. Both solutions are fine by me.

When making the constructor protected, I encountered some problems with some
Mojo generated code. 

Come to think of it, there might be some Mojo related technical reasons as to
why we would want the default to be 0 initialized (although I didn't look too
deeply into any workarounds/implications of the NonceDataView).

[tguilbert, 2016-09-14 00:03:27]

On 2016/09/13 23:52:52, danakj wrote:
> On Tue, Sep 13, 2016 at 4:42 PM, <mailto:tguilbert@chromium.org> wrote:
> 
> > On 2016/09/13 23:21:13, Ken Rockot wrote:
> > > A few drive-by comments.
> > >
> > > We also use random 128-bit tokens inside the Mojo system layer, and we
> > could
> > > probably de-dupe some code by reusing this now.
> > >
> > > https://codereview.chromium.org/2333443002/diff/140001/base/nonce.cc
> > > File base/nonce.cc (right):
> > >
> > > https://codereview.chromium.org/2333443002/diff/140001/
> > base/nonce.cc#newcode21
> > > base/nonce.cc:21: // Static
> > > nit: lowercase
> > >
> > > https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
> > > File base/nonce.h (right):
> > >
> > > https://codereview.chromium.org/2333443002/diff/140001/
> > base/nonce.h#newcode28
> > > base/nonce.h:28: static Nonce Generate();
> > > Why Generate? Why can't the default constructor just do what most
> > consumers
> > will
> > > want it to do? We could add a special constructor for creating an empty
> > Nonce
> > > (e.g. over nullptr_t or something)?
> > >
> > > https://codereview.chromium.org/2333443002/diff/140001/
> > base/nonce.h#newcode34
> > > base/nonce.h:34: // Creates a 0 initiliazed Nonce.
> > > nit: initialized (typo)
> >
> > My reasoning for Generate() was that it differentiates between intentional
> > use
> > and accidental use. This forces any future user of the class to be
> > deliberate,
> > and (hopefully) think about the way they are using the Nonce. Also, I
> > wanted to
> > avoid stray 'Nonce nonce;' from being used, and thought that the zeroed-out
> > Nonce would alert the developer that they may have made a mistake.
> >
> > sandersd@ also mentioned to me offline that the Nonce() should be the
> > generated
> > Nonce, and I can see why that makes sense.
> >
> > Does anyone else want to weigh in on the matter?
> >
> 
> It's not POD then anymore right? Does that matter?
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

It is no longer POD from a standards point of view, but I don't think it matters
anymore. It was important when I was memcpy-ing the bytes directly rather than
going through the Deserialize(uint64_t, uint64_t).

[danakj, 2016-09-14 00:05:48]

On Tue, Sep 13, 2016 at 5:00 PM, <tguilbert@chromium.org> wrote:

> On 2016/09/13 23:51:53, sandersd wrote:
> > https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
> > File base/nonce.h (right):
> >
> > https://codereview.chromium.org/2333443002/diff/140001/
> base/nonce.h#newcode28
> > base/nonce.h:28: static Nonce Generate();
> > On 2016/09/13 23:21:13, Ken Rockot wrote:
> > > Why Generate? Why can't the default constructor just do what most
> consumers
> > will
> > > want it to do? We could add a special constructor for creating an empty
> Nonce
> > > (e.g. over nullptr_t or something)?
> >
> > I do like the clarity of explicitly calling Generate(). A different
> option
> would
> > be to make the constructor protected. Both solutions are fine by me.
>
> When making the constructor protected, I encountered some problems with
> some
> Mojo generated code.
>
> Come to think of it, there might be some Mojo related technical reasons as
> to
> why we would want the default to be 0 initialized (although I didn't look
> too
> deeply into any workarounds/implications of the NonceDataView).
>

I think I prefer a default public constructor which sets to 0. Having
uninitialized states is useful, the alternative would force people to put
it into a base::Optional or std::unique_ptr instead.


>
> https://codereview.chromium.org/2333443002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tguilbert, 2016-09-14 00:57:08]

On 2016/09/14 00:05:48, danakj wrote:
> On Tue, Sep 13, 2016 at 5:00 PM, <mailto:tguilbert@chromium.org> wrote:
> 
> > On 2016/09/13 23:51:53, sandersd wrote:
> > > https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
> > > File base/nonce.h (right):
> > >
> > > https://codereview.chromium.org/2333443002/diff/140001/
> > base/nonce.h#newcode28
> > > base/nonce.h:28: static Nonce Generate();
> > > On 2016/09/13 23:21:13, Ken Rockot wrote:
> > > > Why Generate? Why can't the default constructor just do what most
> > consumers
> > > will
> > > > want it to do? We could add a special constructor for creating an empty
> > Nonce
> > > > (e.g. over nullptr_t or something)?
> > >
> > > I do like the clarity of explicitly calling Generate(). A different
> > option
> > would
> > > be to make the constructor protected. Both solutions are fine by me.
> >
> > When making the constructor protected, I encountered some problems with
> > some
> > Mojo generated code.
> >
> > Come to think of it, there might be some Mojo related technical reasons as
> > to
> > why we would want the default to be 0 initialized (although I didn't look
> > too
> > deeply into any workarounds/implications of the NonceDataView).
> >
> 
> I think I prefer a default public constructor which sets to 0. Having
> uninitialized states is useful, the alternative would force people to put
> it into a base::Optional or std::unique_ptr instead.
> 
> 
> >
> > https://codereview.chromium.org/2333443002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

After talking with sandersd@ offline, he brought up the very valid point that,
if the default constructor sets everything as 0, and we use Nonces as security
primitives, it allows a developer to accidentally used the zero-ed Nonce
everywhere.
Another way of saying this is that
Nonce() -> random-bits and explicit Nonce::Empty()  : misuse is annoying to
debug.
Nonce() -> zeros-out and explicit Nonce::Generate() : misuse is a security hole.

Which means that I will change the Nonce() to generate random data, and add an
explicit Nonce::Empty();


There is somewhat of a problem with StructTraits and the random Nonce().
StructTraits generates a NonceDataView which uses the default constructor to
create a local variable, and then passes that value by reference to the read
function. This means that, anytime a StructTrait is used, the NonceDataView
would create a Nonce() with new random data, and then overwrite that data.

I will start a thread with the Mojo team and ask if there is a way to avoid
using the default constructor in generated code.

In the meantime, in order not to not have StructTraits generate random bits
every time a Nonce is sent over mojo, I will write a TypeConverter instead, but
I might not have time by EOD.

[dcheng, 2016-09-14 01:01:25]

On 2016/09/14 00:57:08, ThomasGuilbert wrote:
> On 2016/09/14 00:05:48, danakj wrote:
> > On Tue, Sep 13, 2016 at 5:00 PM, <mailto:tguilbert@chromium.org> wrote:
> > 
> > > On 2016/09/13 23:51:53, sandersd wrote:
> > > > https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
> > > > File base/nonce.h (right):
> > > >
> > > > https://codereview.chromium.org/2333443002/diff/140001/
> > > base/nonce.h#newcode28
> > > > base/nonce.h:28: static Nonce Generate();
> > > > On 2016/09/13 23:21:13, Ken Rockot wrote:
> > > > > Why Generate? Why can't the default constructor just do what most
> > > consumers
> > > > will
> > > > > want it to do? We could add a special constructor for creating an
empty
> > > Nonce
> > > > > (e.g. over nullptr_t or something)?
> > > >
> > > > I do like the clarity of explicitly calling Generate(). A different
> > > option
> > > would
> > > > be to make the constructor protected. Both solutions are fine by me.
> > >
> > > When making the constructor protected, I encountered some problems with
> > > some
> > > Mojo generated code.
> > >
> > > Come to think of it, there might be some Mojo related technical reasons as
> > > to
> > > why we would want the default to be 0 initialized (although I didn't look
> > > too
> > > deeply into any workarounds/implications of the NonceDataView).
> > >
> > 
> > I think I prefer a default public constructor which sets to 0. Having
> > uninitialized states is useful, the alternative would force people to put
> > it into a base::Optional or std::unique_ptr instead.
> > 
> > 
> > >
> > > https://codereview.chromium.org/2333443002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> After talking with sandersd@ offline, he brought up the very valid point that,
> if the default constructor sets everything as 0, and we use Nonces as security
> primitives, it allows a developer to accidentally used the zero-ed Nonce
> everywhere.
> Another way of saying this is that
> Nonce() -> random-bits and explicit Nonce::Empty()  : misuse is annoying to
> debug.
> Nonce() -> zeros-out and explicit Nonce::Generate() : misuse is a security
hole.
> 
> Which means that I will change the Nonce() to generate random data, and add an
> explicit Nonce::Empty();
> 
> 
> There is somewhat of a problem with StructTraits and the random Nonce().
> StructTraits generates a NonceDataView which uses the default constructor to
> create a local variable, and then passes that value by reference to the read
> function. This means that, anytime a StructTrait is used, the NonceDataView
> would create a Nonce() with new random data, and then overwrite that data.
> 
> I will start a thread with the Mojo team and ask if there is a way to avoid
> using the default constructor in generated code.
> 
> In the meantime, in order not to not have StructTraits generate random bits
> every time a Nonce is sent over mojo, I will write a TypeConverter instead,
but
> I might not have time by EOD.

TypeConverter is deprecated and something we're actively (or at least, we should
be =P) trying to remove.

[danakj, 2016-09-14 01:08:28]

On Tue, Sep 13, 2016 at 5:57 PM, <tguilbert@chromium.org> wrote:

> On 2016/09/14 00:05:48, danakj wrote:
>
> > On Tue, Sep 13, 2016 at 5:00 PM, <mailto:tguilbert@chromium.org> wrote:
> >
> > > On 2016/09/13 23:51:53, sandersd wrote:
> > > > https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
> > > > File base/nonce.h (right):
> > > >
> > > > https://codereview.chromium.org/2333443002/diff/140001/
> > > base/nonce.h#newcode28
> > > > base/nonce.h:28: static Nonce Generate();
> > > > On 2016/09/13 23:21:13, Ken Rockot wrote:
> > > > > Why Generate? Why can't the default constructor just do what most
> > > consumers
> > > > will
> > > > > want it to do? We could add a special constructor for creating an
> empty
> > > Nonce
> > > > > (e.g. over nullptr_t or something)?
> > > >
> > > > I do like the clarity of explicitly calling Generate(). A different
> > > option
> > > would
> > > > be to make the constructor protected. Both solutions are fine by me.
> > >
> > > When making the constructor protected, I encountered some problems with
> > > some
> > > Mojo generated code.
> > >
> > > Come to think of it, there might be some Mojo related technical
> reasons as
> > > to
> > > why we would want the default to be 0 initialized (although I didn't
> look
> > > too
> > > deeply into any workarounds/implications of the NonceDataView).
> > >
> >
> > I think I prefer a default public constructor which sets to 0. Having
> > uninitialized states is useful, the alternative would force people to put
> > it into a base::Optional or std::unique_ptr instead.
> >
> >
> > >
> > > https://codereview.chromium.org/2333443002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
> After talking with sandersd@ offline, he brought up the very valid point
> that,
> if the default constructor sets everything as 0, and we use Nonces as
> security
> primitives, it allows a developer to accidentally used the zero-ed Nonce
> everywhere.
>

Can we just DCHECK that the nonce is not empty when sending it over IPC?


> Another way of saying this is that
> Nonce() -> random-bits and explicit Nonce::Empty() : misuse is annoying to
> debug.
> Nonce() -> zeros-out and explicit Nonce::Generate() : misuse is a security
> hole.
>
> Which means that I will change the Nonce() to generate random data, and
> add an
> explicit Nonce::Empty();
>
>
> There is somewhat of a problem with StructTraits and the random Nonce().
> StructTraits generates a NonceDataView which uses the default constructor
> to
> create a local variable, and then passes that value by reference to the
> read
> function. This means that, anytime a StructTrait is used, the NonceDataView
> would create a Nonce() with new random data, and then overwrite that data.
>
> I will start a thread with the Mojo team and ask if there is a way to avoid
> using the default constructor in generated code.
>
> In the meantime, in order not to not have StructTraits generate random bits
> every time a Nonce is sent over mojo, I will write a TypeConverter
> instead, but
> I might not have time by EOD.
>
> https://codereview.chromium.org/2333443002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tguilbert, 2016-09-14 22:40:48]

@dcheng Oops, yep, no TypeConverter :P

CHECKs/DCHECKS seem like the way to go :)

I have added a CHECK to:
- ParamTraits<base::Nonce>::Write()
- Nonce::Deserialize()
- NonceHash::operator()

And a DCHECK to Nonce::high()/Nonce::low() (although I am less sure about the
usefulness of these two)

I do realize that the CHECK in Deserialize will be hit right before
ParamTraits<>::Read() or StructTraits<>::Read() would return false. I however
like the idea of having the DCHECK in Deserialize, to cover the deserialization
on Android, after the high/low bits have been send through AIDL.

Does anybody have another suggested way of adding CHECKs? I.e. not having the
CHECK in deserialize and instead letting IPC/Mojo handle the invalid empty
nonce?

I also updated the == operator to use the same logic that is in SecureMemEqual.

Thanks!
Thomas

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode17
base/nonce.h:17: // An Nonce is a cryptographically strong, randomly generated,
unguessable 128
On 2016/09/13 23:19:05, dcheng wrote:
> A Nonce is an unguessable 128-bit token generated from a cryptographically
> strong random source. Nonce should be used when <insert scenarios here>.

Done. However, I don't know what all useful scenarios for Nonces are. I know
they can be used in some authentication schemes, but I think adding further uses
might be out of scope for the comments of this header file.

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode20
base/nonce.h:20: // TO CREATE NEW NONCES, ONLY USE GENERATE()!
On 2016/09/13 23:19:05, dcheng wrote:
> Suggested wording:
> 
> Use Generate() for creating new unguessable nonces. Deserialize() is a helper
> intended only for implementing IPC deserialization.
> 
> (I think we don't need to repeat the bit about cryptographically strong
random,
> since it's such a long phrase =)

TY for the better wording! Done.

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode26
base/nonce.h:26: // Generates a unique, strongly random, unguessable nonce.
On 2016/09/13 23:19:05, dcheng wrote:
> Ditto: "Create an unguessable and unique nonce."

Done.

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode28
base/nonce.h:28: static Nonce Generate();
On 2016/09/13 23:51:53, sandersd wrote:
> On 2016/09/13 23:21:13, Ken Rockot wrote:
> > Why Generate? Why can't the default constructor just do what most consumers
> will
> > want it to do? We could add a special constructor for creating an empty
Nonce
> > (e.g. over nullptr_t or something)?
> 
> I do like the clarity of explicitly calling Generate(). A different option
would
> be to make the constructor protected. Both solutions are fine by me.

As per the discussions floating around, I have maintained the Generate()
function (in order to not generate random bits when simply reserving memory on
the stack, if it's about to be overwritten immediately after, like in some Mojo
generated code).

I have added DCHECKs and CHECKs in various places to palliate the security
missuses that would come from forgetting to Generate() Nonce.

More on this in the main discussion.

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode34
base/nonce.h:34: // Creates a 0 initiliazed Nonce.
On 2016/09/13 23:19:05, dcheng wrote:
> Nit: Create a zero-initialized Nonce (so it doesn't look like an O)

Done.

https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h#newcode71
base/nonce.h:71: // If base::Nonce is no longer 128 bits, the IPC serialization
logic and Mojo
On 2016/09/13 23:39:48, DaleCurtis wrote:
> Drive-by: Worth moving to C++ file to avoid all includes from running
> static_assert? I.e. in c++ it'll only run once, versus once for each include.

Done.

https://codereview.chromium.org/2333443002/diff/140001/mojo/common/common_cus...
File mojo/common/common_custom_types.typemap (right):

https://codereview.chromium.org/2333443002/diff/140001/mojo/common/common_cus...
mojo/common/common_custom_types.typemap:29:
"mojo.common.mojom.Nonce=base::Nonce",
On 2016/09/13 23:19:06, dcheng wrote:
> If we expect people to pass Nonce by value (and it sounds like we do?), then
> this should be annotated with [copyable_pass_by_value]

From danakj@'s comment earlier in the thread, I think the preferred way is via
const ref.

https://codereview.chromium.org/2333443002/diff/140001/mojo/common/common_cus...
File mojo/common/common_custom_types_struct_traits.h (right):

https://codereview.chromium.org/2333443002/diff/140001/mojo/common/common_cus...
mojo/common/common_custom_types_struct_traits.h:31: static void
SetToNull(base::Nonce* out) { *out = base::Nonce(); }
On 2016/09/13 23:19:06, dcheng wrote:
> Do we actually need to support nullability? I'd suggest only adding this when
> there's a clear need.

Done.

[danakj, 2016-09-14 23:00:29]

On Wed, Sep 14, 2016 at 3:40 PM, <tguilbert@chromium.org> wrote:

> @dcheng Oops, yep, no TypeConverter :P
>
> CHECKs/DCHECKS seem like the way to go :)
>

Thanks!


>
> I have added a CHECK to:
> - ParamTraits<base::Nonce>::Write()
>
- Nonce::Deserialize()
> - NonceHash::operator()
>

Why CHECK and not DCHECK? Use CHECK to track down a bug, or prevent a
security problem. I don't see either here.


>
> And a DCHECK to Nonce::high()/Nonce::low() (although I am less sure about
> the
> usefulness of these two)
>

Ya I don't think these are needed, feels overly paranoid. They may get in
the way of tests also.


>
> I do realize that the CHECK in Deserialize will be hit right before
> ParamTraits<>::Read() or StructTraits<>::Read() would return false. I
> however
> like the idea of having the DCHECK in Deserialize, to cover the
> deserialization
> on Android, after the high/low bits have been send through AIDL.
>
> Does anybody have another suggested way of adding CHECKs? I.e. not having
> the
> CHECK in deserialize and instead letting IPC/Mojo handle the invalid empty
> nonce?
>

Return false seems like enough? We don't want a compromised renderer to
crash the browser, we want the message to be rejected so we kill the
renderer.


>
> I also updated the == operator to use the same logic that is in
> SecureMemEqual.
>
> Thanks!
> Thomas
>
>
> https://codereview.chromium.org/2333443002/diff/140001/base/nonce.h
> File base/nonce.h (right):
>
> https://codereview.chromium.org/2333443002/diff/140001/
> base/nonce.h#newcode17
> base/nonce.h:17: // An Nonce is a cryptographically strong, randomly
> generated, unguessable 128
> On 2016/09/13 23:19:05, dcheng wrote:
> > A Nonce is an unguessable 128-bit token generated from a
> cryptographically
> > strong random source. Nonce should be used when <insert scenarios
> here>.
>
> Done. However, I don't know what all useful scenarios for Nonces are. I
> know they can be used in some authentication schemes, but I think adding
> further uses might be out of scope for the comments of this header file.
>
> https://codereview.chromium.org/2333443002/diff/140001/
> base/nonce.h#newcode20
> base/nonce.h:20: // TO CREATE NEW NONCES, ONLY USE GENERATE()!
> On 2016/09/13 23:19:05, dcheng wrote:
> > Suggested wording:
> >
> > Use Generate() for creating new unguessable nonces. Deserialize() is a
> helper
> > intended only for implementing IPC deserialization.
> >
> > (I think we don't need to repeat the bit about cryptographically
> strong random,
> > since it's such a long phrase =)
>
> TY for the better wording! Done.
>
> https://codereview.chromium.org/2333443002/diff/140001/
> base/nonce.h#newcode26
> base/nonce.h:26: // Generates a unique, strongly random, unguessable
> nonce.
> On 2016/09/13 23:19:05, dcheng wrote:
> > Ditto: "Create an unguessable and unique nonce."
>
> Done.
>
> https://codereview.chromium.org/2333443002/diff/140001/
> base/nonce.h#newcode28
> base/nonce.h:28: static Nonce Generate();
> On 2016/09/13 23:51:53, sandersd wrote:
> > On 2016/09/13 23:21:13, Ken Rockot wrote:
> > > Why Generate? Why can't the default constructor just do what most
> consumers
> > will
> > > want it to do? We could add a special constructor for creating an
> empty Nonce
> > > (e.g. over nullptr_t or something)?
> >
> > I do like the clarity of explicitly calling Generate(). A different
> option would
> > be to make the constructor protected. Both solutions are fine by me.
>
> As per the discussions floating around, I have maintained the Generate()
> function (in order to not generate random bits when simply reserving
> memory on the stack, if it's about to be overwritten immediately after,
> like in some Mojo generated code).
>
> I have added DCHECKs and CHECKs in various places to palliate the
> security missuses that would come from forgetting to Generate() Nonce.
>
> More on this in the main discussion.
>
> https://codereview.chromium.org/2333443002/diff/140001/
> base/nonce.h#newcode34
> base/nonce.h:34: // Creates a 0 initiliazed Nonce.
> On 2016/09/13 23:19:05, dcheng wrote:
> > Nit: Create a zero-initialized Nonce (so it doesn't look like an O)
>
> Done.
>
> https://codereview.chromium.org/2333443002/diff/140001/
> base/nonce.h#newcode71
> base/nonce.h:71: // If base::Nonce is no longer 128 bits, the IPC
> serialization logic and Mojo
> On 2016/09/13 23:39:48, DaleCurtis wrote:
> > Drive-by: Worth moving to C++ file to avoid all includes from running
> > static_assert? I.e. in c++ it'll only run once, versus once for each
> include.
>
> Done.
>
> https://codereview.chromium.org/2333443002/diff/140001/
> mojo/common/common_custom_types.typemap
> File mojo/common/common_custom_types.typemap (right):
>
> https://codereview.chromium.org/2333443002/diff/140001/
> mojo/common/common_custom_types.typemap#newcode29
> mojo/common/common_custom_types.typemap:29:
> "mojo.common.mojom.Nonce=base::Nonce",
> On 2016/09/13 23:19:06, dcheng wrote:
> > If we expect people to pass Nonce by value (and it sounds like we
> do?), then
> > this should be annotated with [copyable_pass_by_value]
>
> From danakj@'s comment earlier in the thread, I think the preferred way
> is via const ref.
>
> https://codereview.chromium.org/2333443002/diff/140001/
> mojo/common/common_custom_types_struct_traits.h
> File mojo/common/common_custom_types_struct_traits.h (right):
>
> https://codereview.chromium.org/2333443002/diff/140001/
> mojo/common/common_custom_types_struct_traits.h#newcode31
> mojo/common/common_custom_types_struct_traits.h:31: static void
> SetToNull(base::Nonce* out) { *out = base::Nonce(); }
> On 2016/09/13 23:19:06, dcheng wrote:
> > Do we actually need to support nullability? I'd suggest only adding
> this when
> > there's a clear need.
>
> Done.
>
> https://codereview.chromium.org/2333443002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sandersd (OOO until July 31), 2016-09-14 23:02:05]

> And a DCHECK to Nonce::high()/Nonce::low() (although I am less sure about the
> usefulness of these two)

I'd actually like to advocate removing these methods, in favor of either friend
classes or a Serialize() method with out parameters. Just to make it clear that
accessing these internals should not be taken lightly. Whatever you choose, they
should have checks, as should the comparison operators.

In the same vein, I don't think is_empty() should be exposed. At the very least
it should be renamed to is_initialized(), but if that feature exists I would
also expect operator bool() to be implemented.

ToString() should have a special case for printing uninitialized Nonces, but I
don't think it should CHECK(), since I imagine that the main use of ToString()
will in fact be CHECK()-style messages.

> I do realize that the CHECK in Deserialize will be hit right before
> ParamTraits<>::Read() or StructTraits<>::Read() would return false. I however
> like the idea of having the DCHECK in Deserialize, to cover the
deserialization
> on Android, after the high/low bits have been send through AIDL.
> 
> Does anybody have another suggested way of adding CHECKs? I.e. not having the
> CHECK in deserialize and instead letting IPC/Mojo handle the invalid empty
> nonce?

I would recommend having the check in both places for now. Then the Mojo code
will be correct and other theoretical Nonce clients at least won't be able to
get it wrong.

[danakj, 2016-09-14 23:11:23]

On Wed, Sep 14, 2016 at 4:02 PM, <sandersd@chromium.org> wrote:

> > And a DCHECK to Nonce::high()/Nonce::low() (although I am less sure
> about the
> > usefulness of these two)
>
> I'd actually like to advocate removing these methods, in favor of either
> friend
> classes or a Serialize() method with out parameters. Just to make it clear
> that
> accessing these internals should not be taken lightly. Whatever you
> choose, they
> should have checks, as should the comparison operators.
>
> In the same vein, I don't think is_empty() should be exposed. At the very
> least
> it should be renamed to is_initialized(), but if that feature exists I
> would
> also expect operator bool() to be implemented.
>

What happens when you write a test with a SurfaceId which is composed of
some things with a Nonce, and you want to test that first some SurfaceId is
not initialized, then you do some steps, and you see the SurfaceId be
populated with a value from the renderer? Being able to see that a Nonce
has not been set seems extremely relevant for tests, or we'll have to
litter the code with bool has_set_nonce_ which isn't particularly useful?

I'm not a security engineer so maybe I'm being super naive, but I feel like
uses of the class (ie Serialization/Deserialization) should be made secure
but being overly paranoid inside the type itself feels wrong.


>
> ToString() should have a special case for printing uninitialized Nonces,
> but I
> don't think it should CHECK(), since I imagine that the main use of
> ToString()
> will in fact be CHECK()-style messages.
>
> > I do realize that the CHECK in Deserialize will be hit right before
> > ParamTraits<>::Read() or StructTraits<>::Read() would return false. I
> however
> > like the idea of having the DCHECK in Deserialize, to cover the
> deserialization
> > on Android, after the high/low bits have been send through AIDL.
> >
> > Does anybody have another suggested way of adding CHECKs? I.e. not
> having the
> > CHECK in deserialize and instead letting IPC/Mojo handle the invalid
> empty
> > nonce?
>
> I would recommend having the check in both places for now. Then the Mojo
> code
> will be correct and other theoretical Nonce clients at least won't be able
> to
> get it wrong.
>
> https://codereview.chromium.org/2333443002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sandersd (OOO until July 31), 2016-09-14 23:30:09]

> What happens when you write a test with a SurfaceId which is composed of
> some things with a Nonce, and you want to test that first some SurfaceId is
> not initialized, then you do some steps, and you see the SurfaceId be
> populated with a value from the renderer? Being able to see that a Nonce
> has not been set seems extremely relevant for tests, or we'll have to
> litter the code with bool has_set_nonce_ which isn't particularly useful?
> 
> I'm not a security engineer so maybe I'm being super naive, but I feel like
> uses of the class (ie Serialization/Deserialization) should be made secure
> but being overly paranoid inside the type itself feels wrong.

It is true that I may be overly paranoid. That said, for this specific case, I
would advocate base::Optional<base::Nonce>. Given that that exists and quite
clear in meaning, I would not want base::Nonce itself to gain similar features.

[danakj, 2016-09-14 23:59:45]

On Wed, Sep 14, 2016 at 4:30 PM, <sandersd@chromium.org> wrote:

> > What happens when you write a test with a SurfaceId which is composed of
> > some things with a Nonce, and you want to test that first some SurfaceId
> is
> > not initialized, then you do some steps, and you see the SurfaceId be
> > populated with a value from the renderer? Being able to see that a Nonce
> > has not been set seems extremely relevant for tests, or we'll have to
> > litter the code with bool has_set_nonce_ which isn't particularly useful?
> >
> > I'm not a security engineer so maybe I'm being super naive, but I feel
> like
> > uses of the class (ie Serialization/Deserialization) should be made
> secure
> > but being overly paranoid inside the type itself feels wrong.
>
> It is true that I may be overly paranoid. That said, for this specific
> case, I
> would advocate base::Optional<base::Nonce>. Given that that exists and
> quite
> clear in meaning, I would not want base::Nonce itself to gain similar
> features.
>

I guess my thought is that Nonce is not going to always (often?) be used on
its own, it is part of some id like SurfaceId. In these cases, we (chromium
in general? c++ developers?) like to make objects that are
default-initialized to empty, so I feel like a majority of uses of Nonce
will end up being wrapped in Optional (including inside of types that we
serialize like SurfaceId). So then the serializer will have to check that
the Nonce exists, instead of checking that it is null. So we gain little
there.

There are other cases like gpu::Mailbox where the Nonce kinda lives on its
own, but it's still a member of gpu::Mailbox, which we for sure create as
empty sometimes, example:
https://cs.chromium.org/chromium/src/cc/resources/texture_mailbox.h?rcl=0&l=42
. So we'd have to use Optional again which means IPCing a mailbox needs to
check if the Nonce exists again.. idk, I just feel like we're going to make
more work for ourselves by ending up with Optional around every Nonce.


>
> https://codereview.chromium.org/2333443002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-09-15 00:03:46]

On Sep 14, 2016 4:51 PM, <danakj@chromium.org> wrote:
>
> On Wed, Sep 14, 2016 at 4:30 PM, <sandersd@chromium.org> wrote:
>>
>> > What happens when you write a test with a SurfaceId which is composed
of
>> > some things with a Nonce, and you want to test that first some
SurfaceId is
>> > not initialized, then you do some steps, and you see the SurfaceId be
>> > populated with a value from the renderer? Being able to see that a
Nonce
>> > has not been set seems extremely relevant for tests, or we'll have to
>> > litter the code with bool has_set_nonce_ which isn't particularly
useful?
>> >
>> > I'm not a security engineer so maybe I'm being super naive, but I feel
like
>> > uses of the class (ie Serialization/Deserialization) should be made
secure
>> > but being overly paranoid inside the type itself feels wrong.
>>
>> It is true that I may be overly paranoid. That said, for this specific
case, I
>> would advocate base::Optional<base::Nonce>. Given that that exists and
quite
>> clear in meaning, I would not want base::Nonce itself to gain similar
features.
>
>
> I guess my thought is that Nonce is not going to always (often?) be used
on its own, it is part of some id like SurfaceId. In these cases, we
(chromium in general? c++ developers?) like to make objects that are
default-initialized to empty, so I feel like a majority of uses of Nonce
will end up being wrapped in Optional (including inside of types that we
serialize like SurfaceId). So then the serializer will have to check that
the Nonce exists, instead of checking that it is null. So we gain little
there.
>
> There are other cases like gpu::Mailbox where the Nonce kinda lives on
its own, but it's still a member of gpu::Mailbox, which we for sure create
as empty sometimes, example:
https://cs.chromium.org/chromium/src/cc/resources/texture_mailbox.h?rcl=0&l=42
. So we'd have to use Optional again which means IPCing a mailbox needs to
check if the Nonce exists again.. idk, I just feel like we're going to make
more work for ourselves by ending up with Optional around every Nonce.
>
>>
>>
>> https://codereview.chromium.org/2333443002/
>
>

FWIW, I agree with danakj here. Nonce shouldn't try to stop people from
using an empty value.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tom Sepez, 2016-09-15 16:55:08]

Ok, sounds like it's more trouble than its worth. Still LGTM.

[danakj, 2016-09-15 18:06:37]

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.cc
File base/nonce.cc (right):

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.cc#newcode40
base/nonce.cc:40: CHECK((high | low));
DCHECK?

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode43
base/nonce.h:43: DCHECK(!is_empty());
I don't think you need to DCHECK these.

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode59
base/nonce.h:59: // Constant time comparison check.
I don't know what you mean by constant time. Comparing 2 things is always
constant time. Do you have data to support this being faster than just writing
high == high && low == low? This looks like trying to write compilers to me.

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode63
base/nonce.h:63: bool operator!=(const Nonce& other) const { return
!operator==(other); }
nit: normally written !(*this == other)

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode70
base/nonce.h:70: uint64_t high_;
can you  "= 0" here and just "=default" the constructor definition?

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode77
base/nonce.h:77: CHECK(!nonce.is_empty());
DCHECK?

Also, you need to include base/logging.h to use this.

[watk, 2016-09-15 18:46:09]

watk@chromium.org changed reviewers:
+ watk@chromium.org

[watk, 2016-09-15 18:46:10]

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.cc
File base/nonce.cc (right):

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.cc#newcode38
base/nonce.cc:38: // Sending an empty nonce accross processes likely means that
across

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode69
base/nonce.h:69: // simpler ToString() and is_null().
drive by: s/is_null/is_empty

[sandersd (OOO until July 31), 2016-09-15 18:52:26]

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode59
base/nonce.h:59: // Constant time comparison check.
On 2016/09/15 18:06:37, danakj wrote:
> I don't know what you mean by constant time. Comparing 2 things is always
> constant time. Do you have data to support this being faster than just writing
> high == high && low == low? This looks like trying to write compilers to me.

I just want to call out here that constant-time does not mean faster; it's a
security property. However, without looking at the generated assembly on every
platform, we can't actually say much about either version of the code, so I
probably wouldn't bother. (It's probably misleading to have that comment
currently.)

[Tom Sepez, 2016-09-15 19:43:36]

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode59
base/nonce.h:59: // Constant time comparison check.
On 2016/09/15 18:06:37, danakj wrote:
> I don't know what you mean by constant time. Comparing 2 things is always
> constant time. Do you have data to support this being faster than just writing
> high == high && low == low? This looks like trying to write compilers to me.

https://en.wikipedia.org/wiki/Timing_attack.  Extremly unlikely since we are not
going byte by byte, but you never know.

[danakj, 2016-09-15 21:09:32]

On Thu, Sep 15, 2016 at 12:43 PM, <tsepez@chromium.org> wrote:

>
> https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h
> File base/nonce.h (right):
>
> https://codereview.chromium.org/2333443002/diff/160001/
> base/nonce.h#newcode59
> base/nonce.h:59: // Constant time comparison check.
> On 2016/09/15 18:06:37, danakj wrote:
> > I don't know what you mean by constant time. Comparing 2 things is
> always
> > constant time. Do you have data to support this being faster than just
> writing
> > high == high && low == low? This looks like trying to write compilers
> to me.
>
> https://en.wikipedia.org/wiki/Timing_attack. Extremly unlikely since we
> are not going byte by byte, but you never know.
>

This should be explained in the comment in a way for non-security experts
to understand then. Now I get what constant-time is trying to say with a
lot more context.


>
> https://codereview.chromium.org/2333443002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tguilbert, 2016-09-15 21:11:45]

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

[commit-bot: I haz the power, 2016-09-15 21:12:39]

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

[dcheng, 2016-09-15 21:14:57]

On 2016/09/15 21:09:32, danakj wrote:
> On Thu, Sep 15, 2016 at 12:43 PM, <mailto:tsepez@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h
> > File base/nonce.h (right):
> >
> > https://codereview.chromium.org/2333443002/diff/160001/
> > base/nonce.h#newcode59
> > base/nonce.h:59: // Constant time comparison check.
> > On 2016/09/15 18:06:37, danakj wrote:
> > > I don't know what you mean by constant time. Comparing 2 things is
> > always
> > > constant time. Do you have data to support this being faster than just
> > writing
> > > high == high && low == low? This looks like trying to write compilers
> > to me.
> >
> > https://en.wikipedia.org/wiki/Timing_attack. Extremly unlikely since we
> > are not going byte by byte, but you never know.
> >
> 
> This should be explained in the comment in a way for non-security experts
> to understand then. Now I get what constant-time is trying to say with a
> lot more context.
> 
> 
> >
> > https://codereview.chromium.org/2333443002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Ideally, we would implement a helper for a constant-time compare somewhere
outside and use it here, then it'd be clear. IDK how hard that would be though.

[commit-bot: I haz the power, 2016-09-15 21:17:28]

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

[commit-bot: I haz the power, 2016-09-15 21:17:30]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tguilbert, 2016-09-15 21:59:11]

On 2016/09/15 21:14:57, dcheng wrote:
> On 2016/09/15 21:09:32, danakj wrote:
> > On Thu, Sep 15, 2016 at 12:43 PM, <mailto:tsepez@chromium.org> wrote:
> > 
> > >
> > > https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h
> > > File base/nonce.h (right):
> > >
> > > https://codereview.chromium.org/2333443002/diff/160001/
> > > base/nonce.h#newcode59
> > > base/nonce.h:59: // Constant time comparison check.
> > > On 2016/09/15 18:06:37, danakj wrote:
> > > > I don't know what you mean by constant time. Comparing 2 things is
> > > always
> > > > constant time. Do you have data to support this being faster than just
> > > writing
> > > > high == high && low == low? This looks like trying to write compilers
> > > to me.
> > >
> > > https://en.wikipedia.org/wiki/Timing_attack. Extremly unlikely since we
> > > are not going byte by byte, but you never know.
> > >
> > 
> > This should be explained in the comment in a way for non-security experts
> > to understand then. Now I get what constant-time is trying to say with a
> > lot more context.
> > 
> > 
> > >
> > > https://codereview.chromium.org/2333443002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Ideally, we would implement a helper for a constant-time compare somewhere
> outside and use it here, then it'd be clear. IDK how hard that would be
though.

There is already SecureMemEqual here:
https://cs.chromium.org/chromium/src/crypto/secure_util.h?q=securememequal&sq...

I am in the process of writing a fuller message to summarize what changed and
why for patch 10.

[tguilbert, 2016-09-15 22:03:51]

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

[commit-bot: I haz the power, 2016-09-15 22:04:24]

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

[tguilbert, 2016-09-15 22:57:40]

After some thought, I have renamed base::Nonce to base::NonceToken.

I think this clears some of the issues that come with the intrinsic meaning
behind a "Nonce". From what I understand, a cryptographic nonce should
"uncompromisingly never ever" be equal to 0. Providing a "base::Nonce", which
could also technically be used in authentication scenarios, means that we have
to guarantee that it cannot be misused.

Writing a "NonceToken" instead allows us to be more flexible with its meaning an
its usage. The only use we have for base::NonceToken right now is make certain
IDs unguessable. It makes it clearer that the class is a use of nonce, and not a
nonce in and of itself.

This means that is is OK to have an empty NonceToken, and to compare and assign
them.

This also however means that, since NonceToken is supposed to identify some
sensitive unguessable resource, it is never OK to send an empty NonceToken
accross processes. There are therefore CHECKs in both the serialization and the
deserialization code.

In the case where a NonceToken may be empty and needs to be sent across
processes, using base::Optional will make it explicitly clear that the "null"
scenario is acceptable and should be separately handled.

Thoughts?

Thanks,
Thomas

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.cc
File base/nonce.cc (right):

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.cc#newcode38
base/nonce.cc:38: // Sending an empty nonce accross processes likely means that
On 2016/09/15 18:46:10, watk wrote:
> across

Done.

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.cc#newcode40
base/nonce.cc:40: CHECK((high | low));
On 2016/09/15 18:06:36, danakj wrote:
> DCHECK?

I think this CHECK is important. There is no case where deserializing a zeroed
out NonceToken is a valid scenario. The IPC ParamTraits and Mojo StructTraits
are now checking to see if high/low are both 0, and return false instead of
calling Deserialize, which means we shouldn't run into this CHECK.

If someone planned on creating an intentional empty NonceToken, NonceToken() is
the way to go.

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h
File base/nonce.h (right):

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode43
base/nonce.h:43: DCHECK(!is_empty());
On 2016/09/15 18:06:37, danakj wrote:
> I don't think you need to DCHECK these.

Removed in newest iteration.

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode59
base/nonce.h:59: // Constant time comparison check.
On 2016/09/15 19:43:36, Tom Sepez wrote:
> On 2016/09/15 18:06:37, danakj wrote:
> > I don't know what you mean by constant time. Comparing 2 things is always
> > constant time. Do you have data to support this being faster than just
writing
> > high == high && low == low? This looks like trying to write compilers to me.
> 
> https://en.wikipedia.org/wiki/Timing_attack.  Extremly unlikely since we are
not
> going byte by byte, but you never know.

Removed the comment. Leaving the equality check as is unless someone else has
any other comments.

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode63
base/nonce.h:63: bool operator!=(const Nonce& other) const { return
!operator==(other); }
On 2016/09/15 18:06:37, danakj wrote:
> nit: normally written !(*this == other)

Done.

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode69
base/nonce.h:69: // simpler ToString() and is_null().
On 2016/09/15 18:46:10, watk wrote:
> drive by: s/is_null/is_empty

Done.

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode70
base/nonce.h:70: uint64_t high_;
On 2016/09/15 18:06:37, danakj wrote:
> can you  "= 0" here and just "=default" the constructor definition?

Done.

https://codereview.chromium.org/2333443002/diff/160001/base/nonce.h#newcode77
base/nonce.h:77: CHECK(!nonce.is_empty());
On 2016/09/15 18:06:36, danakj wrote:
> DCHECK?
> 
> Also, you need to include base/logging.h to use this.

Done.

[tguilbert, 2016-09-15 23:07:32]

Description was changed from

==========
Add base::Nonce

cc::SurfaceId, gpu::Mailbox and ScopedSurfaceRequestManager (currently
blocked on this CL) need an unguessable identifier. Security recommends
using 128 bits to consider an ID to be unguessable. However, there is no
conveniently serializable way to represent 128 bits.

This change introduces base::Nonce, a 128 bit struct with a
cryptographically strong Generate() function. The change also introduces
ParamTraits and a custom Mojo type map, to easily serialize the Nonce
over Mojo or IPC.

TEST=Added unittests. Also tested in a prototype that uses IPC and Mojo.
BUG=643857
==========

to

==========
Add base::NonceToken

cc::SurfaceId, gpu::Mailbox and ScopedSurfaceRequestManager need an 
unguessable identifier. Security recommends using 128 bits to make sure
an ID is unguessable. However, there is no conveniently serializable
way to represent 128 bits.

This change introduces base::NonceToken, a 128 bit class with a
cryptographically strong Create() function. NonceTokens can be used by
themselves, or as part of an aggregate ID.

An empty NonceToken is a valid value. This means the NonceToken is not
in and of itself a cryptographic nonce. It is however illegal to send
empty NonceToken across processes (because the resource that is
supposed to be protected by the token would now be guessable).

This change also introduces the appropriate code to send NonceTokens
over IPC and Mojo. Trying to serialize/deserialize an empty NonceToken
will result in CHECKs and failures. base::Optional should be used in
cases where it may be valid to return no token.

TEST=Added unittests. Also tested in a prototype that uses IPC and Mojo.
BUG=643857
==========

[danakj, 2016-09-15 23:35:16]

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc
File base/nonce_token.cc (right):

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc#ne...
base/nonce_token.cc:15: static_assert(sizeof(NonceToken) == 2 *
sizeof(uint64_t),
Can you put this assert at the site(s) of the code that would need to be updated
instead?

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc#ne...
base/nonce_token.cc:35: CHECK(!is_empty());
Why not DCHECK?

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc#ne...
base/nonce_token.cc:44: CHECK(!NonceToken::IsZeroData(high, low));
I don't think CHECK is appropriate here, it means a misbehaving renderer can
crash the whole browser. When deserializing, if the message is invalid we should
just fail to deserialize it and destroy the misbehaving process.

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.h
File base/nonce_token.h (right):

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.h#new...
base/nonce_token.h:35: // USE ONLY THIS FUNCTION TO GENERATE NEW NONCES.
I'd prefer less capslock in comments. The comment above already calls out this
method as the place to call too. I think you can drop this whole capslock part.

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.h#new...
base/nonce_token.h:39: // NOTE: The NonceToken still needs to be initialized via
Create().
How about // Creates an empty NonceToken. Assign to it with Create() before
using it.

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.h#new...
base/nonce_token.h:51: static NonceToken Deserialize(uint64_t high, uint64_t
low);
It's easier to read if the static methods are grouped, can you put this with
Create()

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.h#new...
base/nonce_token.h:54: static inline bool IsZeroData(uint64_t high, uint64_t
low) {
Can be private? Why isn't it used for is_empty() as well?

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.h#new...
base/nonce_token.h:69: return !((high_ ^ other.high_) | (low_ ^ other.low_));
Either just do high_==other.high_ && low==other.low_ or explain this with a
comment (in a widely understandable way).

[tguilbert, 2016-09-16 01:03:02]

PTAL.

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc
File base/nonce_token.cc (right):

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc#ne...
base/nonce_token.cc:15: static_assert(sizeof(NonceToken) == 2 *
sizeof(uint64_t),
On 2016/09/15 23:35:15, danakj wrote:
> Can you put this assert at the site(s) of the code that would need to be
updated
> instead?

Done.

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc#ne...
base/nonce_token.cc:35: CHECK(!is_empty());
On 2016/09/15 23:35:16, danakj wrote:
> Why not DCHECK?

Empty NonceTokens can be copied and assigned normally within a process. If
Serialize is called, it is only for the IPC/Mojo/AIDL case. If a process tries
to serialize an empty NonceToken, it means the resource that is meant to be
protected by the unguessability of the NonceToken is not actually being
protected. I think this is a security issue, and that CHECK should therefore be
used here.

WDYT?

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc#ne...
base/nonce_token.cc:44: CHECK(!NonceToken::IsZeroData(high, low));
On 2016/09/15 23:35:16, danakj wrote:
> I don't think CHECK is appropriate here, it means a misbehaving renderer can
> crash the whole browser. When deserializing, if the message is invalid we
should
> just fail to deserialize it and destroy the misbehaving process.

The IPC and the Mojo deserialization code use NonceToken::IsZeroData() to make
sure the given bits are not equal to zero before calling deserialize. If
high/low are both equal to 0 in the IPC and the Mojo case,
StructTraits::Read()/ParamTraits::Read() return false, and never calls
Deserialize.

In Mojo's case, this prompts the browser process to kill the misbehaving
renderer.(https://groups.google.com/a/chromium.org/d/msg/chromium-mojo/_Dn9YV9ghxs/S2tZ0mnwBAAJ)

In IPC's case, I do not have a clear cut comment to link to, (but I assume it is
the same).
@tsepez: Is returning false from ParamTraits::Read() sufficient?


I added a comment to Deserialize() in the .h saying that IsZeroData() should be
used to validate the data in cases where we can gracefully fail the
deserialization.

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.h
File base/nonce_token.h (right):

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.h#new...
base/nonce_token.h:35: // USE ONLY THIS FUNCTION TO GENERATE NEW NONCES.
On 2016/09/15 23:35:16, danakj wrote:
> I'd prefer less capslock in comments. The comment above already calls out this
> method as the place to call too. I think you can drop this whole capslock
part.

Done.

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.h#new...
base/nonce_token.h:39: // NOTE: The NonceToken still needs to be initialized via
Create().
On 2016/09/15 23:35:16, danakj wrote:
> How about // Creates an empty NonceToken. Assign to it with Create() before
> using it.

Nicely worded, thank you! Done.

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.h#new...
base/nonce_token.h:51: static NonceToken Deserialize(uint64_t high, uint64_t
low);
On 2016/09/15 23:35:16, danakj wrote:
> It's easier to read if the static methods are grouped, can you put this with
> Create()

Done.

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.h#new...
base/nonce_token.h:54: static inline bool IsZeroData(uint64_t high, uint64_t
low) {
On 2016/09/15 23:35:16, danakj wrote:
> Can be private? Why isn't it used for is_empty() as well?

I am using it in the IPC and Mojo deserialization code before actually calling
NonceToken::Deserialize, and therefore it can't be private.

And yes, I updated it to be used in is_empty(). (This was Leftover from a local
commit I had where I renamed is_empty() to is_initialized())

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.h#new...
base/nonce_token.h:69: return !((high_ ^ other.high_) | (low_ ^ other.low_));
On 2016/09/15 23:35:16, danakj wrote:
> Either just do high_==other.high_ && low==other.low_ or explain this with a
> comment (in a widely understandable way).

I think SecureMemEqual's comments give good context, so I added a  pointer to
those comments.

[danakj, 2016-09-16 01:21:55]

Thanks, few more comments.

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc
File base/nonce_token.cc (right):

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc#ne...
base/nonce_token.cc:35: CHECK(!is_empty());
On 2016/09/16 01:03:01, ThomasGuilbert wrote:
> On 2016/09/15 23:35:16, danakj wrote:
> > Why not DCHECK?
> 
> Empty NonceTokens can be copied and assigned normally within a process. If
> Serialize is called, it is only for the IPC/Mojo/AIDL case. If a process tries
> to serialize an empty NonceToken, it means the resource that is meant to be
> protected by the unguessability of the NonceToken is not actually being
> protected. I think this is a security issue, and that CHECK should therefore
be
> used here.
> 
> WDYT?

You're right it's a security issue but you can't trust an untrusted process to
do anything right anyway, so I think using CHECK here is misleading. The
validation should happen at the time of deserialization, which happens in a
trusted place.

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc#ne...
base/nonce_token.cc:44: CHECK(!NonceToken::IsZeroData(high, low));
On 2016/09/16 01:03:01, ThomasGuilbert wrote:
> On 2016/09/15 23:35:16, danakj wrote:
> > I don't think CHECK is appropriate here, it means a misbehaving renderer can
> > crash the whole browser. When deserializing, if the message is invalid we
> should
> > just fail to deserialize it and destroy the misbehaving process.
> 
> The IPC and the Mojo deserialization code use NonceToken::IsZeroData() to make
> sure the given bits are not equal to zero before calling deserialize. If
> high/low are both equal to 0 in the IPC and the Mojo case,
> StructTraits::Read()/ParamTraits::Read() return false, and never calls
> Deserialize.
> 
> In Mojo's case, this prompts the browser process to kill the misbehaving
>
renderer.(https://groups.google.com/a/chromium.org/d/msg/chromium-mojo/_Dn9YV9ghxs/S2tZ0mnwBAAJ)
> 
> In IPC's case, I do not have a clear cut comment to link to, (but I assume it
is
> the same).
> @tsepez: Is returning false from ParamTraits::Read() sufficient?
> 
> 
> I added a comment to Deserialize() in the .h saying that IsZeroData() should
be
> used to validate the data in cases where we can gracefully fail the
> deserialization.

That sounds good, but then I don't think you need this CHECK. You can DCHECK to
verify that this does indeed not happen, but please don't overuse CHECK.
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2333443002/diff/240001/base/nonce_token.h
File base/nonce_token.h (right):

https://codereview.chromium.org/2333443002/diff/240001/base/nonce_token.h#new...
base/nonce_token.h:49: static inline bool IsZeroData(uint64_t high, uint64_t
low) {
I think this should not be part of this api, here's my thoughts:

If you need a check that is constant-time because you're worried an attacker may
use the timing of the result (they'd have to do this in a tight loop or where
they have control of the system right before and after the check I guess
right?), you can do this check by doing == NonceToken() already, so it's
redundant with that.

The deserialization code is going to be running after performing a whole lot of
other process/thread/something hopping. So I don't see how timing attacks are
relevant there and they can just check (high&&low), which really doesn't need a
helper. But please let me know if that isn't the case.

TBH I'm not sure where you're worried about timing attacks for is_empty() either
and I would just write it the most naive way.

I can kinda see an argument for operator==, this is something that is more
likely to appear in code outside of IPC, and could be used to fish for a
matching NonceToken with brute force.

is_empty() doesn't satisfy those concerns tho, right?

https://codereview.chromium.org/2333443002/diff/240001/base/nonce_token.h#new...
base/nonce_token.h:72: // See crypto::SecureMemEqual()'s comments for more
context.
Thanks for this pointer.

[Tom Sepez, 2016-09-16 16:02:59]

Deserialization should just return false on error, the browser then invokes its
bad message path typically killing the renderer, rather than asserting in the
browser process.

Bikeshed: Nonce implies something sent only once to prevent replays; since we
are passing this back and forth a number of times what we have isn't so much a
nonce but a token, just a crytopgraphically random one.  SecureToken? 
UnguessableToken?

[Tom Sepez, 2016-09-16 16:05:41]

On 2016/09/16 16:02:59, Tom Sepez wrote:
> Deserialization should just return false on error, the browser then invokes
its
> bad message path typically killing the renderer, rather than asserting in the
> browser process.
> 
> Bikeshed: Nonce implies something sent only once to prevent replays; since we
> are passing this back and forth a number of times what we have isn't so much a
> nonce but a token, just a crytopgraphically random one.  SecureToken? 
> UnguessableToken?

There's also some precedent for calling an "unforgeable object reference" a
capability, but that term is widely overloaded and I'd not suggest using it:
https://en.wikipedia.org/wiki/Object-capability_model

[Tom Sepez, 2016-09-16 16:07:18]

On 2016/09/16 16:05:41, Tom Sepez wrote:
> On 2016/09/16 16:02:59, Tom Sepez wrote:
> > Deserialization should just return false on error, the browser then invokes
> its
> > bad message path typically killing the renderer, rather than asserting in
the
> > browser process.
> > 
> > Bikeshed: Nonce implies something sent only once to prevent replays; since
we
> > are passing this back and forth a number of times what we have isn't so much
a
> > nonce but a token, just a crytopgraphically random one.  SecureToken? 
> > UnguessableToken?
> 
> There's also some precedent for calling an "unforgeable object reference" a
> capability, but that term is widely overloaded and I'd not suggest using it:
> https://en.wikipedia.org/wiki/Object-capability_model

https://en.wikipedia.org/wiki/Capability-based_security uses the phrase
"unforgeable token"

[danakj, 2016-09-16 17:54:19]

On Fri, Sep 16, 2016 at 9:02 AM, <tsepez@chromium.org> wrote:

> Deserialization should just return false on error, the browser then
> invokes its
> bad message path typically killing the renderer, rather than asserting in
> the
> browser process.
>
> Bikeshed: Nonce implies something sent only once to prevent replays; since
> we
> are passing this back and forth a number of times what we have isn't so
> much a
> nonce but a token, just a crytopgraphically random one. SecureToken?
> UnguessableToken?
>

FWIW If I had to name it I'd go with UnguessableToken.


>
>
> https://codereview.chromium.org/2333443002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Fady Samuel, 2016-09-16 17:56:24]

+1 to UnguessableToken!

On 2016/09/16 17:54:19, danakj wrote:
> On Fri, Sep 16, 2016 at 9:02 AM, <mailto:tsepez@chromium.org> wrote:
> 
> > Deserialization should just return false on error, the browser then
> > invokes its
> > bad message path typically killing the renderer, rather than asserting in
> > the
> > browser process.
> >
> > Bikeshed: Nonce implies something sent only once to prevent replays; since
> > we
> > are passing this back and forth a number of times what we have isn't so
> > much a
> > nonce but a token, just a crytopgraphically random one. SecureToken?
> > UnguessableToken?
> >
> 
> FWIW If I had to name it I'd go with UnguessableToken.
> 
> 
> >
> >
> > https://codereview.chromium.org/2333443002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[tguilbert, 2016-09-16 18:50:16]

On 2016/09/16 17:56:24, Fady Samuel wrote:
> +1 to UnguessableToken!
> 
> On 2016/09/16 17:54:19, danakj wrote:
> > On Fri, Sep 16, 2016 at 9:02 AM, <mailto:tsepez@chromium.org> wrote:
> > 
> > > Deserialization should just return false on error, the browser then
> > > invokes its
> > > bad message path typically killing the renderer, rather than asserting in
> > > the
> > > browser process.
> > >
> > > Bikeshed: Nonce implies something sent only once to prevent replays; since
> > > we
> > > are passing this back and forth a number of times what we have isn't so
> > > much a
> > > nonce but a token, just a crytopgraphically random one. SecureToken?
> > > UnguessableToken?
> > >
> > 
> > FWIW If I had to name it I'd go with UnguessableToken.
> > 
> > 
> > >
> > >
> > > https://codereview.chromium.org/2333443002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

I will change the name to UnguessableToken :)

[tguilbert, 2016-09-16 21:46:27]

Description was changed from

==========
Add base::NonceToken

cc::SurfaceId, gpu::Mailbox and ScopedSurfaceRequestManager need an 
unguessable identifier. Security recommends using 128 bits to make sure
an ID is unguessable. However, there is no conveniently serializable
way to represent 128 bits.

This change introduces base::NonceToken, a 128 bit class with a
cryptographically strong Create() function. NonceTokens can be used by
themselves, or as part of an aggregate ID.

An empty NonceToken is a valid value. This means the NonceToken is not
in and of itself a cryptographic nonce. It is however illegal to send
empty NonceToken across processes (because the resource that is
supposed to be protected by the token would now be guessable).

This change also introduces the appropriate code to send NonceTokens
over IPC and Mojo. Trying to serialize/deserialize an empty NonceToken
will result in CHECKs and failures. base::Optional should be used in
cases where it may be valid to return no token.

TEST=Added unittests. Also tested in a prototype that uses IPC and Mojo.
BUG=643857
==========

to

==========
Add base::UnguessableToken

cc::SurfaceId, gpu::Mailbox and ScopedSurfaceRequestManager need an 
unguessable identifier. Security recommends using 128 bits to make sure
an ID is unguessable. However, there is no conveniently serializable
way to represent 128 bits.

This change introduces base::UnguessableToken, a 128 bit class with a
cryptographically strong Create() function. UnguessableToken can be
used by themselves, or as part of an aggregate ID.

An empty UnguessableToken is a valid value. It is however illegal to
send empty UnguessableToken across processes (because the resource that
is supposed to be protected by the token would now be guessable).
Sending empty tokens across processes is a security issue, and should
be handled as such.

This change also introduces the appropriate code to send tokens
over IPC and Mojo. base::Optional should be used in cases where it may
be valid to send no token (rather than sending an empty token).

TEST=Added unittests. Also tested in a prototype that uses IPC and Mojo.
BUG=643857
==========

[tguilbert, 2016-09-16 21:51:12]

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

[commit-bot: I haz the power, 2016-09-16 21:51:40]

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

[Tom Sepez, 2016-09-16 22:02:14]

I'm totally happy.  LGTM++.

[sandersd (OOO until July 31), 2016-09-16 22:13:55]

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token.cc
File base/unguessable_token.cc (right):

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token...
base/unguessable_token.cc:17: return base::StringPrintf("(%" PRIu64 ":%" PRIu64
")", high_, low_);
I would suggest 0-padded hex rather than exposing the internal structure.

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token.h
File base/unguessable_token.h (right):

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token...
base/unguessable_token.h:64: // Based on crypto::SecureMemEqual().
I am still opposed to suggesting that the implementation of this class has this
property, especially sine even if this method works, the property is lost as
soon as you use it as a map or hash key.

[tguilbert, 2016-09-16 22:16:40]

After a few offline discussions, I have changed all CHECKs to DCHECKs.

Following comments and discussion, I have updated the name to UnguessableToken
(which has gone full circle, because it was the original name I had before
changing it to Nonce :P)

PTAL. Thank you everyone for the comments and discussion!

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc
File base/nonce_token.cc (right):

https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc#ne...
base/nonce_token.cc:35: CHECK(!is_empty());
On 2016/09/16 01:21:55, danakj wrote:
> On 2016/09/16 01:03:01, ThomasGuilbert wrote:
> > On 2016/09/15 23:35:16, danakj wrote:
> > > Why not DCHECK?
> > 
> > Empty NonceTokens can be copied and assigned normally within a process. If
> > Serialize is called, it is only for the IPC/Mojo/AIDL case. If a process
tries
> > to serialize an empty NonceToken, it means the resource that is meant to be
> > protected by the unguessability of the NonceToken is not actually being
> > protected. I think this is a security issue, and that CHECK should therefore
> be
> > used here.
> > 
> > WDYT?
> 
> You're right it's a security issue but you can't trust an untrusted process to
> do anything right anyway, so I think using CHECK here is misleading. The
> validation should happen at the time of deserialization, which happens in a
> trusted place.

Ah, I understand the point you are making.

My concern however was more along the lines of the Browser --> Renderer path.
If, due to some edge case, some ID that is meant to be unguessable is never
initialized in the Browser process, then the serialization is the only place
where we can perform validations in a trusted place. If an empty ID is
mistakenly sent by the browser to a valid Renderer, I feel like this will be
hard to debug without a stack trace that shows the browser is the culprit
(because, any valid Renderer that deserializes the 0 token that the browser sent
would then be killed, from what I understand).

That being said, it is a DCHECK now.

https://codereview.chromium.org/2333443002/diff/240001/base/nonce_token.h
File base/nonce_token.h (right):

https://codereview.chromium.org/2333443002/diff/240001/base/nonce_token.h#new...
base/nonce_token.h:49: static inline bool IsZeroData(uint64_t high, uint64_t
low) {
On 2016/09/16 01:21:55, danakj wrote:
> I think this should not be part of this api, here's my thoughts:
> 
> If you need a check that is constant-time because you're worried an attacker
may
> use the timing of the result (they'd have to do this in a tight loop or where
> they have control of the system right before and after the check I guess
> right?), you can do this check by doing == NonceToken() already, so it's
> redundant with that.
> 
> The deserialization code is going to be running after performing a whole lot
of
> other process/thread/something hopping. So I don't see how timing attacks are
> relevant there and they can just check (high&&low), which really doesn't need
a
> helper. But please let me know if that isn't the case.
> 
> TBH I'm not sure where you're worried about timing attacks for is_empty()
either
> and I would just write it the most naive way.
> 
> I can kinda see an argument for operator==, this is something that is more
> likely to appear in code outside of IPC, and could be used to fish for a
> matching NonceToken with brute force.
> 
> is_empty() doesn't satisfy those concerns tho, right?

I was not concerned about timing attacks with this function. I used it a
syntactic sugar (that also brought attention to the importance of validating the
data) in the IPC and Mojo deserialization code.

Now that there is no CHECK in deserialize (and therefore no need to pre-validate
the data), I can construct the UnguessableToken, and then check for is_empty()
(and return false if there is an issue).

I updated the comments above deserialize to mention that the empty case should
be handled.

[danakj, 2016-09-16 22:18:32]

LGTM too thanks for all the iterations

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token...
File base/unguessable_token_unittest.cc (right):

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token...
base/unguessable_token_unittest.cc:74:
TestSmallerThanOperator(UnguessableToken::Deserialize(0, 1),
Use SCOPED_TRACE for each call to TestSmallerThanOperator so we can tell which
one caused a failure if they fail.

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token...
base/unguessable_token_unittest.cc:96: EXPECT_EQ(base::HashInts64(high, low),
UnguessableTokenHash{}(token));
nit: UnguessableTokenHash()(token)

(chromium-dev has asked that we use {} for constructing containers with their
contents, and () for calling other constructors)

[danakj, 2016-09-16 22:20:24]

On 2016/09/16 22:16:40, ThomasGuilbert wrote:
> After a few offline discussions, I have changed all CHECKs to DCHECKs.
> 
> Following comments and discussion, I have updated the name to UnguessableToken
> (which has gone full circle, because it was the original name I had before
> changing it to Nonce :P)
> 
> PTAL. Thank you everyone for the comments and discussion!
> 
> https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc
> File base/nonce_token.cc (right):
> 
>
https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc#ne...
> base/nonce_token.cc:35: CHECK(!is_empty());
> On 2016/09/16 01:21:55, danakj wrote:
> > On 2016/09/16 01:03:01, ThomasGuilbert wrote:
> > > On 2016/09/15 23:35:16, danakj wrote:
> > > > Why not DCHECK?
> > > 
> > > Empty NonceTokens can be copied and assigned normally within a process. If
> > > Serialize is called, it is only for the IPC/Mojo/AIDL case. If a process
> tries
> > > to serialize an empty NonceToken, it means the resource that is meant to
be
> > > protected by the unguessability of the NonceToken is not actually being
> > > protected. I think this is a security issue, and that CHECK should
therefore
> > be
> > > used here.
> > > 
> > > WDYT?
> > 
> > You're right it's a security issue but you can't trust an untrusted process
to
> > do anything right anyway, so I think using CHECK here is misleading. The
> > validation should happen at the time of deserialization, which happens in a
> > trusted place.
> 
> Ah, I understand the point you are making.
> 
> My concern however was more along the lines of the Browser --> Renderer path.
> If, due to some edge case, some ID that is meant to be unguessable is never
> initialized in the Browser process, then the serialization is the only place
> where we can perform validations in a trusted place. If an empty ID is
> mistakenly sent by the browser to a valid Renderer, I feel like this will be
> hard to debug without a stack trace that shows the browser is the culprit
> (because, any valid Renderer that deserializes the 0 token that the browser
sent
> would then be killed, from what I understand).
> 
> That being said, it is a DCHECK now.
> 
> https://codereview.chromium.org/2333443002/diff/240001/base/nonce_token.h
> File base/nonce_token.h (right):
> 
>
https://codereview.chromium.org/2333443002/diff/240001/base/nonce_token.h#new...
> base/nonce_token.h:49: static inline bool IsZeroData(uint64_t high, uint64_t
> low) {
> On 2016/09/16 01:21:55, danakj wrote:
> > I think this should not be part of this api, here's my thoughts:
> > 
> > If you need a check that is constant-time because you're worried an attacker
> may
> > use the timing of the result (they'd have to do this in a tight loop or
where
> > they have control of the system right before and after the check I guess
> > right?), you can do this check by doing == NonceToken() already, so it's
> > redundant with that.
> > 
> > The deserialization code is going to be running after performing a whole lot
> of
> > other process/thread/something hopping. So I don't see how timing attacks
are
> > relevant there and they can just check (high&&low), which really doesn't
need
> a
> > helper. But please let me know if that isn't the case.
> > 
> > TBH I'm not sure where you're worried about timing attacks for is_empty()
> either
> > and I would just write it the most naive way.
> > 
> > I can kinda see an argument for operator==, this is something that is more
> > likely to appear in code outside of IPC, and could be used to fish for a
> > matching NonceToken with brute force.
> > 
> > is_empty() doesn't satisfy those concerns tho, right?
> 
> I was not concerned about timing attacks with this function. I used it a
> syntactic sugar (that also brought attention to the importance of validating
the
> data) in the IPC and Mojo deserialization code.
> 
> Now that there is no CHECK in deserialize (and therefore no need to
pre-validate
> the data), I can construct the UnguessableToken, and then check for is_empty()
> (and return false if there is an issue).

I don't agree with this really. The serialization code should check that high
and low are non-zero before calling Deserialize. Don't write code that can
DCHECK, they are for verifying that we didn't write code that can do it.

> I updated the comments above deserialize to mention that the empty case should
> be handled.

[danakj, 2016-09-16 22:21:35]

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token.h
File base/unguessable_token.h (right):

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token...
base/unguessable_token.h:64: // Based on crypto::SecureMemEqual().
On 2016/09/16 22:13:55, sandersd wrote:
> I am still opposed to suggesting that the implementation of this class has
this
> property, especially sine even if this method works, the property is lost as
> soon as you use it as a map or hash key.

I'm super fine with just doing high==other.high&&low==other.low too if that's
preferred.

[tguilbert, 2016-09-16 22:48:15]

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token.cc
File base/unguessable_token.cc (right):

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token...
base/unguessable_token.cc:17: return base::StringPrintf("(%" PRIu64 ":%" PRIu64
")", high_, low_);
On 2016/09/16 22:13:55, sandersd wrote:
> I would suggest 0-padded hex rather than exposing the internal structure.

Done and added UT.

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token.h
File base/unguessable_token.h (right):

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token...
base/unguessable_token.h:64: // Based on crypto::SecureMemEqual().
On 2016/09/16 22:21:35, danakj wrote:
> On 2016/09/16 22:13:55, sandersd wrote:
> > I am still opposed to suggesting that the implementation of this class has
> this
> > property, especially sine even if this method works, the property is lost as
> > soon as you use it as a map or hash key.
> 
> I'm super fine with just doing high==other.high&&low==other.low too if that's
> preferred.

Done.

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token...
File base/unguessable_token_unittest.cc (right):

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token...
base/unguessable_token_unittest.cc:74:
TestSmallerThanOperator(UnguessableToken::Deserialize(0, 1),
On 2016/09/16 22:18:32, danakj wrote:
> Use SCOPED_TRACE for each call to TestSmallerThanOperator so we can tell which
> one caused a failure if they fail.

Done.

https://codereview.chromium.org/2333443002/diff/260001/base/unguessable_token...
base/unguessable_token_unittest.cc:96: EXPECT_EQ(base::HashInts64(high, low),
UnguessableTokenHash{}(token));
On 2016/09/16 22:18:32, danakj wrote:
> nit: UnguessableTokenHash()(token)
> 
> (chromium-dev has asked that we use {} for constructing containers with their
> contents, and () for calling other constructors)

Done.

[tguilbert, 2016-09-16 22:49:36]

On 2016/09/16 22:20:24, danakj wrote:
> On 2016/09/16 22:16:40, ThomasGuilbert wrote:
> > After a few offline discussions, I have changed all CHECKs to DCHECKs.
> > 
> > Following comments and discussion, I have updated the name to
UnguessableToken
> > (which has gone full circle, because it was the original name I had before
> > changing it to Nonce :P)
> > 
> > PTAL. Thank you everyone for the comments and discussion!
> > 
> > https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc
> > File base/nonce_token.cc (right):
> > 
> >
>
https://codereview.chromium.org/2333443002/diff/220001/base/nonce_token.cc#ne...
> > base/nonce_token.cc:35: CHECK(!is_empty());
> > On 2016/09/16 01:21:55, danakj wrote:
> > > On 2016/09/16 01:03:01, ThomasGuilbert wrote:
> > > > On 2016/09/15 23:35:16, danakj wrote:
> > > > > Why not DCHECK?
> > > > 
> > > > Empty NonceTokens can be copied and assigned normally within a process.
If
> > > > Serialize is called, it is only for the IPC/Mojo/AIDL case. If a process
> > tries
> > > > to serialize an empty NonceToken, it means the resource that is meant to
> be
> > > > protected by the unguessability of the NonceToken is not actually being
> > > > protected. I think this is a security issue, and that CHECK should
> therefore
> > > be
> > > > used here.
> > > > 
> > > > WDYT?
> > > 
> > > You're right it's a security issue but you can't trust an untrusted
process
> to
> > > do anything right anyway, so I think using CHECK here is misleading. The
> > > validation should happen at the time of deserialization, which happens in
a
> > > trusted place.
> > 
> > Ah, I understand the point you are making.
> > 
> > My concern however was more along the lines of the Browser --> Renderer
path.
> > If, due to some edge case, some ID that is meant to be unguessable is never
> > initialized in the Browser process, then the serialization is the only place
> > where we can perform validations in a trusted place. If an empty ID is
> > mistakenly sent by the browser to a valid Renderer, I feel like this will be
> > hard to debug without a stack trace that shows the browser is the culprit
> > (because, any valid Renderer that deserializes the 0 token that the browser
> sent
> > would then be killed, from what I understand).
> > 
> > That being said, it is a DCHECK now.
> > 
> > https://codereview.chromium.org/2333443002/diff/240001/base/nonce_token.h
> > File base/nonce_token.h (right):
> > 
> >
>
https://codereview.chromium.org/2333443002/diff/240001/base/nonce_token.h#new...
> > base/nonce_token.h:49: static inline bool IsZeroData(uint64_t high, uint64_t
> > low) {
> > On 2016/09/16 01:21:55, danakj wrote:
> > > I think this should not be part of this api, here's my thoughts:
> > > 
> > > If you need a check that is constant-time because you're worried an
attacker
> > may
> > > use the timing of the result (they'd have to do this in a tight loop or
> where
> > > they have control of the system right before and after the check I guess
> > > right?), you can do this check by doing == NonceToken() already, so it's
> > > redundant with that.
> > > 
> > > The deserialization code is going to be running after performing a whole
lot
> > of
> > > other process/thread/something hopping. So I don't see how timing attacks
> are
> > > relevant there and they can just check (high&&low), which really doesn't
> need
> > a
> > > helper. But please let me know if that isn't the case.
> > > 
> > > TBH I'm not sure where you're worried about timing attacks for is_empty()
> > either
> > > and I would just write it the most naive way.
> > > 
> > > I can kinda see an argument for operator==, this is something that is more
> > > likely to appear in code outside of IPC, and could be used to fish for a
> > > matching NonceToken with brute force.
> > > 
> > > is_empty() doesn't satisfy those concerns tho, right?
> > 
> > I was not concerned about timing attacks with this function. I used it a
> > syntactic sugar (that also brought attention to the importance of validating
> the
> > data) in the IPC and Mojo deserialization code.
> > 
> > Now that there is no CHECK in deserialize (and therefore no need to
> pre-validate
> > the data), I can construct the UnguessableToken, and then check for
is_empty()
> > (and return false if there is an issue).
> 
> I don't agree with this really. The serialization code should check that high
> and low are non-zero before calling Deserialize. Don't write code that can
> DCHECK, they are for verifying that we didn't write code that can do it.
> 
> > I updated the comments above deserialize to mention that the empty case
should
> > be handled.

You are right. I explicitly check for the high == 0 && low == 0 now.

[danakj, 2016-09-16 22:52:46]

Thanks! LGTM

https://codereview.chromium.org/2333443002/diff/280001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2333443002/diff/280001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:1033: return false;
please "git cl format" there's a couple style errors

[sandersd (OOO until July 31), 2016-09-16 22:56:59]

lgtm

[dcheng, 2016-09-16 23:01:36]

https://codereview.chromium.org/2333443002/diff/280001/mojo/common/common_cus...
File mojo/common/common_custom_types_struct_traits.h (right):

https://codereview.chromium.org/2333443002/diff/280001/mojo/common/common_cus...
mojo/common/common_custom_types_struct_traits.h:38: token.Serialize(&high,
&low);
So, for Reasons (tm), each getter here is actually called twice during mojo
serialization. So this is a bit inefficient.

(We usually out-of-line serialization methods that aren't just calling simple
getters as well.)

I know sandersd@ advocated for removing the getters earlier... but for Mojo
purposes, we should probably just expose them.

danakj@, can you think of any clever tricks here so we would limit who the
getters are exposed to?

[danakj, 2016-09-16 23:26:51]

https://codereview.chromium.org/2333443002/diff/280001/mojo/common/common_cus...
File mojo/common/common_custom_types_struct_traits.h (right):

https://codereview.chromium.org/2333443002/diff/280001/mojo/common/common_cus...
mojo/common/common_custom_types_struct_traits.h:38: token.Serialize(&high,
&low);
On 2016/09/16 23:01:35, dcheng wrote:
> So, for Reasons (tm), each getter here is actually called twice during mojo
> serialization. So this is a bit inefficient.
> 
> (We usually out-of-line serialization methods that aren't just calling simple
> getters as well.)
> 
> I know sandersd@ advocated for removing the getters earlier... but for Mojo
> purposes, we should probably just expose them.
> 
> danakj@, can you think of any clever tricks here so we would limit who the
> getters are exposed to?

Honestly I think we can just expose them, I'm not sure what we're saving
ourselves from. If arbitrary code ran it could read them whether we expose them
or not, and when serializing people won't pull them out to serialize, they will
serialize the type which would take us here.

Otherwise, we can GetHighForSerialization() or something but like super
overkill?

[tguilbert, 2016-09-17 00:10:16]

On 2016/09/16 23:26:51, danakj wrote:
>
https://codereview.chromium.org/2333443002/diff/280001/mojo/common/common_cus...
> File mojo/common/common_custom_types_struct_traits.h (right):
> 
>
https://codereview.chromium.org/2333443002/diff/280001/mojo/common/common_cus...
> mojo/common/common_custom_types_struct_traits.h:38: token.Serialize(&high,
> &low);
> On 2016/09/16 23:01:35, dcheng wrote:
> > So, for Reasons (tm), each getter here is actually called twice during mojo
> > serialization. So this is a bit inefficient.
> > 
> > (We usually out-of-line serialization methods that aren't just calling
simple
> > getters as well.)
> > 
> > I know sandersd@ advocated for removing the getters earlier... but for Mojo
> > purposes, we should probably just expose them.
> > 
> > danakj@, can you think of any clever tricks here so we would limit who the
> > getters are exposed to?
> 
> Honestly I think we can just expose them, I'm not sure what we're saving
> ourselves from. If arbitrary code ran it could read them whether we expose
them
> or not, and when serializing people won't pull them out to serialize, they
will
> serialize the type which would take us here.
> 
> Otherwise, we can GetHighForSerialization() or something but like super
> overkill?

Added GetHighForSerialization/GetLowForSerialization.

[Fady Samuel, 2016-09-17 00:21:38]

On 2016/09/17 00:10:16, ThomasGuilbert wrote:
> On 2016/09/16 23:26:51, danakj wrote:
> >
>
https://codereview.chromium.org/2333443002/diff/280001/mojo/common/common_cus...
> > File mojo/common/common_custom_types_struct_traits.h (right):
> > 
> >
>
https://codereview.chromium.org/2333443002/diff/280001/mojo/common/common_cus...
> > mojo/common/common_custom_types_struct_traits.h:38: token.Serialize(&high,
> > &low);
> > On 2016/09/16 23:01:35, dcheng wrote:
> > > So, for Reasons (tm), each getter here is actually called twice during
mojo
> > > serialization. So this is a bit inefficient.
> > > 
> > > (We usually out-of-line serialization methods that aren't just calling
> simple
> > > getters as well.)
> > > 
> > > I know sandersd@ advocated for removing the getters earlier... but for
Mojo
> > > purposes, we should probably just expose them.
> > > 
> > > danakj@, can you think of any clever tricks here so we would limit who the
> > > getters are exposed to?
> > 
> > Honestly I think we can just expose them, I'm not sure what we're saving
> > ourselves from. If arbitrary code ran it could read them whether we expose
> them
> > or not, and when serializing people won't pull them out to serialize, they
> will
> > serialize the type which would take us here.
> > 
> > Otherwise, we can GetHighForSerialization() or something but like super
> > overkill?
> 
> Added GetHighForSerialization/GetLowForSerialization.

Best method names ever! ;)

[dcheng, 2016-09-17 00:22:20]

LGTM with comments addressed.

https://codereview.chromium.org/2333443002/diff/320001/base/unguessable_token.cc
File base/unguessable_token.cc (right):

https://codereview.chromium.org/2333443002/diff/320001/base/unguessable_token...
base/unguessable_token.cc:30: // Serializing an empty UnguessableToken is a
security issue.
Just inline this. logging.h is already in our header, so we're not saving
anything. And this will make the release path fast, since it is just a getter

(We can keep the name)

https://codereview.chromium.org/2333443002/diff/320001/mojo/common/common_cus...
File mojo/common/common_custom_types_struct_traits.h (right):

https://codereview.chromium.org/2333443002/diff/320001/mojo/common/common_cus...
mojo/common/common_custom_types_struct_traits.h:36: DCHECK(token);
Nit: remove these DCHECKs, we already check them inside
Get{High,Low}ForSerialization anyway

https://codereview.chromium.org/2333443002/diff/320001/mojo/common/common_cus...
mojo/common/common_custom_types_struct_traits.h:55: return true;
Please out-of-line the Read() method into the .cc file.

[tguilbert, 2016-09-17 00:49:49]

Done.

Thank you so much everyone for your comments and for the discussion!

I am sending this last patch to the CQ :)

https://codereview.chromium.org/2333443002/diff/320001/base/unguessable_token.cc
File base/unguessable_token.cc (right):

https://codereview.chromium.org/2333443002/diff/320001/base/unguessable_token...
base/unguessable_token.cc:30: // Serializing an empty UnguessableToken is a
security issue.
On 2016/09/17 00:22:20, dcheng wrote:
> Just inline this. logging.h is already in our header, so we're not saving
> anything. And this will make the release path fast, since it is just a getter
> 
> (We can keep the name)

Done.

https://codereview.chromium.org/2333443002/diff/320001/mojo/common/common_cus...
File mojo/common/common_custom_types_struct_traits.h (right):

https://codereview.chromium.org/2333443002/diff/320001/mojo/common/common_cus...
mojo/common/common_custom_types_struct_traits.h:36: DCHECK(token);
On 2016/09/17 00:22:20, dcheng wrote:
> Nit: remove these DCHECKs, we already check them inside
> Get{High,Low}ForSerialization anyway

Done.

https://codereview.chromium.org/2333443002/diff/320001/mojo/common/common_cus...
mojo/common/common_custom_types_struct_traits.h:55: return true;
On 2016/09/17 00:22:20, dcheng wrote:
> Please out-of-line the Read() method into the .cc file.

Done.

[tguilbert, 2016-09-17 00:50:07]

The CQ bit was checked by tguilbert@chromium.org

[tguilbert, 2016-09-17 00:50:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, avi@chromium.org, tsepez@chromium.org,
sandersd@chromium.org, danakj@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2333443002/#ps340001
(title: "Addressed last comments.")

[commit-bot: I haz the power, 2016-09-17 00:50:44]

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-17 00:59:34]

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

[commit-bot: I haz the power, 2016-09-17 00:59:36]

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

[tguilbert, 2016-09-17 01:12:37]

@rockot Can you OWNERS review mojo/* ?

These two files do not have an OWNERS LGTM
mojo/common/common_custom_types.typemap
mojo/common/common_custom_types_unittest.cc

[Ken Rockot(use gerrit already), 2016-09-17 01:43:12]

Woops, sorry. LGTM

On Sep 16, 2016 6:12 PM, <tguilbert@chromium.org> wrote:

> @rockot Can you OWNERS review mojo/* ?
>
> These two files do not have an OWNERS LGTM
> mojo/common/common_custom_types.typemap
> mojo/common/common_custom_types_unittest.cc
>
> https://codereview.chromium.org/2333443002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-09-17 01:48:45]

lgtm

[tguilbert, 2016-09-17 01:50:22]

The CQ bit was checked by tguilbert@chromium.org

[commit-bot: I haz the power, 2016-09-17 01:50:42]

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-17 03:18:08]

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

[commit-bot: I haz the power, 2016-09-17 03:18:10]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[tguilbert, 2016-09-19 17:51:02]

The CQ bit was checked by tguilbert@chromium.org

[tguilbert, 2016-09-19 17:51:02]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
avi@chromium.org, sandersd@chromium.org, dcheng@chromium.org,
danakj@chromium.org, tsepez@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2333443002/#ps360001
(title: "Fixed typemap linking issue.")

[commit-bot: I haz the power, 2016-09-19 17:51:20]

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

[tguilbert, 2016-09-19 18:20:04]

The CQ bit was unchecked by tguilbert@chromium.org

[tguilbert, 2016-09-19 18:32:56]

The CQ bit was checked by tguilbert@chromium.org

[tguilbert, 2016-09-19 18:32:56]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
avi@chromium.org, sandersd@chromium.org, dcheng@chromium.org,
danakj@chromium.org, tsepez@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2333443002/#ps380001
(title: "Removed MOJO_COMMON_EXPORT")

[commit-bot: I haz the power, 2016-09-19 18:33:22]

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-19 21:11:44]

Description was changed from

==========
Add base::UnguessableToken

cc::SurfaceId, gpu::Mailbox and ScopedSurfaceRequestManager need an 
unguessable identifier. Security recommends using 128 bits to make sure
an ID is unguessable. However, there is no conveniently serializable
way to represent 128 bits.

This change introduces base::UnguessableToken, a 128 bit class with a
cryptographically strong Create() function. UnguessableToken can be
used by themselves, or as part of an aggregate ID.

An empty UnguessableToken is a valid value. It is however illegal to
send empty UnguessableToken across processes (because the resource that
is supposed to be protected by the token would now be guessable).
Sending empty tokens across processes is a security issue, and should
be handled as such.

This change also introduces the appropriate code to send tokens
over IPC and Mojo. base::Optional should be used in cases where it may
be valid to send no token (rather than sending an empty token).

TEST=Added unittests. Also tested in a prototype that uses IPC and Mojo.
BUG=643857
==========

to

==========
Add base::UnguessableToken

cc::SurfaceId, gpu::Mailbox and ScopedSurfaceRequestManager need an 
unguessable identifier. Security recommends using 128 bits to make sure
an ID is unguessable. However, there is no conveniently serializable
way to represent 128 bits.

This change introduces base::UnguessableToken, a 128 bit class with a
cryptographically strong Create() function. UnguessableToken can be
used by themselves, or as part of an aggregate ID.

An empty UnguessableToken is a valid value. It is however illegal to
send empty UnguessableToken across processes (because the resource that
is supposed to be protected by the token would now be guessable).
Sending empty tokens across processes is a security issue, and should
be handled as such.

This change also introduces the appropriate code to send tokens
over IPC and Mojo. base::Optional should be used in cases where it may
be valid to send no token (rather than sending an empty token).

TEST=Added unittests. Also tested in a prototype that uses IPC and Mojo.
BUG=643857
==========

[commit-bot: I haz the power, 2016-09-19 21:11:47]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-09-19 21:13:40]

Description was changed from

==========
Add base::UnguessableToken

cc::SurfaceId, gpu::Mailbox and ScopedSurfaceRequestManager need an 
unguessable identifier. Security recommends using 128 bits to make sure
an ID is unguessable. However, there is no conveniently serializable
way to represent 128 bits.

This change introduces base::UnguessableToken, a 128 bit class with a
cryptographically strong Create() function. UnguessableToken can be
used by themselves, or as part of an aggregate ID.

An empty UnguessableToken is a valid value. It is however illegal to
send empty UnguessableToken across processes (because the resource that
is supposed to be protected by the token would now be guessable).
Sending empty tokens across processes is a security issue, and should
be handled as such.

This change also introduces the appropriate code to send tokens
over IPC and Mojo. base::Optional should be used in cases where it may
be valid to send no token (rather than sending an empty token).

TEST=Added unittests. Also tested in a prototype that uses IPC and Mojo.
BUG=643857
==========

to

==========
Add base::UnguessableToken

cc::SurfaceId, gpu::Mailbox and ScopedSurfaceRequestManager need an
unguessable identifier. Security recommends using 128 bits to make sure
an ID is unguessable. However, there is no conveniently serializable
way to represent 128 bits.

This change introduces base::UnguessableToken, a 128 bit class with a
cryptographically strong Create() function. UnguessableToken can be
used by themselves, or as part of an aggregate ID.

An empty UnguessableToken is a valid value. It is however illegal to
send empty UnguessableToken across processes (because the resource that
is supposed to be protected by the token would now be guessable).
Sending empty tokens across processes is a security issue, and should
be handled as such.

This change also introduces the appropriate code to send tokens
over IPC and Mojo. base::Optional should be used in cases where it may
be valid to send no token (rather than sending an empty token).

TEST=Added unittests. Also tested in a prototype that uses IPC and Mojo.
BUG=643857

Committed: https://crrev.com/4a5ac609f7ea52f2eb69fe871ba6b00c463b9b25
Cr-Commit-Position: refs/heads/master@{#419550}
==========

[commit-bot: I haz the power, 2016-09-19 21:13:43]

Patchset 20 (id:??) landed as
https://crrev.com/4a5ac609f7ea52f2eb69fe871ba6b00c463b9b25
Cr-Commit-Position: refs/heads/master@{#419550}