Libguestfs - Feb 2023 - [libnbd PATCH v3 09/29] lib/utils: introduce async-signal-safe execvpe()

On Tue, Feb 21, 2023 at 12:55:49PM +0100, Laszlo Ersek
wrote:
> On 2/15/23 23:28, Eric Blake wrote:
> > On Wed, Feb 15, 2023 at 03:11:38PM +0100, Laszlo Ersek wrote:
> >> execvp() [01] is powerful:
> > 
> > You KNOW this is going to be a juicy commit message when the first
> > line contains a 2-digit footnote ;)
> 
> Haha, yes :) Clearly I originally started the commit message with
> single-digit footnotes, but I ended up needing another digit :)
Trimming my responses to just your open questions...
> > _Technically_, PATH= should behave identically to PATH=.; if you have
> > an executable named './ls' in your working directory, bash
WILL
> > execute it!
> > 
> > $ mkdir -p /tmp/blah
> > $ cd /tmp/blah
> > $ cp /bin/ls .
> > $ PATH= ls -a
> > .  ..  ls
> > 
> > But whether you want to tweak this patch to actually comply with that
> > corner case is up to you (no one in their right mind should set PATH
> > to empty).  Below, I'll refer to this as [1].
> 
> Thanks for the observation.
> 
> I'll modify the commit message -- remove the note that this behavior is
> consistent with bash/Linux/glibc. I've re-checked [02], and it's
> implementation-defined what we do when PATH is set to the empty string.
> POSIX does not mandate (even as "legacy") the fallback to
"." when PATH
> is empty. The code was extremely hard to review for myself (much harder
> than just writing it), so I'd *really* like to avoid
"tweaking" it. I'd
> need to review the whole thing from the ground up, and I'm kind of
"out
> of steam" for that, and this is such a corner case indeed that I
don't
> think maintaining the above-noted consistency with linux/glibc/bash is
> worth repeating the whole effort.
> 
> (Not to mention the unit tests in the next patch, which were super
> difficult to *write* as well.)
Yes, I can accept your reasoning that PATH="" is different than
PATH=":" (the latter is well-defined, albeit obsolete, as a synonym to
PATH=".:.", the former is explicitly
"implementation-defined").  So
your argument of leaving this corner-case explicitly documented as our
implementation choice different from glibc, all in order to avoid
further coding efforts, is fine with me.

