Add an ELF loader that can load and run an executable in the sandbox

Add an ELF loader that can load and run an executable in the sandbox

This can load both statically-linked executables and ld.so (the
dynamic linker).  These have to be pre-patched to do syscalls via
"int $0" - I will add a tool for doing this patching in another change.

The ELF loader itself also runs with sandboxing enabled.

Currently the loader is dynamically linked against glibc, which will
probably cause problems when another copy of libc gets loaded into the
process, so in the long term we will want to statically link the
loader.

BUG=http://code.google.com/p/seccompsandbox/issues/detail?id=13
TEST=to be added in a later change

Committed: http://code.google.com/p/seccompsandbox/source/detail?r=171

[Markus (顧孟勤), 2011-08-15 22:05:23]

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc
File elf_loader.cc (right):

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode24
elf_loader.cc:24: # error "Unsupported target platform"
I keep making this same mistake all the time. There shouldn't be any quotes
around the string in an "#error" statement. Yes, it is confusing...

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode28
elf_loader.cc:28: uintptr_t PageSizeRoundDown(uintptr_t val) {
Are you deliberately not making these functions "static"? It might in fact make
sense to make them globally available, but in that case I wonder whether this is
the best place to put them.

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode29
elf_loader.cc:29: return val & ~(getpagesize() - 1);
You can also write:

 val & -getpagesize()

Not sure which one is more readable. Ignore this comment, if you think your
statement works better.

And of course, throughout the rest of the code, we just assume that the
user-visible pagesize is always 4096 on the platforms we care about. The people
on the kernel mailing list seem divided on whether that is correct or not.

I remember Linus saying that this will never change, but others claim that he
his since softened his stance on this question.

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode33
elf_loader.cc:33: return (val + getpagesize() - 1) & ~(getpagesize() - 1);
Maybe:

  return PageSizeRoundDown(val + getpagesize() - 1);

The compiler should optimize both versions to do the same.

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode40
elf_loader.cc:40: ptr++;
I'd prefer "{ }" around this statement. But maybe, that's just Google3-habits
failing to die :-)

Been burned by not having braces one too many times.

Alternatively, you could always write:

  while (*ptr++) { }

