[2/2] linux: posix_spawn: return EINVAL on argc < 1

  posix_spawn allows argc < 1 but the specification says that
"The value in argv[0] should point to a filename string.."
So there must be at least one non-null argument.

Signed-off-by: Cristian Rodríguez <crrodriguez@opensuse.org>
---
 sysdeps/unix/sysv/linux/spawni.c | 5 +++++
 1 file changed, 5 insertions(+)
  

This apparently does not do what I wanted..

if (argv[0] == NULL || !strlen(argv[0]))
    {
    errno = EINVAL;
    return errno;
}

is a better check I think.

On Fri, Jan 28, 2022 at 10:48 AM Cristian Rodríguez
<crrodriguez@opensuse.org> wrote:
>
> posix_spawn allows argc < 1 but the specification says that
> "The value in argv[0] should point to a filename string.."
> So there must be at least one non-null argument.
>
> Signed-off-by: Cristian Rodríguez <crrodriguez@opensuse.org>
> ---
>  sysdeps/unix/sysv/linux/spawni.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index 93359c708b..23617c9165 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -334,6 +334,11 @@ __spawnix (pid_t * pid, const char *file,
>         errno = E2BIG;
>         return errno;
>        }
> +    if (argc < 1)
> +      {
> +       errno = EINVAL;
> +       return errno;
> +      }
>
>    int prot = (PROT_READ | PROT_WRITE
>              | ((GL (dl_stack_flags) & PF_X) ? PROT_EXEC : 0));
> --
> 2.34.1
  

On 28/01/2022 12:26, Cristian Rodríguez wrote:
> This apparently does not do what I wanted..
> 
> if (argv[0] == NULL || !strlen(argv[0]))
>     {
>     errno = EINVAL;
>     return errno;
> }
> 
> is a better check I think.
> 
> On Fri, Jan 28, 2022 at 10:48 AM Cristian Rodríguez
> <crrodriguez@opensuse.org> wrote:
>>
>> posix_spawn allows argc < 1 but the specification says that
>> "The value in argv[0] should point to a filename string.."
>> So there must be at least one non-null argument.
>>
>> Signed-off-by: Cristian Rodríguez <crrodriguez@opensuse.org>
>> ---
>>  sysdeps/unix/sysv/linux/spawni.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
>> index 93359c708b..23617c9165 100644
>> --- a/sysdeps/unix/sysv/linux/spawni.c
>> +++ b/sysdeps/unix/sysv/linux/spawni.c
>> @@ -334,6 +334,11 @@ __spawnix (pid_t * pid, const char *file,
>>         errno = E2BIG;
>>         return errno;
>>        }
>> +    if (argc < 1)
>> +      {
>> +       errno = EINVAL;
>> +       return errno;
>> +      }
>>
>>    int prot = (PROT_READ | PROT_WRITE
>>              | ((GL (dl_stack_flags) & PF_X) ? PROT_EXEC : 0));
>> --
>> 2.34.1


Since Linux is discussing changing on execve syscall [1], I think it would be
better to the same not only for posix_spawn, but rather to all execve
functions.

And since all ends up calling execve, even posix_spawn, I think it would be
better to:

  1. Make posix/execve.c call __execveat.
    1.1. It would also allow remove the Hurd implementation 'sysdeps/mach/hurd/execve.c'
  2. Add the proper check on generic, Linux, and Hurd implementation execveat.
    2.1. Maybe even add __execveat_internal that just issue the syscall and let the
         generic wrapper handle the argument parsing.
  3. Add a regression test.

I also think returning EINVAL is better than the kernel EFAULT one (it seems that
the last message on thread does settle for that).

[1] https://lore.kernel.org/all/20220126114447.25776-1-ariadne@dereferenced.org/
  

On Fri, 28 Jan 2022, Adhemerval Zanella via Libc-alpha wrote:

