Compile and link libcrypto_nacl for nacl targets.

Compile and link libcrypto_nacl for nacl targets.

NaCl depends on libipc_nacl, which recently started depending on libcrypto_nacl.
I failed to update the NaCl build files appropriately.

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

[erikchen, 2015-07-16 00:40:11]

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

[commit-bot: I haz the power, 2015-07-16 00:40:30]

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

[erikchen, 2015-07-16 00:41:26]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-07-16 00:41:26]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2015-07-16 00:41:26]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2015-07-16 00:43:19]

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

[erikchen, 2015-07-16 00:43:31]

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

[erikchen, 2015-07-16 00:43:31]

mseaborn: Please review.

[commit-bot: I haz the power, 2015-07-16 00:43:44]

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

[commit-bot: I haz the power, 2015-07-16 00:49:13]

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

[commit-bot: I haz the power, 2015-07-16 00:49:14]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

(exceeded global retry quota)

[Mark Seaborn, 2015-07-16 01:07:10]

This seems to be failing on some trybots.

Which change introduced the dependency on libcrypto?

[erikchen, 2015-07-16 01:10:42]

On 2015/07/16 01:07:10, Mark Seaborn wrote:
> This seems to be failing on some trybots.
> 
> Which change introduced the dependency on libcrypto?