You wouldn't need the extra increment outside of the block. But that might be
too condensed and hard to read.

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode51
elf_loader.cc:51: }
Is it OK, if we fail to find the matching type? Should we die() when this
happens?

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode55
elf_loader.cc:55: void *atexit_func) {
You might consider declaring this function as __attribute__((noreturn))

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode83
elf_loader.cc:83: ssize_t phdrs_size = elf_header.e_phentsize *
elf_header.e_phnum;
Are we worried about malicious binaries overflowing the the phdrs_size variable?
Are we worried about e_phtentsize or e_phnum being negative? (more precisely,
they are currently unsigned, but they subsequently get used in situations where
they would be treated as signed)

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode84
elf_loader.cc:84: char *phdrs = (char *) alloca(phdrs_size);
You should check that the alloca() request succeeded.

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode100
elf_loader.cc:100: if (elf_header.e_type == ET_EXEC) {
I take it, we should see no or exactly one ET_EXEC header. Can we check this?
Maybe also add a comment explaining the two possible cases, as that makes the
next couple of lines more obvious.

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode105
elf_loader.cc:105: 
You might want to add a sanity check that "load_size" is positive and non-zero.

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode129
elf_loader.cc:129: // offset, but Linux assumes this too when setting AT_PHDR.
Any chance that we could actually test this assumption at run-time? I know, I'm
paranoid. Maybe, overly so. Tell me to shut up, if that's the case.

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode137
elf_loader.cc:137: size_t segment_end = PageSizeRoundUp(segment->p_vaddr +
segment->p_memsz);
Again, do we worry about overflows?

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode145
elf_loader.cc:145: void *result = mmap(load_offset + segment_start,
Don't we have the same problem as for the other call to mmap()? MAP_FIXED is
potentially going to overwrite things that shouldn't be clobbered.

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode152
elf_loader.cc:152: // TODO(mseaborn): Support a BSS that goes beyond the file's
extent.
What happens if we encounter something like that? Is the loader going to crash?
The loaded application?

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode155
elf_loader.cc:155: uintptr_t bss_start = segment->p_vaddr + segment->p_filesz;
Do we need sanity checks that we don't zero out something that we shouldn't
zero?

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode180
elf_loader.cc:180: playground::g_policy.allow_file_namespace = true;
We probably want a more fine-grained policy at some point. How difficult is is
to do what "ldd" does and find all the files that the program depends on?

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode185
elf_loader.cc:185: SetAuxvField(auxv, AT_SYSINFO, (uintptr_t)
syscallEntryPointNoFrame);
Does this work on both x86-32 and x86-64. I vaguely recall that glibc behaves
differently on these two platforms.

http://codereview.chromium.org/7634024/diff/1/elf_loader.cc#newcode187
elf_loader.cc:187: *(long *) stack = argc - 1;
Are there alignment requirements that we need to worry about? Do we have a
guarantee that argv is already aligned properly?

[Mark Seaborn, 2011-11-10 01:29:19]

I'm returning to this change after a break. :-)

I've redone a chunk of it because I realised it was calculating the overall
mapping size wrongly for ET_EXEC executables (it was reserving more address
space than was needed).

On 15 August 2011 15:05, <markus@chromium.org> wrote:

>
>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc<http://codere...
> File elf_loader.cc (right):
>
>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode24<htt...
> elf_loader.cc:24: # error "Unsupported target platform"
> I keep making this same mistake all the time. There shouldn't be any
> quotes around the string in an "#error" statement. Yes, it is
> confusing...
>

I've tended to use quotes because otherwise gcc warns in cases where there
are apostrophes in the text.  I don't think the quotes hurt.  But changed,
anyway.


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode28<htt...
> elf_loader.cc:28: uintptr_t PageSizeRoundDown(uintptr_t val) {
> Are you deliberately not making these functions "static"? It might in
> fact make sense to make them globally available, but in that case I
> wonder whether this is the best place to put them.
>

An oversight on my part.  Changed.


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode29<htt...
> elf_loader.cc:29: return val & ~(getpagesize() - 1);
> You can also write:
>
>  val & -getpagesize()
>
> Not sure which one is more readable. Ignore this comment, if you think
> your statement works better.
>
> And of course, throughout the rest of the code, we just assume that the
> user-visible pagesize is always 4096 on the platforms we care about.


Elsewhere I might do that, but using getpagesize() is more
self-documenting. :-)


> The people on the kernel mailing list seem divided on whether that is
> correct or not.
>
> I remember Linus saying that this will never change, but others claim
> that he his since softened his stance on this question.
>
>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode33<htt...
> elf_loader.cc:33: return (val + getpagesize() - 1) & ~(getpagesize() -
> 1);
> Maybe:
>
>  return PageSizeRoundDown(val + getpagesize() - 1);
>
> The compiler should optimize both versions to do the same.
>

Done.


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode40<htt...
> elf_loader.cc:40: ptr++;
> I'd prefer "{ }" around this statement. But maybe, that's just
> Google3-habits failing to die :-)
>

Done.


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode51<htt...
> elf_loader.cc:51: }
> Is it OK, if we fail to find the matching type? Should we die() when
> this happens?
>

It's kind of OK:  if the kernel loaded us without this auxv field, then
presumably ld.so and other executables can cope with this field's absence.
I could put in a warning...


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode55<htt...
> elf_loader.cc:55: void *atexit_func) {
> You might consider declaring this function as __attribute__((noreturn))
>

I tried that, but I get:

cc1plus: warnings being treated as errors
elf_loader.cc: In function ‘void JumpToElfEntryPoint(void*, void*, void*)’:
elf_loader.cc:71: error: ‘noreturn’ function does return

The compiler doesn't understand that the asm() does not return.  What I'd
really want is a noreturn declaration on the asm().


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode83<htt...
> elf_loader.cc:83: ssize_t phdrs_size = elf_header.e_phentsize *
> elf_header.e_phnum;
> Are we worried about malicious binaries overflowing the the phdrs_size
> variable? Are we worried about e_phtentsize or e_phnum being negative?
> (more precisely, they are currently unsigned, but they subsequently get
> used in situations where they would be treated as signed)
>

I've put in a limit for e_phnum as a sanity check.

In general I am not worried about overflows, because this ELF loader is
sandboxed.  I could put in checks, but it's not trivial to test that they
are correct.


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode84<htt...
> elf_loader.cc:84: char *phdrs = (char *) alloca(phdrs_size);
> You should check that the alloca() request succeeded.
>

I'm not sure alloca() can return an error, actually.  It is just shifting
the stack pointer, after all.  The man page doesn't say it can return NULL.

Anyway, I have changed it to place an arbitrary limit on e_phnum and not
use alloca().


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode100<ht...
> elf_loader.cc:100: if (elf_header.e_type == ET_EXEC) {
> I take it, we should see no or exactly one ET_EXEC header. Can we check
> this? Maybe also add a comment explaining the two possible cases, as
> that makes the next couple of lines more obvious.
>

I noticed the code was calculating the overall size wrongly for ET_EXEC, so
I've redone it.  It should now be more obvious that ET_EXEC is an overall
condition, and not per-segment.


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode105<ht...
> elf_loader.cc:105:
> You might want to add a sanity check that "load_size" is positive and
> non-zero.
>

I think the mmap is going to fail if load_size is negative...


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode129<ht...
> elf_loader.cc:129: // offset, but Linux assumes this too when setting
> AT_PHDR.
> Any chance that we could actually test this assumption at run-time? I
> know, I'm paranoid. Maybe, overly so. Tell me to shut up, if that's the
> case.
>

I don't think we need to worry about badly-linked executables...


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode137<ht...
> elf_loader.cc:137: size_t segment_end = PageSizeRoundUp(segment->p_**vaddr
> + segment->p_memsz);
> Again, do we worry about overflows?
>

Same here.


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode145<ht...
> elf_loader.cc:145: void *result = mmap(load_offset + segment_start,
> Don't we have the same problem as for the other call to mmap()?
> MAP_FIXED is potentially going to overwrite things that shouldn't be
> clobbered.
>

In this case we are intentionally overwriting parts of the range that we
initially reserved with the first mmap() call.


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode152<ht...
> elf_loader.cc:152: // TODO(mseaborn): Support a BSS that goes beyond the
> file's extent.
> What happens if we encounter something like that? Is the loader going to
> crash? The loaded application?
>

The loader would crash when zeroing the BSS.  You can mmap() pages with
offsets beyond the file's extent, but you can't write to them, even if they
are MAP_PRIVATE.

I have some test cases for this ELF loader.  They depend on rewrite.cc
which I was going to add in a separate commit.  I was going to fix the BSS
handling in a separate commit so that it can be some in the same commit as
adding a test for it.


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode155<ht...
> elf_loader.cc:155: uintptr_t bss_start = segment->p_vaddr +
> segment->p_filesz;
> Do we need sanity checks that we don't zero out something that we
> shouldn't zero?
>

We don't need them for sandbox safety...


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode180<ht...
> elf_loader.cc:180: playground::g_policy.allow_**file_namespace = true;
> We probably want a more fine-grained policy at some point. How difficult
> is is to do what "ldd" does and find all the files that the program
> depends on?
>

Ah, bigger question.  My plan is to translate calls like open() into IPC
calls to a server process which implements a virtualised file namespace, as
in Plash (http://plash.beasts.org).  Plash has various ways of populating a
file namespace with libraries and other files...


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode185<ht...
> elf_loader.cc:185: SetAuxvField(auxv, AT_SYSINFO, (uintptr_t)
> syscallEntryPointNoFrame);
> Does this work on both x86-32 and x86-64.


Yep.  This works with the tests that I plan to commit separately.


> I vaguely recall that glibc
> behaves differently on these two platforms.
>

There are variations in when it uses AT_SYSINFO, but the interface is
basically the same.


>
http://codereview.chromium.**org/7634024/diff/1/elf_loader.**cc#newcode187<ht...
> elf_loader.cc:187: *(long *) stack = argc - 1;
> Are there alignment requirements that we need to worry about? Do we have
> a guarantee that argv is already aligned properly?
>

%esp/%rsp do not have to be aligned on entry to the ELF entry point.  ld.so
or start.S generally realigns the stack pointer itself.

Mark

[Markus (顧孟勤), 2011-11-14 23:48:27]

LGTM

Thanks!