add restricted filesystem access to sel_ldr

add restricted filesystem access to sel_ldr

As discussed on
https://groups.google.com/forum/#!topic/native-client-discuss/5UFlj0e5fvw

Overall strategy: simply add one new command line option (-m), for
specifying a chroot-style folder to use as the root for the
untrusted process. If provided, the untrusted process will be able
to read and write data (trusted OS filesystem permissions permitting)
to the provided folder as if it were the root.

Looking for critical feedback

[jtolds, 2015-06-25 22:58:02]

jtolds@gmail.com changed reviewers:
+ mseaborn@chromium.org

[jtolds, 2015-06-25 22:58:33]

jtolds@gmail.com changed reviewers:
+ jfb@chromium.org, phosek@chromium.org

[jtolds, 2015-06-25 23:05:06]

just some questions and comments

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
File src/trusted/service_runtime/nacl_syscall_common.c (right):

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
src/trusted/service_runtime/nacl_syscall_common.c:101: * end with a trailing
slash.
hmm, i guess we'll need to use a path separator. not sure how this prefix
approach works with windows and drive letters. do binaries inside native client
use backslashes and drive letters?

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
File src/trusted/service_runtime/sel_main.c (right):

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
src/trusted/service_runtime/sel_main.c:619:
NaClMountRootFolder(options->root_mount);
super open to other names for all these things. it's not really a mount at all.
i would have called it a "prefix", but -p was taken (and seemed kind of
insecure, so I wasn't super thrilled to use -P, to try and avoid people mixing
them up), and "-m" was available.

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
File src/trusted/service_runtime/sys_fdio.c (right):

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
src/trusted/service_runtime/sys_fdio.c:138: if (!NaClAclBypassChecks &&
NaClRootFolder == NULL) {
definitely worried the most about Getdents here. Do I need to look for ".." dir
entries or anything and remap them if it's the untrusted root?

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
File src/trusted/service_runtime/sys_filename.c (right):

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
src/trusted/service_runtime/sys_filename.c:77: *  path elements.
does this even need to happen?

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
src/trusted/service_runtime/sys_filename.c:85: * TODO(jtolds): remove
NaClRootFolder prefix and fail if it doesn't match
this function is only used to get the CWD. what should happen if the CWD is not
inside the untrusted root? should we chdir before starting to reduce that
chance?

[Mark Seaborn, 2015-06-25 23:55:09]

High-level feedback:

* Can you add a text file under documentation/ to explain the feature?  This can
cover details such as how symlinks and the cwd are handled.  For example, the
feature might only be safe if there are no existing symlinks inside the sandbox
directory.

* Can you add test coverage?  The way to do this is to add a Scons-based test in
the "tests" dir.  You could add a new subdirectory under "tests".

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
File src/trusted/service_runtime/nacl_syscall_common.c (right):

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
src/trusted/service_runtime/nacl_syscall_common.c:101: * end with a trailing
slash.
On 2015/06/25 23:05:05, jtolds wrote:
> hmm, i guess we'll need to use a path separator. not sure how this prefix
> approach works with windows and drive letters. do binaries inside native
client
> use backslashes and drive letters?

I also don't know what the exact rules would need to be for Windows.

It would be reasonable to make this functionality Unix-only for this CL.  So
"-m" could report an error on Windows.

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
File src/trusted/service_runtime/nacl_syscall_common.h (right):

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
src/trusted/service_runtime/nacl_syscall_common.h:48: extern char
*NaClRootFolder;
Nit: "Directory" or "Dir" would be preferred over "Folder" here, since the
former term is more commonly used in systems programming.

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
File src/trusted/service_runtime/sys_fdio.c (right):

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
src/trusted/service_runtime/sys_fdio.c:138: if (!NaClAclBypassChecks &&
NaClRootFolder == NULL) {
On 2015/06/25 23:05:05, jtolds wrote:
> definitely worried the most about Getdents here. Do I need to look for ".."
dir
> entries or anything and remap them if it's the untrusted root?

The ".." entries reported by getdents() don't convey any authority, they just
reveal inode numbers.  So for security it's not essential to hide or remap them.

Programs might get confused if inode numbers are reported inconsistently, though
maybe that's not very likely.  Hiding/remapping inode numbers would just be
useful optional functionality to avoid that sort of problem.

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
File src/trusted/service_runtime/sys_filename.c (right):

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
src/trusted/service_runtime/sys_filename.c:77: *  path elements.
On 2015/06/25 23:05:05, jtolds wrote:
> does this even need to happen?

If you don't remove ".." elements, isn't it trivial to read outside of
NaClRootFolder?

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
src/trusted/service_runtime/sys_filename.c:388: * this means having symlinks
that point outside of your mounted root
Furthermore, note that if you have a relative symlink whose destination contains
a "..", it might start out pointing to within the sandbox filesystem, but
rename() or link() could be used to move it so that it points outside.

This is the kind of detail that can be covered in a file in documentation/.

[Mark Seaborn, 2015-06-25 23:59:33]

By the way, one piece of admin is that you would need to need to sign the CLA
(Contributor License Agreement) in order to contribute a patch to NaCl:
https://cla.developers.google.com/about/google-individual

[jtolds, 2015-06-26 22:07:11]

signed the CLA (thought I already had, but I guess the Go and Android ones are
different)

working through the rest of these now

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
File src/trusted/service_runtime/sys_filename.c (right):

https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
src/trusted/service_runtime/sys_filename.c:77: *  path elements.
On 2015/06/25 23:55:09, Mark Seaborn wrote:
> On 2015/06/25 23:05:05, jtolds wrote:
> > does this even need to happen?
> 
> If you don't remove ".." elements, isn't it trivial to read outside of
> NaClRootFolder?

currently working through these comments, but i just didn't know if path
sanitization happened at some higher level or something.

[jtolds, 2015-06-27 16:27:37]

Uh oh, new wrinkle I stupidly didn't realize at first:

Of course of course, paths requested are sometimes relative, which means simply
prepending the prefix is not viable.

So I see two options and I'm interested in your thoughts:

1) I've been hoping to sanitize paths completely lexically without hitting disk
(look for .., ., etc, just like how https://golang.org/pkg/path/filepath/#Clean
does it). I can keep doing that, and in the case of a relative path also prepend
the CWD first. I'm kind of leaning toward this one from a security perspective,
but it's more gross.

2) I could give up on lexical path sanitization and just use realpath calls
everywhere to check what realpath is getting accessed. This *would* have the
unintended side effect of making symlink support a little easier.

