Non-SFI NaCl: Disallow fancy clock IDs

Non-SFI NaCl: Disallow fancy clock IDs

Restrict the admissible set of clock IDs to just CLOCK_MONOTONIC,
CLOCK_PROCESS_CPUTIME_ID, CLOCK_REALTIME, and CLOCK_THREAD_CPUTIME_ID
(i.e., the same clocks allowed in regular NaCl).  In particular, we do
not allow arbitrary per-process CPU clocks, which can leak information
about the state of other non-SFI processes.

BUG=374479
TBR=cpu@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271859

[mdempsky, 2014-05-17 03:18:00]

Some thoughts I had about this on the way home:
1. POSIX requires clock_gettime(), etc to return EINVAL if given an invalid
clock ID.  Not sure how much NaCl cares about POSIX compliance here though.
2. If we did return EINVAL instead, it would make testing for failure easier,
since it could be tested in a single test case instead of needing multiple death
tests.

[hamaji, 2014-05-19 05:54:10]

Thanks for doing this! I think returning EINVAL should be done in (nexe-side)
libc, so current code is fine.

https://codereview.chromium.org/286363003/diff/60001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right):

https://codereview.chromium.org/286363003/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:399: BPF_ASSERT_EQ(0,
clock_gettime(CLOCK_MONOTONIC, &ts));
Maybe better to check if |ts| is filled.

https://codereview.chromium.org/286363003/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:405: class
ClockGetTimeCrashCPUClockDelegate : public sandbox::BPFTesterDelegate {
Could you add a brief comment to explain why we need a special class?

https://codereview.chromium.org/286363003/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:429:
ClockGetTimeCrashCPUClockDelegate);
I think you can also test CLOCK_MONOTONIC_RAW

[mdempsky, 2014-05-19 06:59:21]

https://codereview.chromium.org/286363003/diff/60001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right):

https://codereview.chromium.org/286363003/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:399: BPF_ASSERT_EQ(0,
clock_gettime(CLOCK_MONOTONIC, &ts));
On 2014/05/19 05:54:11, hamaji wrote:
> Maybe better to check if |ts| is filled.

Done.

https://codereview.chromium.org/286363003/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:405: class
ClockGetTimeCrashCPUClockDelegate : public sandbox::BPFTesterDelegate {
On 2014/05/19 05:54:11, hamaji wrote:
> Could you add a brief comment to explain why we need a special class?

Done.

https://codereview.chromium.org/286363003/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:429:
ClockGetTimeCrashCPUClockDelegate);
On 2014/05/19 05:54:11, hamaji wrote:
> I think you can also test CLOCK_MONOTONIC_RAW

Done.

[hamaji, 2014-05-19 10:01:01]

LGTM, but you'd like to wait for Julien's review.

[jln (very slow on Chromium), 2014-05-20 03:18:48]

Great, thanks! (We should probably do the same in the baseline policy).

Adding Mark for yet-another sanity check.

[jln (very slow on Chromium), 2014-05-20 03:19:07]

I forgot: lgtm

[Mark Seaborn, 2014-05-20 21:21:30]

LGTM

https://codereview.chromium.org/286363003/diff/100001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right):

https://codereview.chromium.org/286363003/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:78: // CLOCK_REALTIME, and
CLOCK_THREAD_CPUTIME_ID.
Maybe also say: "Don't allow reading the clocks of other processes by specifying
their PIDs."

It's not obvious the syscalls allow that.  I don't think there are even any
#defines for that in the kernel headers.  (glibc has
sysdeps/unix/sysv/linux/kernel-posix-cpu-timers.h, which #defines
MAKE_PROCESS_CPUCLOCK and CPUCLOCK_*.)

https://codereview.chromium.org/286363003/diff/100001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right):

https://codereview.chromium.org/286363003/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:420: // We need this
test delegate so we can compute the clock ID for the init
You might also test this by constructing the clock ID manually, as glibc's
MAKE_PROCESS_CPUCLOCK macro does, from the current PID.

When we switch to building this with newlib, we simply won't have a working
clock_getcpuclockid() to test this against.  It would be good to prepare for
that switch.

We could continue running the tests against glibc as well as newlib, but that's
not perfect.

[mdempsky, 2014-05-20 23:02:50]

https://codereview.chromium.org/286363003/diff/100001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right):

https://codereview.chromium.org/286363003/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:78: // CLOCK_REALTIME, and
CLOCK_THREAD_CPUTIME_ID.
On 2014/05/20 21:21:30, Mark Seaborn wrote:
> Maybe also say: "Don't allow reading the clocks of other processes by
specifying
> their PIDs."
> 
> It's not obvious the syscalls allow that.  I don't think there are even any
> #defines for that in the kernel headers.  (glibc has
> sysdeps/unix/sysv/linux/kernel-posix-cpu-timers.h, which #defines
> MAKE_PROCESS_CPUCLOCK and CPUCLOCK_*.)

Done.

https://codereview.chromium.org/286363003/diff/100001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right):

https://codereview.chromium.org/286363003/diff/100001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:420: // We need this
test delegate so we can compute the clock ID for the init
On 2014/05/20 21:21:30, Mark Seaborn wrote:
> You might also test this by constructing the clock ID manually, as glibc's
> MAKE_PROCESS_CPUCLOCK macro does, from the current PID.

Done.  I found that LSS has the same MAKE_PROCESS_CPUCLOCK() macro, so I went
ahead and reused that here.  This also let us use the simpler BPF_DEATH_TEST_C
macro instead.

[mdempsky, 2014-05-20 23:03:37]

cpu: Please review addition to components/nacl/loader/nonsfi/DEPS for OWNERS
approval.  Thanks!

[mdempsky, 2014-05-21 00:24:21]

mseaborn: jln@ suggests that I emphasize that I've added a use of LSS to
nonsfi_sandbox_unittest.cc and make sure you're okay with that, since you're not
otherwise a fan of LSS.

On 2014/05/20 23:03:37, mdempsky wrote:
> cpu: Please review addition to components/nacl/loader/nonsfi/DEPS for OWNERS
> approval.  Thanks!

cpu: To clarify, this is because third_party/lss doesn't have its own OWNERS
file, and you're an owner for third_party.  jln@ (upstream OWNER for lss)
approves this dependency.

[Mark Seaborn, 2014-05-21 01:11:56]

On 20 May 2014 17:24, <mdempsky@chromium.org> wrote:

> mseaborn: jln@ suggests that I emphasize that I've added a use of LSS to
> nonsfi_sandbox_unittest.cc and make sure you're okay with that, since
> you're not
> otherwise a fan of LSS.


It's fine with me.

I was surprised that LSS defines MAKE_PROCESS_CPUCLOCK, but then there's so
much stuff in there. :-)

I've recently added this in NaCl:
https://src.chromium.org/viewvc/native_client/trunk/src/native_client/src/non...
See the comment there about why that's not (currently) reusing LSS.

At some point we'll need to make the BPF sandbox code build against
nacl-newlib.  We might need to duplicate the definition
of MAKE_PROCESS_CPUCLOCK here, or refactor LSS into smaller parts to avoid
header conflicts, but for now using LSS is fine.

Cheers,
Mark

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

[mdempsky, 2014-05-21 01:12:54]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-05-21 01:14:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mdempsky@chromium.org/286363003/120001

[commit-bot: I haz the power, 2014-05-21 04:07:40]

Change committed as 271859