Re: [Chicken-hackers] Fix buffer overflow found by #1308

[Peter Bex]

On Sat, Jul 23, 2016 at 10:26:49PM +0200, Christian Kellermann wrote:
> Hi!
> 
> The attached patch adds range checks avoiding a buffer overflow for
> the environment and argument buffers used in the posix execvp/execve
> wrappers on linux and windows.
> 
> Does that look OK?

We've discussed this some more on IRC, and I think that we all agree
that it's better to refactor this whole mess.  The attached patch is
a complete overhaul of process-execute and process-spawn (which was
equally affected by the ARG_MAX/ENV_MAX sized buffer overrun).

Why I think this substantial refactoring is a good idea:

- ARG_MAX does not represent the _number_ of arguments, but the total
   size of the memory slab holding both the arguments PLUS the
   environment (so this includes the argument / environment strings
   themselves, not just the vector).  ENV_MAX doesn't seem to exist (?!)

- In modern glibc-based Linuxen, ARG_MAX is no longer being defined
   by limits.h.  Instead, you can query it using sysconf(3).
   Our fallback code defining it to be 256 is conservative to the point
   of being useless (especially if you take into account the preceding
   bullet point).

- There's no need to check the buffer length in advance, the kernel will
   guard against too large sizes, so the syscall will fail, and we can
   simply check errno when the function returns (but see below).

- There's too much C in the old implementation.  This code doesn't have
   to be blazingly fast, and it should really be safe.  So it makes more
   sense to do the array creation and indexing in Scheme too.  The
   patch uses a pointer-vector.

- Case in point: the original code also had a nasty memory leak: if the
   arguments or environment list contained non-string values, it would
   raise an exception, but it didn't free the already-allocated strings
   in the vectors.  The current call-with-exec-args takes care to do
   this correctly.

- This code is kind of long, and tricky.  I decided to put as much shared
   and reusable code as possible into posix-common.scm.  The shell
   quoting stuff is simply a "conversion" procedure you can pass to
   call-with-exec-args, which in the unix case is the IDENTITY procedure
   (but not by that name, because we don't load data-structures here).


The one disadvantage of this approach, which DeeEff on IRC found out, is
that on Windows, the errno is _always_ set to ENOFILE, even if you pass
too many arguments.  This is misleading, because the error message is
"no such file", which is the same error you get if the command is unknown.

Cheers,
Peter

0001-Fix-buffer-overflow-in-posix-execvp-execve-wrapper.patch
Description: Text Data

signature.asc
Description: Digital signature