Reland: Add seccomp sandbox for non-SFI NaCl

Reland: Add seccomp sandbox for non-SFI NaCl

This is the reland of https://codereview.chromium.org/196793023/
In the old patch, ldflags! for ASan was specified in nacl_loader target,
which is a static_library. Now we set this in nacl_helper target. The
diff from the previous change is:
https://codereview.chromium.org/240783003/

All syscalls except whitelisted ones will cause SIGSYS.

We test the sandbox with BPF_TEST and BPF_TEST_DEATH, which
appropriately fork the process so the main process of the test
will never enable the sandbox.

TEST=Our app works with this sandbox on i686 and ARM
TEST=Build chrome and nacl_helper on i686, x86-64, and ARM
TEST=./out/Release/components_unittests --gtest_filter='NaClNonSfi*'
TEST=SFI NaCl apps still work
TEST=trybots
BUG=359285
R=jln@chromium.org, mseaborn@chromium.org
TBR=jochen

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

[hamaji, 2014-04-17 07:01:50]

Thanks for the review in the previous change. The previous change was reverted
because it broke ASan build. Now I think I resolved the issue.

[jochen (gone - plz use gerrit), 2014-04-17 08:23:04]

On 2014/04/17 07:01:50, hamaji wrote:
> Thanks for the review in the previous change. The previous change was reverted
> because it broke ASan build. Now I think I resolved the issue.

what did you change?

[hamaji, 2014-04-17 17:20:45]

On 2014/04/17 08:23:04, jochen (OOO until Apr 22) wrote:
> On 2014/04/17 07:01:50, hamaji wrote:
> > Thanks for the review in the previous change. The previous change was
reverted
> > because it broke ASan build. Now I think I resolved the issue.
> 
> what did you change?

This is the diff from the previous patch:
https://codereview.chromium.org/240783003/

But hmm now ASan build compiles, but it seems there is an incompatibility
between ASan and the seecomp-bpf filter I'm adding. ASan internally use ioctl
(16) and it confuses the result?

[jln (very slow on Chromium), 2014-04-17 17:28:05]

Note: in general when you reland a CL, it's better to first upload the original
one and then the new version, so that the diff is obvious.

https://codereview.chromium.org/239703011/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right):

https://codereview.chromium.org/239703011/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:253: 
Maybe add something such as:

inline bool RunningOnASAN() {
#if defined(ADDRESS_SANITIZER)
  return true;
#else
  return false;
#endif
}

in the anonymous namespace.

Then:

if (RunningOnAsan) {
 if (__NR_ioctl == sysno)
   return RestrictIoctl(sb);
}

[hamaji, 2014-04-17 18:25:36]

https://codereview.chromium.org/239703011/diff/1/components/nacl/loader/nonsf...
File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right):

https://codereview.chromium.org/239703011/diff/1/components/nacl/loader/nonsf...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:253: 
On 2014/04/17 17:28:06, jln wrote:
> Maybe add something such as:
> 
> inline bool RunningOnASAN() {
> #if defined(ADDRESS_SANITIZER)
>   return true;
> #else
>   return false;
> #endif
> }
> 
> in the anonymous namespace.
> 
> Then:
> 
> if (RunningOnAsan) {
>  if (__NR_ioctl == sysno)
>    return RestrictIoctl(sb);
> }

Thanks for the suggestion! It seems we need getpid, ioctl, and readlink, but all
of them can return EPERM. So, I've added them to IsGracefullyDenied().

https://codereview.chromium.org/239703011/diff/20001/components/nacl.gyp
File components/nacl.gyp (right):

https://codereview.chromium.org/239703011/diff/20001/components/nacl.gyp#newc...
components/nacl.gyp:146: 'nacl/renderer/manifest_service_channel.cc',
Merged hidehiko@'s change.

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

https://codereview.chromium.org/239703011/diff/20001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:174: getsockname(0,
NULL, &addrlen);
It seems ASan checks the memory the third argument points to. Passing a more
realistic value would harm nothing.

[hamaji, 2014-04-17 18:26:13]

On 2014/04/17 17:28:05, jln wrote:
> Note: in general when you reland a CL, it's better to first upload the
original
> one and then the new version, so that the diff is obvious.

Thanks. I'll do this from the next time.

[jln (very slow on Chromium), 2014-04-17 18:34:16]

This lgtm.

However I wonder if we could just gracefully deny these system calls in all
cases.
(In which case we would need to check that they EPERM in the tests).

Not gracefully denying them gives us a good way to catch a caller that expects
them to work but doesn't check EPERM (which could be the case for getpid()
notably), but I wonder if it's worth the cost in complexity.