The failures are all analyze, with "null" output. I had assumed it was more
try-bot flakiness (they've been having various problems all day)

This CL adds a dependency from libipc_nacl to libcrypto_nacl
https://codereview.chromium.org/1188923003

[Mark Seaborn, 2015-07-16 01:11:22]

On 2015/07/16 01:07:10, Mark Seaborn wrote:
> This seems to be failing on some trybots.
> 
> Which change introduced the dependency on libcrypto?

Also, if there's a missing dependency, how come the build is passing without
this change?

[erikchen, 2015-07-16 01:13:04]

On 2015/07/16 01:11:22, Mark Seaborn wrote:
> On 2015/07/16 01:07:10, Mark Seaborn wrote:
> > This seems to be failing on some trybots.
> > 
> > Which change introduced the dependency on libcrypto?
> 
> Also, if there's a missing dependency, how come the build is passing without
> this change?

The crypto symbol is used by IPC code, but that IPC code never makes it into
libipc. I'm working on another CL which happens to introduce the symbol into
libipc
https://codereview.chromium.org/1206093002/

[erikchen, 2015-07-16 01:13:24]

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

[commit-bot: I haz the power, 2015-07-16 01:13:56]

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

[commit-bot: I haz the power, 2015-07-16 01:18:08]

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

[commit-bot: I haz the power, 2015-07-16 01:18:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

(exceeded global retry quota)

[erikchen, 2015-07-16 23:33:18]

On 2015/07/16 01:18:09, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
> 
> (exceeded global retry quota)

mseaborn: ping? Would it help if we met to discuss this CL?

[Mark Seaborn, 2015-07-17 00:36:06]

On 15 July 2015 at 18:13, <erikchen@chromium.org> wrote:

> On 2015/07/16 01:11:22, Mark Seaborn wrote:
>
>> On 2015/07/16 01:07:10, Mark Seaborn wrote:
>> > This seems to be failing on some trybots.
>> >
>> > Which change introduced the dependency on libcrypto?
>>
>
>  Also, if there's a missing dependency, how come the build is passing
>> without
>> this change?
>>
>
> The crypto symbol is used by IPC code, but that IPC code never makes it
> into
> libipc. I'm working on another CL which happens to introduce the symbol
> into
> libipc
> https://codereview.chromium.org/1206093002/


So what is the dependency on libcrypto that the change above introduces?
It's not obvious to me -- the string "crypto" doesn't appear in the patch.
Is there a particular crypto function you're calling?

The reason I ask is that I'm wondering if you're introducing any uses of
random IDs into NaCl untrusted code.  And I wonder if the change would
increase the code size of the IRT.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[erikchen, 2015-07-17 18:25:45]

"""
So what is the dependency on libcrypto that the change above introduces?
It's not obvious to me -- the string "crypto" doesn't appear in the patch.
Is there a particular crypto function you're calling?
"""

There is a single call to crypto::RandBytes() in ipc/. This call is made by
brokerable_attachment.cc in the default constructor to BrokerableAttachment().
On trunk, there are no calls to the default constructor of
BrokerableAttachment().

So when I compile trunk, out\Debug\gen\tc_irt\lib64\libipc_nacl.a contains:
"""
U _ZN6crypto9RandBytesEPvj
"""

but out\Debug\nacl_irt_x86_64.nexe.debug doesn't need or require this symbol.

In the referenced CL, the default constructor of BrokerableAttachment() gets
invoked, so now the symbol must be linked into the nacl_irt.

"""
The reason I ask is that I'm wondering if you're introducing any uses of random
IDs into NaCl untrusted code. 
"""
Yes. Any IPC channel that attempts to pass a Windows HANDLE needs to generate a
random ID. The random ID prevents guessing of HANDLES passed from other
processes. Untrusted code can choose to not use a random ID, which would allow
its own handles to be guessable by another process, but the untrusted process
still can't guess the HANDLES of other processes.

"""
And I wonder if the change would increase the code size of the IRT.
"""
Nope, only the debug symbols. crypto::RandBytes points to base::RandBytes, which
is a symbol already present in the nacl_irt.

Trunk:
07/16/2015  06:29 PM         5,768,296 nacl_irt_x86_32.nexe
07/16/2015  06:26 PM             3,634 nacl_irt_x86_32.nexe.cmd
07/16/2015  06:29 PM        76,464,616 nacl_irt_x86_32.nexe.debug
07/16/2015  06:28 PM        76,464,616 nacl_irt_x86_32.nexe.debug.raw
07/16/2015  06:29 PM         6,489,456 nacl_irt_x86_64.nexe
07/16/2015  06:26 PM             3,622 nacl_irt_x86_64.nexe.cmd
07/16/2015  06:28 PM        66,162,432 nacl_irt_x86_64.nexe.debug
07/16/2015  06:28 PM        66,162,432 nacl_irt_x86_64.nexe.debug.raw


Including this CL, and https://codereview.chromium.org/1206093002/
07/17/2015  11:10 AM         5,768,296 nacl_irt_x86_32.nexe
07/17/2015  11:09 AM             3,648 nacl_irt_x86_32.nexe.cmd
07/17/2015  11:10 AM        76,871,488 nacl_irt_x86_32.nexe.debug
07/17/2015  11:10 AM        76,871,488 nacl_irt_x86_32.nexe.debug.raw
07/17/2015  11:11 AM         6,489,456 nacl_irt_x86_64.nexe
07/17/2015  11:09 AM             3,636 nacl_irt_x86_64.nexe.cmd
07/17/2015  11:10 AM        66,506,648 nacl_irt_x86_64.nexe.debug
07/17/2015  11:10 AM        66,506,648 nacl_irt_x86_64.nexe.debug.raw

[Mark Seaborn, 2015-07-17 18:43:09]

On 17 July 2015 at 11:25, <erikchen@chromium.org> wrote:

> """
> So what is the dependency on libcrypto that the change above introduces?
> It's not obvious to me -- the string "crypto" doesn't appear in the patch.
> Is there a particular crypto function you're calling?
> """
>
> There is a single call to crypto::RandBytes() in ipc/. This call is made by
> brokerable_attachment.cc in the default constructor to
> BrokerableAttachment().
>

OK.  Are you expecting this will actually get called at runtime by the NaCl
IRT?  Would it get called when passing a NaCl FD (but not otherwise)?


> """
> The reason I ask is that I'm wondering if you're introducing any uses of
> random
> IDs into NaCl untrusted code.
> """
> Yes. Any IPC channel that attempts to pass a Windows HANDLE needs to
> generate a
> random ID. The random ID prevents guessing of HANDLES passed from other
> processes. Untrusted code can choose to not use a random ID, which would
> allow
> its own handles to be guessable by another process, but the untrusted
> process
> still can't guess the HANDLES of other processes.


I'm not sure what would be happening here, since NaCl untrusted code does
not deal with Windows handles -- it should only deal with NaCl FDs.  Would
you associate random IDs with NaCl FDs too?

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[erikchen, 2015-07-17 21:34:52]

On 2015/07/17 18:43:09, Mark Seaborn wrote:
> On 17 July 2015 at 11:25, <mailto:erikchen@chromium.org> wrote:
> 
> > """
> > So what is the dependency on libcrypto that the change above introduces?
> > It's not obvious to me -- the string "crypto" doesn't appear in the patch.
> > Is there a particular crypto function you're calling?
> > """
> >
> > There is a single call to crypto::RandBytes() in ipc/. This call is made by
> > brokerable_attachment.cc in the default constructor to
> > BrokerableAttachment().
> >
> 
> OK.  Are you expecting this will actually get called at runtime by the NaCl
> IRT?  Would it get called when passing a NaCl FD (but not otherwise)?
I expect this to be called at runtime by the NaCl IRT, if the NaCl IRT ever
attempts to create a Windows HANDLE and send it to another process. 

> 
> 
> > """
> > The reason I ask is that I'm wondering if you're introducing any uses of
> > random
> > IDs into NaCl untrusted code.
> > """
> > Yes. Any IPC channel that attempts to pass a Windows HANDLE needs to
> > generate a
> > random ID. The random ID prevents guessing of HANDLES passed from other
> > processes. Untrusted code can choose to not use a random ID, which would
> > allow
> > its own handles to be guessable by another process, but the untrusted
> > process
> > still can't guess the HANDLES of other processes.
> 
> 
> I'm not sure what would be happening here, since NaCl untrusted code does
> not deal with Windows handles -- it should only deal with NaCl FDs.  Would
> you associate random IDs with NaCl FDs too?
You're right, the untrusted code should never see a Windows handle, and as such
would never need to generate random bytes in this context.

> 
> Cheers,
> Mark
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2015-07-17 23:50:51]

OK, LGTM.  I would be more comfortable if the random-ID-using code never got
compiled into the IRT, to make sure we're not using it there, but I realise that
might make the code more complicated.

[erikchen, 2015-07-17 23:56:28]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-07-17 23:58:04]

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

[commit-bot: I haz the power, 2015-07-18 00:09:18]

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

[commit-bot: I haz the power, 2015-07-18 00:09:19]

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-07-18 00:11:04]

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

[erikchen, 2015-07-18 00:11:04]

agl: Please review.

[agl, 2015-07-20 22:37:07]

lgtm

[erikchen, 2015-07-20 22:37:26]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-07-20 22:37:51]

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

[commit-bot: I haz the power, 2015-07-20 23:27:22]

Committed patchset #3 (id:40001)

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

Patchset 3 (id:??) landed as
https://crrev.com/675c4a0b28eb5b8e922153af2feb1ff68b635eb7
Cr-Commit-Position: refs/heads/master@{#339558}