Haven't pushed yet, but here's my documentation/ bit about symlinks and cwd for
reference:

  ### Symlinks

  Ideally, symlinks should behave as if we had just chrooted into the path given
  to `-m` (which means they would resolve relative to the untrusted root).

  However, it's not straightforward to have `sel_ldr` call `chroot` itself to
  make the kernel do appropriate symlink resolution in a cross-platform way. So
  our other option to support symlinks is to do an Lstat on every path load and
  try to resolve symlinks ourselves. This would be a pretty big performance hit
  and a significant amount of complication needing auditing. We wouldn't be able
  to just pass through sanitized paths to the host's open call without making
  sure the untrusted app hadn't created a symlink pointing elsewhere to jump out
  of jail.

  So instead, for v1 we're just not supporting symlinks. With `-m`, the syscalls
  for creating and reading symlinks will fail. However, the path passed to `-m`
  might have symlinks in it, so it is up to the creator of the filesystem passed
  to `-m` to sanitize it for bad (or even relative!) symlinks prior to safely
  running `sel_ldr`. This is a significant amount of sharpness and potential
  insecurity this feature might bring, so it might be worth just failing to
start
  if symlinks exist at any subpath from the path given to `-m`. Currently we're
  going to allow the sharpness of this interface.

  The following syscalls relate to symlinks.

   * `NaClSysLstat` - calls the host's `stat` call (not the host's `lstat`)
   * `NaClSysSymlink` - always fails
   * `NaClSysReadlink` - always fails

  ### Current working directory

  Being able to read the current working directory (aside from `readlink`,
  previously discussed) is the only syscall that has a path going from the host
  back to the untrusted application. All other syscalls are one way - paths go
  from the untrusted application out to the host.

  The current working directory, unless we call the host's `chdir` at startup to
  the path given to `-m`, might not be inside of the untrusted application's
  filesystem.

  So for the following two syscalls they're implemented the following way:

   * `NaClSysChdir` - always adds the path prefix
   * `NaClSysGetcwd` - checks if the path is sanitized and starts with the path
      prefix. if it doesn't start with the path prefix this syscall fails.