Up to you.

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

https://codereview.chromium.org/239703011/diff/20001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:79:
BPF_TEST(NaClNonSfiSandboxTest, clone_by_pthread_create,
sandbox/linux/tests/unit_tests.h provides DISABLE_ON_TSAN().

You could create DISABLE_ON_ASAN and use that instead.

https://codereview.chromium.org/239703011/diff/20001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:174: getsockname(0,
NULL, &addrlen);
It probably doesn't hurt to make the second argument non NULL as well, in case
ASAN adds new improvements.

[hamaji, 2014-04-17 19:13:42]

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

https://codereview.chromium.org/239703011/diff/20001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:79:
BPF_TEST(NaClNonSfiSandboxTest, clone_by_pthread_create,
On 2014/04/17 18:34:17, jln wrote:
> sandbox/linux/tests/unit_tests.h provides DISABLE_ON_TSAN().
> 
> You could create DISABLE_ON_ASAN and use that instead.

Done.

https://codereview.chromium.org/239703011/diff/20001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:174: getsockname(0,
NULL, &addrlen);
On 2014/04/17 18:34:17, jln wrote:
> It probably doesn't hurt to make the second argument non NULL as well, in case
> ASAN adds new improvements.

Done.

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

https://codereview.chromium.org/239703011/diff/40001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:399:
BPF_TEST(NaClNonSfiSandboxTest, getegid_EPERM,
I feel we might want to move these EPERM only tests to another file, but I'd
like to do this in a separate change.

[hamaji, 2014-04-17 19:19:05]

+blundell for components_tests.gyp.

This gyp file is not different from the previous version.

[jln (very slow on Chromium), 2014-04-17 19:20:43]

On 2014/04/17 19:19:05, hamaji wrote:
> +blundell for components_tests.gyp.
> 
> This gyp file is not different from the previous version.

It's fine to TBR=jochen or the GYP file. Let's just see if Mark is happy with
the change and proceed.

[Mark Seaborn, 2014-04-17 19:36:57]

LGTM

https://codereview.chromium.org/239703011/diff/40001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right):

https://codereview.chromium.org/239703011/diff/40001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:38: inline bool
IsRunningOnASAN() {
Not used any more?

BTW, in general you don't need "inline" for static functions defined in .c/.cc
files.  In this case, I think it turns off the "defined but not used" warning.

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

https://codereview.chromium.org/239703011/diff/40001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:442:
BPF_TEST(NaClNonSfiSandboxTest, raedlink_EPERM,
Typo: "readlink"

[Mark Seaborn, 2014-04-17 19:41:06]

BTW, it rather tiresome to have changes reverted because of ASan.  Making NaCl
(whether SFI or Non-SFI) work with ASan is a losing battle.  It's not going to
work when we switch Chrome's Non-SFI code to build with newlib anyway, unless
someone adds ASan support to the PNaCl toolchain (which is non-trivial).

So if we have problems in future, I'd be happy if we just disabled the Non-SFI
NaCl tests under ASan.

But if we are going to support ASan, everyone should know to manually run ASan
trybots before committing, like how we have to manually run the
linux_rel_precise32 trybot. :-)

[hamaji, 2014-04-17 19:42:06]

https://codereview.chromium.org/239703011/diff/40001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right):

https://codereview.chromium.org/239703011/diff/40001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:38: inline bool
IsRunningOnASAN() {
On 2014/04/17 19:36:57, Mark Seaborn wrote:
> Not used any more?
> 
> BTW, in general you don't need "inline" for static functions defined in .c/.cc
> files.  In this case, I think it turns off the "defined but not used" warning.

Done.

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

https://codereview.chromium.org/239703011/diff/40001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:442:
BPF_TEST(NaClNonSfiSandboxTest, raedlink_EPERM,
On 2014/04/17 19:36:57, Mark Seaborn wrote:
> Typo: "readlink"

Done.

[hamaji, 2014-04-17 19:42:16]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-04-17 19:43:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/239703011/60001

[Mark Seaborn, 2014-04-17 21:41:16]

BTW, I would recommend committing Non-SFI NaCl changes manually rather than
using the CQ at this point.

The CQ is slow but at the same time doesn't actually test what matters -- most
of the Non-SFI NaCl tests are only run on linux_rel_precise32, which the CQ
doesn't cover.  This could easily be wasting a lot of resources to re-run flaky
tests on Windows and Mac, only to find that 32-bit Linux fails after the change
is committed.

[hamaji, 2014-04-17 21:51:18]

Committed patchset #4 manually as r264651 (presubmit successful).