...
> > 
> > [1] And this simplifies if empty PATH can imply searching in
".".
> 
> If we ever get practical problems with PATH="", let's update
the code
> then, incrementally.
Indeed.  No need to fret about a corner case that no one will hit; or
if they can prove that it matters, it's an easy incremental patch at
that time.
> >> + *   This allocation is made in anticipation of execve() failing
for a
> >> + *   particular candidate inside nbd_internal_fork_safe_execvpe()
with ENOEXEC
> >> + *   ("[t]he new process image file has the appropriate
access permission but
> >> + *   has an unrecognized format"). While that failure
terminates the iteration,
> >> + *   the failed call
> >> + *
> >> + *     execve (pathnames[i],
> >> + *             { argv[0], argv[1], ..., NULL }, // (num_args
>= 2) elements
> >> + *             { envp[0], envp[1], ..., NULL })
> >> + *
> >> + *   must be repeated as
> >> + *
> >> + *     execve (<shell-path>,
> >> + *             { argv[0], pathnames[i],         // ((num_args +
1) >= 3)
> > 
> > Are we guaranteed that if we get to this fallback, pathnames[i] does
> > not begin with '-' or '+'?
> 
> No, but it's not something that POSIX requires either.
> 
> > Or is it possible to write
> > PATH=/usr/bin:+my/evil/relative/path:... and/or the user to attempt to
> > execute '+my/evil/name' which skips the PATH search because
the binary
> > name contains '/', such that the resulting argument to /bin/sh
might
> > need to be protected with an extra "--" argument to avoid
being
> > misinterpreted as options to the shell rather than the script name to
> > be executed?
> 
> Certainly possible, but I don't think it's the job of this code to
> prevent that. I don't think we expect any libnbd application to run as
a
> part of a setuid or setgid application (definitely not without the
> application properly separating privileges), so I think the user
> launching the libnbd application is responsible for setting their
> environment (including PATH) correctly. Also I think libnbd application
> writers are responsible for passing sane arguments to
> nbd_connect_systemd_socket_activation() and nbd_connect_command().
> 
> (To begin with -- if we such privilege escalations were relevant
> threats, we should be using the non-portable secure_getenv() already,
> not getenv()!)
Again agreed - any user desperate enough to pass in an atypical binary
name or put atypical relative directory names in PATH gets what they
deserve; as long as we don't think our code misbehaving on argv[0] of
"+s" can be abused as a security hole, then not worrying about the
corner case in our code is just fine for this patch, and an
incremental patch on top if we can think of why it is
security-sensitive after all.
> > Since we are in the parent process during this _init() function,
can't
> > we just use asprintf() instead of concatenating it all out by hand?
> 
> I thought long and hard about asprintf(), and certainly expected this
> question :)
> 
> My problem with asprintf (and to an extent with sprintf / snprintf) is
> that they return the number of bytes printed (or the number of bytes
> that would have been printed), without the terminating NUL, as a *signed
> int*. I can't fathom how that is an acceptable interface design when
> strlen() and sizeof both produce size_t, and when -- specifically
> regarding asprintf() -- malloc() takes a size_t. The glibc manual is
> super vague on this "int" overflow ("If memory  allocation
wasn't
> possible, or some other error occurs"):
> 
>   https://man7.org/linux/man-pages/man3/asprintf.3.html
> 
> and I can't refer to the POSIX standard, because asprintf() is not
POSIX.
Not yet, but coming to POSIX soon:
https://www.austingroupbugs.net/view.php?id=1496

But avoiding it in the short term doesn't hurt my feelings; it's just
a different set of boilerplate code.

...
> >> +
> >> +        memcpy (p, "/", sep_copy_size);
> >> +        p += sep_copy_size;
> > 
> > Clever way to avoid turning PATH=/:/usr/bin:... into an attempt to
> > execute "//binary" which is NOT the same as
"/binary".
> 
> Well to be honest, I did this specifically because reference [02] on
> PATH states:
> 
>   When a non-zero-length prefix is applied to this filename, a
<slash>
>   shall be inserted between the prefix and the filename if the prefix
>   did not end in <slash>.
> 
> It happens to prevent "//binary" too, but that wasn't
specifically my
> intent.
> 
> How does "//binary" differ from "/binary", in pathname
resolution?
POSIX intentionally says that file names beginning with leading "//"
but not "///" have implementation-defined semantics.  Linux's
definition is that "//" is a synonym for "/", so it makes no
difference there.  But on Cygwin, "/machine" is a file (or directory)
living in the root directory, while "//machine/share" is a network
share access path that locates the shared resource "share" on
network-addressable "machine" (think Windows \\MACHINE\SHARE syntax).
Native windows does not let you access bare \\ or \\MACHINE, but
Cygwin has further generalized those so that "ls //" shows you a list
of all machines currently advertising shares in your local subnet (do
not try it on a large enterprise subnet - it can take a LOONG time).
Cygwin also defaults to having "/cygdrive/c" be a synonym to Windows
C:\, but has an option for you to re-spell it as "//c" (that is,
expose all of your drive letters the same as remote-access machines by
setting the cygdrive prefix to empty instead of "cygdrive").

...
> > 
> > Overall looks like nice work.
> > 
> > Based on my review, you may decide to make further tweaks, but I
> > didn't see any hard reason why we couldn't use the patch
as-written if
> > you don't think my review warrants changes.  Thus:
> > 
> > Reviewed-by: Eric Blake <eblake at redhat.com>
> > 
> 
> Thanks!
> 
> I will update the commit message (fix the double space typo, and remove
> the allusion to bash compatibility near PATH=''), and replace the
FIXME
> with a NOTE in get_path().
> 
> Thanks for the thorough review!
> Laszlo
And thanks again for a detailed commit message and code comments for
something that would otherwise be hard to maintain if we didn't have
strong rationale for why we are doing it.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 2/21/23 18:06, Eric Blake wrote:
> On Tue, Feb 21, 2023 at 12:55:49PM +0100, Laszlo Ersek wrote:
>> On 2/15/23 23:28, Eric Blake wrote:
>>> On Wed, Feb 15, 2023 at 03:11:38PM +0100, Laszlo Ersek wrote:
>>>> execvp() [01] is powerful:
>>>
>>> You KNOW this is going to be a juicy commit message when the first
>>> line contains a 2-digit footnote ;)
>>
>> Haha, yes :) Clearly I originally started the commit message with
>> single-digit footnotes, but I ended up needing another digit :)
> 
> Trimming my responses to just your open questions...
Sorry, I should have been trimming too.
>>> [1] And this simplifies if empty PATH can imply searching in
".".
>>
>> If we ever get practical problems with PATH="", let's
update the code
>> then, incrementally.
> 
> Indeed.  No need to fret about a corner case that no one will hit; or
> if they can prove that it matters, it's an easy incremental patch at
> that time.
Well, given Daniel's comments meanwhile, it seems like the original
execvp() is something we shouldn't fret about. :/
> Again agreed - any user desperate enough to pass in an atypical binary
> name or put atypical relative directory names in PATH gets what they
> deserve; as long as we don't think our code misbehaving on argv[0] of
> "+s" can be abused as a security hole, then not worrying about
the
> corner case in our code is just fine for this patch, and an
> incremental patch on top if we can think of why it is
> security-sensitive after all.
Can you please elaborate on "+s"? (I'd like to understand your
point
regardless of this patch, too.)
>> How does "//binary" differ from "/binary", in
pathname resolution?
> 
> POSIX intentionally says that file names beginning with leading
"//"
> but not "///" have implementation-defined semantics.  Linux's
> definition is that "//" is a synonym for "/", so it
makes no
> difference there.  But on Cygwin, "/machine" is a file (or
directory)
> living in the root directory, while "//machine/share" is a
network
> share access path that locates the shared resource "share" on
> network-addressable "machine" (think Windows \\MACHINE\SHARE
syntax).
> Native windows does not let you access bare \\ or \\MACHINE, but
> Cygwin has further generalized those so that "ls //" shows you a
list
> of all machines currently advertising shares in your local subnet (do
> not try it on a large enterprise subnet - it can take a LOONG time).
> Cygwin also defaults to having "/cygdrive/c" be a synonym to
Windows
> C:\, but has an option for you to re-spell it as "//c" (that is,
> expose all of your drive letters the same as remote-access machines by
> setting the cygdrive prefix to empty instead of "cygdrive").
Ah, found it:

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13

"""
If a pathname begins with two successive <slash> characters, the first
component following the leading <slash> characters may be interpreted in
an implementation-defined manner, although more than two leading <slash>
characters shall be treated as a single <slash> character.
"""

(... I wanted to say that "there's no replacement for reading POSIX in
full", but now I'm wondering if reading POSIX *at all* makes sense...)
> And thanks again for a detailed commit message and code comments for
> something that would otherwise be hard to maintain if we didn't have
> strong rationale for why we are doing it.
... That rationale may just have fallen away.

Laszlo