[jtolds, 2015-06-27 16:30:01]

> 2) I could give up on lexical path sanitization and just use realpath calls
> everywhere to check what realpath is getting accessed. This *would* have the
> unintended side effect of making symlink support a little easier.

I think I'd still disallow symlink creation if we went this way since 
otherwise the untrusted application could at least make sel_ldr make realpath 
calls on files outside the untrusted application's root.

[Mark Seaborn, 2015-06-28 20:51:15]

On 26 June 2015 at 15:07, <jtolds@gmail.com> wrote:

> signed the CLA (thought I already had, but I guess the Go and Android ones
> are
> different)
>

Thanks.  I don't know whether the difference projects require different
CLAs.


>
>
https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
> File src/trusted/service_runtime/sys_filename.c (right):
>
>
>
https://codereview.chromium.org/1211173002/diff/1/src/trusted/service_runtime...
> src/trusted/service_runtime/sys_filename.c:77: *  path elements.
> On 2015/06/25 23:55:09, Mark Seaborn wrote:
>
>> On 2015/06/25 23:05:05, jtolds wrote:
>> > does this even need to happen?
>>
>
>  If you don't remove ".." elements, isn't it trivial to read outside of
>> NaClRootFolder?
>>
>
> currently working through these comments, but i just didn't know if path
> sanitization happened at some higher level or something.


Did you mean in the functions called by sys_filename.c?  Anyhow, no, NaCl
doesn't have any pathname sanitisation code at the moment.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Mark Seaborn, 2015-06-28 21:06:28]

On 27 June 2015 at 09:27, <jtolds@gmail.com> wrote:

> Uh oh, new wrinkle I stupidly didn't realize at first:
>
> Of course of course, paths requested are sometimes relative, which means
> simply
> prepending the prefix is not viable.
>

Right. :-)


> So I see two options and I'm interested in your thoughts:
>
> 1) I've been hoping to sanitize paths completely lexically without hitting
> disk
> (look for .., ., etc, just like how
> https://golang.org/pkg/path/filepath/#Clean
> does it). I can keep doing that, and in the case of a relative path also
> prepend
> the CWD first. I'm kind of leaning toward this one from a security
> perspective,
> but it's more gross.
>
> 2) I could give up on lexical path sanitization and just use realpath calls
> everywhere to check what realpath is getting accessed. This *would* have
> the
> unintended side effect of making symlink support a little easier.
>

Of the two, I think (1) would be better, because it's easier to understand
the implications of it.

(2) potentially leaks information about the filesystem outside of the
virtual root dir.  For example, accessing "/../foo" might succeed if the
virtual root dir is named "foo" but fail otherwise.

For a first version, for simplicity, it would be reasonable to not support
relative pathnames and not support getcwd()/chdir().  I'm not sure if it
would meet your needs, but for some use cases, allowing only absolute
pathnames would still be quite useful.



