| Message ID | 20220128133937.9555-1-crrodriguez@opensuse.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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.
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));
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(+)