> Since Linux is discussing changing on execve syscall [1], I think it would be
> better to the same not only for posix_spawn, but rather to all execve
> functions.
> 
> And since all ends up calling execve, even posix_spawn, I think it would be
> better to:
> 
>   1. Make posix/execve.c call __execveat.
>     1.1. It would also allow remove the Hurd implementation 'sysdeps/mach/hurd/execve.c'
>   2. Add the proper check on generic, Linux, and Hurd implementation execveat.
>     2.1. Maybe even add __execveat_internal that just issue the syscall and let the
>          generic wrapper handle the argument parsing.
>   3. Add a regression test.
> 
> I also think returning EINVAL is better than the kernel EFAULT one (it 
> seems that the last message on thread does settle for that).

Apart from the need for a test, a note under "Deprecated and removed 
features, and other changes affecting compatibility" in NEWS, and 
documentation in the manual if there's an appropriate place for it to go 
(there is for execve; posix_spawn isn't documented in the manual at all), 
I'd also think it would be better in terms of application compatibility to 
construct an array { pathname, NULL } and pass that in place of argv when 
argv[0] is NULL (*not* when argv[0] is an empty string, I don't see a 
problem with an empty string there), rather than returning an error.  
That's more similar in spirit to what we do with reopening fds 0, 1, 2 if 
not open at startup (but I'd also tend to think the kernel is a better 
place than libc to deal with this, given that anything the *calling* 
program does in userspace with execve can't avoid security issues in the 
*called* program with NULL argv[0] - in the case of fds 0, 1, 2 glibc is 
addressing the problem state directly in the *called* process).

I don't think the "should" in the POSIX specification of posix_spawn is 
very relevant as a justification for the patch (it only requires things 
for Strictly Conforming POSIX Applications).
  

hi, thanks for reading...

On Fri, Jan 28, 2022 at 2:07 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:

>   2. Add the proper check on generic, Linux, and Hurd implementation execveat.

but what is the proper check.. ?

if (path == NULL || argv == NULL || envp == NULL || argv[0] == NULL)
    {
      __set_errno (EINVAL);
      return -1;
  }

or should I also check fd for validity.. I do not think so.. rhave to
special case various combinations in that case... (AT_FCWD, absolute
vs relative "path".. vs dirfd..nope..)
  

On 28/01/2022 15:03, Joseph Myers wrote:
> On Fri, 28 Jan 2022, Adhemerval Zanella via Libc-alpha wrote:
> 
>> Since Linux is discussing changing on execve syscall [1], I think it would be
>> better to the same not only for posix_spawn, but rather to all execve
>> functions.
>>
>> And since all ends up calling execve, even posix_spawn, I think it would be
>> better to:
>>
>>   1. Make posix/execve.c call __execveat.
>>     1.1. It would also allow remove the Hurd implementation 'sysdeps/mach/hurd/execve.c'
>>   2. Add the proper check on generic, Linux, and Hurd implementation execveat.
>>     2.1. Maybe even add __execveat_internal that just issue the syscall and let the
>>          generic wrapper handle the argument parsing.
>>   3. Add a regression test.
>>
>> I also think returning EINVAL is better than the kernel EFAULT one (it 
>> seems that the last message on thread does settle for that).
> 
> Apart from the need for a test, a note under "Deprecated and removed 
> features, and other changes affecting compatibility" in NEWS, and 
> documentation in the manual if there's an appropriate place for it to go 
> (there is for execve; posix_spawn isn't documented in the manual at all), 

We can add a note on deprecated and removed features, but I am not sure
if would make any difference now that kernels will changes the semantic
anyway and it will cause compatibility issues anyway.

> I'd also think it would be better in terms of application compatibility to 
> construct an array { pathname, NULL } and pass that in place of argv when 
> argv[0] is NULL (*not* when argv[0] is an empty string, I don't see a 
> problem with an empty string there), rather than returning an error.  

I am really not sure about keeping compatibility here, applications running
on newer kernels with older glibc will need to handle it anyway and I think
it is good move forward to mask this wrong usage.  Also, other system already
explicit disallow it, so it is a good indication that portable programs should
not rely on this behavior.

> That's more similar in spirit to what we do with reopening fds 0, 1, 2 if 
> not open at startup (but I'd also tend to think the kernel is a better 
> place than libc to deal with this, given that anything the *calling* 
> program does in userspace with execve can't avoid security issues in the 
> *called* program with NULL argv[0] - in the case of fds 0, 1, 2 glibc is 
> addressing the problem state directly in the *called* process).
> 
> I don't think the "should" in the POSIX specification of posix_spawn is 
> very relevant as a justification for the patch (it only requires things 
> for Strictly Conforming POSIX Applications).
>
  

On Fri, 28 Jan 2022, Adhemerval Zanella via Libc-alpha wrote:

> We can add a note on deprecated and removed features, but I am not sure
> if would make any difference now that kernels will changes the semantic
> anyway and it will cause compatibility issues anyway.

I haven't seen any discussions of this on linux-api, so the kernel 
discussion elsewhere might not be taking any compatibility issues properly 
into account.
  

On Fri, Jan 28, 2022 at 8:26 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Fri, 28 Jan 2022, Adhemerval Zanella via Libc-alpha wrote:
>
> > We can add a note on deprecated and removed features, but I am not sure
> > if would make any difference now that kernels will changes the semantic
> > anyway and it will cause compatibility issues anyway.
>
> I haven't seen any discussions of this on linux-api, so the kernel
> discussion elsewhere might not be taking any compatibility issues properly
> into account.

but making things in libc calling execve potentially  with argc == 0
error out or behave like having argv = {"", NULL} shouldn't break
anything right ?
  

On Fri, 28 Jan 2022, Cristian Rodríguez wrote:

> On Fri, Jan 28, 2022 at 8:26 PM Joseph Myers <joseph@codesourcery.com> wrote:
> >
> > On Fri, 28 Jan 2022, Adhemerval Zanella via Libc-alpha wrote:
> >
> > > We can add a note on deprecated and removed features, but I am not sure
> > > if would make any difference now that kernels will changes the semantic
> > > anyway and it will cause compatibility issues anyway.
> >
> > I haven't seen any discussions of this on linux-api, so the kernel
> > discussion elsewhere might not be taking any compatibility issues properly
> > into account.
> 
> but making things in libc calling execve potentially  with argc == 0
> error out or behave like having argv = {"", NULL} shouldn't break
> anything right ?

Errors for argc == 0 (with argv != NULL) are contrary to POSIX, which 
allows that case other than for Strictly Conforming POSIX Applications, 
that being the meaning of "should" here.  (It's possible that in fact many 
applications that would break also use argv == NULL, which isn't valid in 
POSIX, rather than argv pointing to an array containing a single NULL 
pointer, which is valid.)
  

On Fri, Jan 28, 2022 at 8:49 PM Joseph Myers <joseph@codesourcery.com> wrote:

> Errors for argc == 0 (with argv != NULL) are contrary to POSIX, which
> allows that case other than for Strictly Conforming POSIX Applications,

The case you mention as valid are rejected by BSDs.. QNX and others.
so portable applications cannot rely on this either.
.
freebsd head and openbsd -EINVAL with argc == 0 and has never allowed
argv to be NULL  (EFAULTs)
  

On Fri, Jan 28, 2022 at 10:04 PM Cristian Rodríguez
<crrodriguez@opensuse.org> wrote:
>
> On Fri, Jan 28, 2022 at 8:49 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> > Errors for argc == 0 (with argv != NULL) are contrary to POSIX, which
> > allows that case other than for Strictly Conforming POSIX Applications,
>
> The case you mention as valid are rejected by BSDs.. QNX and others.
> so portable applications cannot rely on this either.
> .
> freebsd head and openbsd -EINVAL with argc == 0 and has never allowed
> argv to be NULL  (EFAULTs)

it has been now addressed in the kernel

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=dcd46d897adb70d63e025f175a00a89797d31a43

I think all the linux specific code in glibc at least should behave
like that for consistency.