> Haven't pushed yet, but here's my documentation/ bit about symlinks and
> cwd for
> reference:
>
>   ### Symlinks
>
>   Ideally, symlinks should behave as if we had just chrooted into the path
> given
>   to `-m` (which means they would resolve relative to the untrusted root).
>
>   However, it's not straightforward to have `sel_ldr` call `chroot` itself
> to
>   make the kernel do appropriate symlink resolution in a cross-platform
> way. So
>   our other option to support symlinks is to do an Lstat on every path
> load and
>   try to resolve symlinks ourselves. This would be a pretty big
> performance hit
>   and a significant amount of complication needing auditing.


Potentially the pathname parsing could be done in untrusted code, so that
part wouldn't add attack surface.

Similarly, handling relative pathnames and tracking the current directory
could be done in untrusted code.



> We wouldn't be able
>   to just pass through sanitized paths to the host's open call without
> making
>   sure the untrusted app hadn't created a symlink pointing elsewhere to
> jump out
>   of jail.
>
>   So instead, for v1 we're just not supporting symlinks. With `-m`, the
> syscalls
>   for creating and reading symlinks will fail. However, the path passed to
> `-m`
>   might have symlinks in it, so it is up to the creator of the filesystem
> passed
>   to `-m` to sanitize it for bad (or even relative!) symlinks prior to
> safely
>   running `sel_ldr`. This is a significant amount of sharpness and
> potential
>   insecurity this feature might bring, so it might be worth just failing to
> start
>   if symlinks exist at any subpath from the path given to `-m`. Currently
> we're
>   going to allow the sharpness of this interface.
>
>   The following syscalls relate to symlinks.
>
>    * `NaClSysLstat` - calls the host's `stat` call (not the host's `lstat`)
>    * `NaClSysSymlink` - always fails
>    * `NaClSysReadlink` - always fails
>

It would be OK to allow readlink() and lstat(), because they only read
data.  It's only symlink() that is dangerous, because it causes the kernel
to interpret the symlink data.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[jtolds, 2015-06-30 02:56:55]

> For a first version, for simplicity, it would be reasonable to not support
> relative pathnames and not support getcwd()/chdir().  I'm not sure if it
> would meet your needs, but for some use cases, allowing only absolute
> pathnames would still be quite useful.

I got most of the way through implementation before I saw your comments here.
I'd be happy to rip this code out if you like, but supporting it does 
definitely help make my project easier.

Tests still coming. It looks like most of the existing tests I can find are
tests where the test runs as untrusted code? I'll keep looking (not super
familiar with scons yet) but are there good examples of unit tests? I'd love to
have a good example of how to check in my path sanitization function tests.

Anyway, I'll keep plugging away at tests, but as far as I know, unless the
approach is bad this changeset is done.

[Mark Seaborn, 2015-06-30 07:20:59]

On 29 June 2015 at 19:56, <jtolds@gmail.com> wrote:
>
> Tests still coming. It looks like most of the existing tests I can find are
> tests where the test runs as untrusted code? I'll keep looking (not super
> familiar with scons yet) but are there good examples of unit tests? I'd
> love to
> have a good example of how to check in my path sanitization function tests.


For unit tests, you can add a .cc file to service_runtime_tests, defined
in src/trusted/service_runtime/build.scons.  You can run this test target
with "./scons ... run_service_runtime_tests".

These unit tests uses the gtest framework.
See src/trusted/service_runtime/thread_suspension_test.cc for some example
test cases.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[jtolds, 2015-07-07 16:18:44]

Hey, just wanted to post and apologize for getting this 90% of the way there and
then bailing. My work project I was going to use this for is going with a
different strategy, so this isn't as urgent for me, but I do intend to finish,
hopefully soon. Anyways, sorry for going radio silent all of a sudden.

