Stub in more IPC attachment brokering functionality.

Stub in more IPC attachment brokering functionality.

This CL fills in most of the logic for sending a Windows HANDLE from a
non-broker process to the broker process. This consists of several small
changes:

- Create the new IPC messages AttachmentBrokerMsg_WinHandleHasBeenBrokered and
AttachmentBrokerMsg_RequestBrokerageOfWinHandle.
- Add a sender_ member variable to AttachmentBrokerWin.
- Define the wire format for HandleAttachmentWin.
- Add logic to ChannelWin to send
AttachmentBrokerMsg_RequestBrokerageOfWinHandle for each HANDLE attachment.

BUG=466437
Committed: https://crrev.com/eece6c3ca617b0d3c37ce137aba1d2079ab892e4
Cr-Commit-Position: refs/heads/master@{#337689}

[erikchen, 2015-06-16 22:48:08]

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

[commit-bot: I haz the power, 2015-06-16 22:48:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188923003/1

[erikchen, 2015-06-16 22:54:21]

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

[commit-bot: I haz the power, 2015-06-16 22:55:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188923003/20001

[commit-bot: I haz the power, 2015-06-16 23:01:13]

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

[commit-bot: I haz the power, 2015-06-16 23:01:13]

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

[erikchen, 2015-06-18 23:06:30]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-06-18 23:10:09]

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

[commit-bot: I haz the power, 2015-06-18 23:10:13]

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

[commit-bot: I haz the power, 2015-06-18 23:10:14]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[erikchen, 2015-06-18 23:18:13]

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

[commit-bot: I haz the power, 2015-06-18 23:19:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188923003/140001

[commit-bot: I haz the power, 2015-06-19 00:04:20]

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

[commit-bot: I haz the power, 2015-06-19 00:04:21]

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

[erikchen, 2015-06-19 00:12:10]

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

[commit-bot: I haz the power, 2015-06-19 00:12:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188923003/160001

[commit-bot: I haz the power, 2015-06-19 00:25:02]

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

[commit-bot: I haz the power, 2015-06-19 00:25:03]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[erikchen, 2015-06-19 17:18:58]

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

[erikchen, 2015-06-19 17:18:59]

tsepez: Please review.

Let me know if you prefer that I split this into smaller CLs.

[Tom Sepez, 2015-06-19 18:04:11]

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
File ipc/attachment_broker_messages.h (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
ipc/attachment_broker_messages.h:37: // Warning: This IPC message assumes that
the sending and receiving processes
nit: the warning is true for all types in IPC -- how can a situation arise where
this wouldn't be the case?

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
ipc/attachment_broker_messages.h:55:
AttachmentBrokerMsg_RequestBrokerageOfWinHandle,
nit: I think you want |brokering|, the gerund form of the verb, with a brokerage
being a house of brokers (Don't throw stuff at me even though I deserve it).  Or
just BrokerWinHandle.

Or maybe DuplicateWinHandle/WinHandleHasBeenDuplicated, to get rid of the
redundant "broker" in the name, and in case brokers can do other things besides
duplicate in the future.

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
File ipc/attachment_broker_win.cc (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
ipc/attachment_broker_win.cc:35: Message* m = new
AttachmentBrokerMsg_RequestBrokerageOfWinHandle(format);
nit: no need for |m|, the pattern is usually just
  sender_->Send(new AttachmentBrokerMsg_...);

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
ipc/attachment_broker_win.cc:48: void
AttachmentBrokerWin::SetSender(IPC::Sender* sender) {
nit: move trivial setter to header.

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
File ipc/attachment_broker_win.h (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
ipc/attachment_broker_win.h:34: void SetSender(IPC::Sender* sender);
nit: |set_sender()| for this trivial method.

https://codereview.chromium.org/1188923003/diff/160001/ipc/brokerable_attachm...
File ipc/brokerable_attachment.cc (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/brokerable_attachm...
ipc/brokerable_attachment.cc:18: id.nonce[i] =
base::RandInt(std::numeric_limits<int>::min(),
Does this need to be unguessable?  If so, you need a routine from crypto/
instead of this.

https://codereview.chromium.org/1188923003/diff/160001/ipc/brokerable_attachm...
ipc/brokerable_attachment.cc:18: id.nonce[i] =
base::RandInt(std::numeric_limits<int>::min(),
nit: signed to unsigned assignment.  Maybe the nonce is uint8_t[16] and use
RandBytes().

https://codereview.chromium.org/1188923003/diff/160001/ipc/handle_attachment_...
File ipc/handle_attachment_win.h (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/handle_attachment_...
ipc/handle_attachment_win.h:25: // The id of the attachment.
nit: this comment doesn't add much value.

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_channel_win.cc
File ipc/ipc_channel_win.cc (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_channel_win.cc...
ipc/ipc_channel_win.cc:98: for (scoped_refptr<BrokerableAttachment> attachment :
Are we sure this doesn't do needless ref/dererf on each call?  Does using
scoped_refptr<BrokerableAttachment>& instead fix this? Can we use a const
reference too?

Hope to simplify to |for (const auto& attachment : ...) {|

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_channel_win.cc...
ipc/ipc_channel_win.cc:100: bool success =
nit: no need for local.

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_channel_win.cc...
ipc/ipc_channel_win.cc:101: broker_->SendAttachmentToProcess(attachment.get(),
peer_pid_);
nit: local var not needed.

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_message.cc
File ipc/ipc_message.cc (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_message.cc#new...
ipc/ipc_message.cc:165: 0 < attachment_set_->num_brokerable_attachments();
nit: prefer blah > 0 to 0 < blah despite what the mojo folks did above.  Let's
fix both.

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_message_attach...
File ipc/ipc_message_attachment_set.cc (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_message_attach...
ipc/ipc_message_attachment_set.cc:57: return std::count_if(attachments_.begin(),
attachments_.end(),
C++11 std library not yet allowed AFAIK (as much as I like these constructs). I
also worry about incrementing/decrementing the refcount
on each call.

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_message_attach...
File ipc/ipc_message_attachment_set.h (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_message_attach...
ipc/ipc_message_attachment_set.h:63:
std::vector<scoped_refptr<BrokerableAttachment>> PeekBrokerableAttachments()
nit: maybe want an interface that doesn't copy all these and increment ref
counts?

https://codereview.chromium.org/1188923003/diff/160001/ipc/mojo/ipc_channel_m...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/mojo/ipc_channel_m...
ipc/mojo/ipc_channel_mojo.cc:558: // Brokerable attachments are handled by the
AttachmentBroker so
Still need a NOTREACHED() here.

[erikchen, 2015-06-22 22:55:53]

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

[commit-bot: I haz the power, 2015-06-22 22:56:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188923003/200001

[commit-bot: I haz the power, 2015-06-22 23:10:11]

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

[commit-bot: I haz the power, 2015-06-22 23:10:11]

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

[erikchen, 2015-06-22 23:38:40]

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

[commit-bot: I haz the power, 2015-06-22 23:40:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188923003/220001

[commit-bot: I haz the power, 2015-06-22 23:54:29]

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

[commit-bot: I haz the power, 2015-06-22 23:54:30]

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

[erikchen, 2015-06-23 01:09:46]

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

[commit-bot: I haz the power, 2015-06-23 01:10:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188923003/260001

[commit-bot: I haz the power, 2015-06-23 01:20:02]

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

[commit-bot: I haz the power, 2015-06-23 01:20:04]

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

[erikchen, 2015-06-23 01:42:34]

On 2015/06/23 01:20:04, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

tsepez: Temporarily removing you from the reviewer list so you stop getting
spammed with try job failures.

[erikchen, 2015-06-23 01:42:43]

erikchen@chromium.org changed reviewers:
- tsepez@chromium.org

[erikchen, 2015-06-23 01:48:35]

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

[commit-bot: I haz the power, 2015-06-23 01:49:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188923003/280001

[commit-bot: I haz the power, 2015-06-23 01:57:20]

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

[commit-bot: I haz the power, 2015-06-23 01:57:21]

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

[erikchen, 2015-06-23 02:33:23]

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

[commit-bot: I haz the power, 2015-06-23 02:34:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188923003/300001

[commit-bot: I haz the power, 2015-06-23 02:44:15]

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

[commit-bot: I haz the power, 2015-06-23 02:44:16]

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

[erikchen, 2015-06-23 17:30:23]

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

[commit-bot: I haz the power, 2015-06-23 17:31:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188923003/320001

[commit-bot: I haz the power, 2015-06-23 17:43:17]

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

[commit-bot: I haz the power, 2015-06-23 17:43:18]

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

[erikchen, 2015-06-23 21:14:53]

Patchset #18 (id:340001) has been deleted

[erikchen, 2015-06-23 21:15:40]

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

[commit-bot: I haz the power, 2015-06-23 21:17:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188923003/360001

[commit-bot: I haz the power, 2015-06-23 21:29:02]

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

[commit-bot: I haz the power, 2015-06-23 21:29:04]

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

[erikchen, 2015-06-23 22:27:35]

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

[commit-bot: I haz the power, 2015-06-23 22:29:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188923003/380001

[erikchen, 2015-06-23 22:36:57]

erikchen@chromium.org changed reviewers:
+ agl@chromium.org, tsepez@chromium.org

[erikchen, 2015-06-23 22:37:00]

tsepez: PTAL
agl: Looking for an OWNER review of crypto/BUILD.gn, as well as the new
dependency from ipc/ onto crypto/. Note that the only non-whitespace change to
crypto/BUILD.gn is lines 8-19.

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
File ipc/attachment_broker_messages.h (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
ipc/attachment_broker_messages.h:37: // Warning: This IPC message assumes that
the sending and receiving processes
On 2015/06/19 18:04:10, Tom Sepez (OOO) wrote:
> nit: the warning is true for all types in IPC -- how can a situation arise
where
> this wouldn't be the case?

Chrome 32-bit Windows sometimes communicates with a 64-bit sandbox. I've updated
the WireFormat to use int32_t instead of HANDLE.

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
ipc/attachment_broker_messages.h:55:
AttachmentBrokerMsg_RequestBrokerageOfWinHandle,
On 2015/06/19 18:04:10, Tom Sepez (OOO) wrote:
> nit: I think you want |brokering|, the gerund form of the verb, with a
brokerage
> being a house of brokers (Don't throw stuff at me even though I deserve it). 
Or
> just BrokerWinHandle.
> 
> Or maybe DuplicateWinHandle/WinHandleHasBeenDuplicated, to get rid of the
> redundant "broker" in the name, and in case brokers can do other things
besides
> duplicate in the future.

I went with your suggestion of DuplicateWinHandle and
WinHandleHasBeenDuplicated.

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
File ipc/attachment_broker_win.cc (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
ipc/attachment_broker_win.cc:35: Message* m = new
AttachmentBrokerMsg_RequestBrokerageOfWinHandle(format);
On 2015/06/19 18:04:10, Tom Sepez (OOO) wrote:
> nit: no need for |m|, the pattern is usually just
>   sender_->Send(new AttachmentBrokerMsg_...);

Done.

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
ipc/attachment_broker_win.cc:48: void
AttachmentBrokerWin::SetSender(IPC::Sender* sender) {
On 2015/06/19 18:04:10, Tom Sepez (OOO) wrote:
> nit: move trivial setter to header.

Done.

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
File ipc/attachment_broker_win.h (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/attachment_broker_...
ipc/attachment_broker_win.h:34: void SetSender(IPC::Sender* sender);
On 2015/06/19 18:04:10, Tom Sepez (OOO) wrote:
> nit: |set_sender()| for this trivial method.

Done.

https://codereview.chromium.org/1188923003/diff/160001/ipc/brokerable_attachm...
File ipc/brokerable_attachment.cc (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/brokerable_attachm...
ipc/brokerable_attachment.cc:18: id.nonce[i] =
base::RandInt(std::numeric_limits<int>::min(),
On 2015/06/19 18:04:10, Tom Sepez (OOO) wrote:
> Does this need to be unguessable?  If so, you need a routine from crypto/
> instead of this.

Yes it does. I've switched to crypto::RandBytes.

https://codereview.chromium.org/1188923003/diff/160001/ipc/brokerable_attachm...
ipc/brokerable_attachment.cc:18: id.nonce[i] =
base::RandInt(std::numeric_limits<int>::min(),
On 2015/06/19 18:04:11, Tom Sepez (OOO) wrote:
> nit: signed to unsigned assignment.  Maybe the nonce is uint8_t[16] and use
> RandBytes().

Good suggestion, done.

https://codereview.chromium.org/1188923003/diff/160001/ipc/handle_attachment_...
File ipc/handle_attachment_win.h (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/handle_attachment_...
ipc/handle_attachment_win.h:25: // The id of the attachment.
On 2015/06/19 18:04:11, Tom Sepez (OOO) wrote:
> nit: this comment doesn't add much value.

Removed it.

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_channel_win.cc
File ipc/ipc_channel_win.cc (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_channel_win.cc...
ipc/ipc_channel_win.cc:98: for (scoped_refptr<BrokerableAttachment> attachment :
On 2015/06/19 18:04:11, Tom Sepez (OOO) wrote:
> Are we sure this doesn't do needless ref/dererf on each call?  Does using
> scoped_refptr<BrokerableAttachment>& instead fix this? Can we use a const
> reference too?
> 
> Hope to simplify to |for (const auto& attachment : ...) {|

As per your later suggestion, I changed the interface to MessageAttachmentSet to
return a vector of raw pointers instead of scoped_refptrs to avoid the otherwise
unnecessary ref/deref.
I chan

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_channel_win.cc...
ipc/ipc_channel_win.cc:100: bool success =
On 2015/06/19 18:04:11, Tom Sepez (OOO) wrote:
> nit: no need for local.

I've removed it.

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_channel_win.cc...
ipc/ipc_channel_win.cc:101: broker_->SendAttachmentToProcess(attachment.get(),
peer_pid_);
On 2015/06/19 18:04:11, Tom Sepez (OOO) wrote:
> nit: local var not needed.

Done.

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_message.cc
File ipc/ipc_message.cc (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_message.cc#new...
ipc/ipc_message.cc:165: 0 < attachment_set_->num_brokerable_attachments();
On 2015/06/19 18:04:11, Tom Sepez (OOO) wrote:
> nit: prefer blah > 0 to 0 < blah despite what the mojo folks did above.  Let's
> fix both.

Done.

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_message_attach...
File ipc/ipc_message_attachment_set.cc (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_message_attach...
ipc/ipc_message_attachment_set.cc:57: return std::count_if(attachments_.begin(),
attachments_.end(),
On 2015/06/19 18:04:11, Tom Sepez (OOO) wrote:
> C++11 std library not yet allowed AFAIK (as much as I like these constructs).
I
> also worry about incrementing/decrementing the refcount
> on each call.

The latter problem could be fixed, but the former cannot. I was copying the code
from the two previous methods. I've created a new function in this translation
unit that does the counting, and I've changed all three methods to use this new
function.

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_message_attach...
File ipc/ipc_message_attachment_set.h (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/ipc_message_attach...
ipc/ipc_message_attachment_set.h:63:
std::vector<scoped_refptr<BrokerableAttachment>> PeekBrokerableAttachments()
On 2015/06/19 18:04:11, Tom Sepez (OOO) wrote:
> nit: maybe want an interface that doesn't copy all these and increment ref
> counts?

I changed the interface to return a vector of raw pointers.

(I want the returned type to be BrokerableAttachment rather than
MessageAttachment, so there's no way to return a reference to the existing
objects.)

https://codereview.chromium.org/1188923003/diff/160001/ipc/mojo/ipc_channel_m...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/1188923003/diff/160001/ipc/mojo/ipc_channel_m...
ipc/mojo/ipc_channel_mojo.cc:558: // Brokerable attachments are handled by the
AttachmentBroker so
On 2015/06/19 18:04:11, Tom Sepez (OOO) wrote:
> Still need a NOTREACHED() here.

Done.

https://codereview.chromium.org/1188923003/diff/380001/crypto/BUILD.gn
File crypto/BUILD.gn (right):

https://codereview.chromium.org/1188923003/diff/380001/crypto/BUILD.gn#newcode10
crypto/BUILD.gn:10: output_name = "crcrypto"  # Avoid colliding with OpenSSL's
libcrypto.
I was wondering whether this needed to have a different output_name. This change
passes the trybots, so I didn't change it.

[commit-bot: I haz the power, 2015-06-23 22:42:04]

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

[commit-bot: I haz the power, 2015-06-23 22:42:06]

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

[agl, 2015-06-24 00:59:56]

https://codereview.chromium.org/1188923003/diff/380001/ipc/brokerable_attachm...
File ipc/brokerable_attachment.cc (right):

https://codereview.chromium.org/1188923003/diff/380001/ipc/brokerable_attachm...
ipc/brokerable_attachment.cc:15: crypto::RandBytes(id.nonce,
BrokerableAttachment::kNonceSize);
What are the security requirements around this? You've increased the nonce size
from 4 to 16 bytes. Is a global counter unsuitable?

The use of crypto::RandBytes suggests that there's some security property here
but that's unclear. If not, then base/rand_util.h should suffice.

[erikchen, 2015-06-24 01:06:02]

https://codereview.chromium.org/1188923003/diff/380001/ipc/brokerable_attachm...
File ipc/brokerable_attachment.cc (right):

https://codereview.chromium.org/1188923003/diff/380001/ipc/brokerable_attachm...
ipc/brokerable_attachment.cc:15: crypto::RandBytes(id.nonce,
BrokerableAttachment::kNonceSize);
On 2015/06/24 00:59:56, agl wrote:
> What are the security requirements around this? You've increased the nonce
size
> from 4 to 16 bytes. Is a global counter unsuitable?
> 
> The use of crypto::RandBytes suggests that there's some security property here
> but that's unclear. If not, then base/rand_util.h should suffice.

The nonce used to be 4 uint32_ts, now it's 16 uint8_ts, so there no change in
total size. The security requirement is that the nonce should be unguessable,
and there should be no nonce collisions. Based on the number of expected IPC
messages and the birthday paradox, 16 bytes seemed like the right size.

(This information is also available on my design doc:
https://docs.google.com/document/d/10OiW_YGxQsiTLaP1x3Yp0ymIEEJEBKqFPMVlFDdFZ...
)

[agl, 2015-06-24 01:28:57]

https://codereview.chromium.org/1188923003/diff/380001/crypto/BUILD.gn
File crypto/BUILD.gn (right):

https://codereview.chromium.org/1188923003/diff/380001/crypto/BUILD.gn#newcode12
crypto/BUILD.gn:12: "random.cc",
This source list seems to be substantially different from the one in
crypto_nacl.gyp. The latter includes everything an excludes the
platform-specific files. Is that not possible here?

https://codereview.chromium.org/1188923003/diff/380001/ipc/brokerable_attachm...
File ipc/brokerable_attachment.cc (right):

https://codereview.chromium.org/1188923003/diff/380001/ipc/brokerable_attachm...
ipc/brokerable_attachment.cc:15: crypto::RandBytes(id.nonce,
BrokerableAttachment::kNonceSize);
On 2015/06/24 01:06:02, erikchen wrote:
> The nonce used to be 4 uint32_ts, now it's 16 uint8_ts, so there no change in
> total size. The security requirement is that the nonce should be unguessable,
> and there should be no nonce collisions. Based on the number of expected IPC
> messages and the birthday paradox, 16 bytes seemed like the right size.

Ok. Please comment:

// In order to prevent mutually untrusted processes from stealing resources from
one another, the nonce must be secret. This generates a 128-bit,
cryptographicaly-strong random number.

[erikchen, 2015-06-24 17:50:25]

https://codereview.chromium.org/1188923003/diff/380001/crypto/BUILD.gn
File crypto/BUILD.gn (right):

https://codereview.chromium.org/1188923003/diff/380001/crypto/BUILD.gn#newcode12
crypto/BUILD.gn:12: "random.cc",
On 2015/06/24 01:28:57, agl wrote:
> This source list seems to be substantially different from the one in
> crypto_nacl.gyp. The latter includes everything an excludes the
> platform-specific files. Is that not possible here?

Not without a lot more changes. crypto_nacl.gyp depends on:
native_client_sdk/native_client_sdk_untrusted.gy, which, among other tasks,
stubs out common system includes such as <arpa/inet.h>. There is no GN
counterpart for this file.

https://codereview.chromium.org/1188923003/diff/380001/ipc/brokerable_attachm...
File ipc/brokerable_attachment.cc (right):

https://codereview.chromium.org/1188923003/diff/380001/ipc/brokerable_attachm...
ipc/brokerable_attachment.cc:15: crypto::RandBytes(id.nonce,
BrokerableAttachment::kNonceSize);
On 2015/06/24 01:28:57, agl wrote:
> On 2015/06/24 01:06:02, erikchen wrote:
> > The nonce used to be 4 uint32_ts, now it's 16 uint8_ts, so there no change
in
> > total size. The security requirement is that the nonce should be
unguessable,
> > and there should be no nonce collisions. Based on the number of expected IPC
> > messages and the birthday paradox, 16 bytes seemed like the right size.
> 
> Ok. Please comment:
> 
> // In order to prevent mutually untrusted processes from stealing resources
from
> one another, the nonce must be secret. This generates a 128-bit,
> cryptographicaly-strong random number.

Done.

[erikchen, 2015-06-24 22:36:50]

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

[commit-bot: I haz the power, 2015-06-24 22:38:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1188923003/400001

[commit-bot: I haz the power, 2015-06-24 22:51:54]

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

[commit-bot: I haz the power, 2015-06-24 22:51:56]

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

[Tom Sepez, 2015-06-29 18:45:26]

lgtm

[erikchen, 2015-07-06 16:35:48]

agl: ping?

[agl2, 2015-07-07 20:27:36]

agl@google.com changed reviewers:
+ agl@google.com

[agl2, 2015-07-07 20:27:43]

lgtm

[agl, 2015-07-07 20:29:05]

lgtm

[erikchen, 2015-07-07 20:30:22]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-07-07 20:30:50]

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

[commit-bot: I haz the power, 2015-07-07 20:34:11]

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

[commit-bot: I haz the power, 2015-07-07 20:34:14]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

(exceeded global retry quota)

[erikchen, 2015-07-07 20:48:53]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-07-07 20:48:53]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
agl@chromium.org, agl@google.com
Link to the patchset: https://codereview.chromium.org/1188923003/#ps460001
(title: "Rebase properly.")

[commit-bot: I haz the power, 2015-07-07 20:49:49]

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

[commit-bot: I haz the power, 2015-07-07 22:13:32]

Committed patchset #23 (id:460001)

[commit-bot: I haz the power, 2015-07-07 22:14:34]

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