On 2015/06/30 07:20:59, Mark Seaborn wrote:
> On 29 June 2015 at 19:56, <mailto:jtolds@gmail.com> wrote:
> >
> > Tests still coming. It looks like most of the existing tests I can find are
> > tests where the test runs as untrusted code? I'll keep looking (not super
> > familiar with scons yet) but are there good examples of unit tests? I'd
> > love to
> > have a good example of how to check in my path sanitization function tests.
> 
> 
> For unit tests, you can add a .cc file to service_runtime_tests, defined
> in src/trusted/service_runtime/build.scons.  You can run this test target
> with "./scons ... run_service_runtime_tests".
> 
> These unit tests uses the gtest framework.
> See src/trusted/service_runtime/thread_suspension_test.cc for some example
> test cases.
> 
> Cheers,
> Mark
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:native-client-reviews+unsubscribe@googlegroups.com.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> Visit this group at http://groups.google.com/group/native-client-reviews.
> For more options, visit https://groups.google.com/d/optout.

[Mark Seaborn, 2015-07-20 18:01:00]

On 7 July 2015 at 09:18, <jtolds@gmail.com> wrote:

> Hey, just wanted to post and apologize for getting this 90% of the way
> there and
> then bailing. My work project I was going to use this for is going with a
> different strategy, so this isn't as urgent for me, but I do intend to
> finish,
> hopefully soon. Anyways, sorry for going radio silent all of a sudden.


Thanks for the update, and apologies for being slow to reply myself.

Do you think you would be able to add tests?  I can do another review pass
if you want, but checking for test coverage is a large part of what I
review for.

What I would recommend for end-to-end testing is to add a Python script
that:
 * creates a temporary directory and populates it with some files;
 * runs sel_ldr on a nexe that checks that files inside the granted
directory are accessible and that files outside aren't accessible.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[jtolds, 2015-07-29 05:49:10]

> Thanks for the update, and apologies for being slow to reply myself.
> 
> Do you think you would be able to add tests?  I can do another review pass
> if you want, but checking for test coverage is a large part of what I
> review for.

I just wrapped up running the http://hackthe.computer/ coding competition just
yesterday, which is what I was originally writing this change for (switched to
using https://github.com/google/nsjail instead), and now I'm off on vacation for
10 days.

I'm so sorry for having dropped and ran like this. I do think I can add tests
when I get back, but I'm not sure if you want to wait that long.

> What I would recommend for end-to-end testing is to add a Python script
> that:
>  * creates a temporary directory and populates it with some files;
>  * runs sel_ldr on a nexe that checks that files inside the granted
> directory are accessible and that files outside aren't accessible.

Yeah that sounds like a great testing plan. I'll get to it when I can, but in
the meantime I do believe the changeset is otherwise feature complete, so up to
you what you want to do.

Sorry again, I realize I'm being the most terrible kind of contributor by having
gotten this 90% of the way there and then bailing.

If you're okay waiting I'll get to it, but I'm also totally fine if you want to
find an adoptive owner.

-JT

[Sean Klein, 2015-08-07 20:58:10]

On 2015/07/29 05:49:10, jtolds wrote:
> > Thanks for the update, and apologies for being slow to reply myself.
> > 
> > Do you think you would be able to add tests?  I can do another review pass
> > if you want, but checking for test coverage is a large part of what I
> > review for.
> 
> I just wrapped up running the http://hackthe.computer/ coding competition just
> yesterday, which is what I was originally writing this change for (switched to
> using https://github.com/google/nsjail instead), and now I'm off on vacation
for
> 10 days.
> 
> I'm so sorry for having dropped and ran like this. I do think I can add tests
> when I get back, but I'm not sure if you want to wait that long.
> 
> > What I would recommend for end-to-end testing is to add a Python script
> > that:
> >  * creates a temporary directory and populates it with some files;
> >  * runs sel_ldr on a nexe that checks that files inside the granted
> > directory are accessible and that files outside aren't accessible.
> 
> Yeah that sounds like a great testing plan. I'll get to it when I can, but in
> the meantime I do believe the changeset is otherwise feature complete, so up
to
> you what you want to do.
> 
> Sorry again, I realize I'm being the most terrible kind of contributor by
having
> gotten this 90% of the way there and then bailing.
> 
> If you're okay waiting I'll get to it, but I'm also totally fine if you want
to
> find an adoptive owner.
> 
> -JT

I'll pick up this issue (and also add tests).