getgroups(2) with negative values

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

getgroups(2) with negative values

Moritz Buhl-3
Hi,

while porting some NetBSD syscall tests to OpenBSD I noticed that the
getgroups test is failing. Simply put:

gid_t gidset[NGROUPS_MAX];
getgroups(-1, gidset);

This was fixed on NetBSD 8 years ago:
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/kern/kern_prot.c
> if (SCARG(uap, gidsetsize) < (int)*retval)
> return EINVAL;

While here, also remove the u_int in setgroups. POSIX does't say a lot
about setgroups and therefore return EINVAL.
https://pubs.opengroup.org/onlinepubs/9699919799/

Index: sys/kern/kern_prot.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_prot.c,v
retrieving revision 1.75
diff -u -p -r1.75 kern_prot.c
--- sys/kern/kern_prot.c 22 Jun 2018 13:33:30 -0000 1.75
+++ sys/kern/kern_prot.c 8 Jul 2019 12:13:04 -0000
@@ -196,7 +196,7 @@ sys_getgroups(struct proc *p, void *v, r
  syscallarg(gid_t *) gidset;
  } */ *uap = v;
  struct ucred *uc = p->p_ucred;
- u_int ngrp;
+ int ngrp;
  int error;
 
  if ((ngrp = SCARG(uap, gidsetsize)) == 0) {
@@ -870,13 +870,13 @@ sys_setgroups(struct proc *p, void *v, r
  struct process *pr = p->p_p;
  struct ucred *pruc, *newcred;
  gid_t groups[NGROUPS_MAX];
- u_int ngrp;
+ int ngrp;
  int error;
 
  if ((error = suser(p)) != 0)
  return (error);
  ngrp = SCARG(uap, gidsetsize);
- if (ngrp > NGROUPS_MAX)
+ if (ngrp > NGROUPS_MAX || ngrp < 0)
  return (EINVAL);
  error = copyin(SCARG(uap, gidset), groups, ngrp * sizeof(gid_t));
  if (error == 0) {

Reply | Threaded
Open this post in threaded view
|

Re: getgroups(2) with negative values

Todd C. Miller-3
On Mon, 08 Jul 2019 16:36:05 +0200, Moritz Buhl wrote:

> while porting some NetBSD syscall tests to OpenBSD I noticed that the
> getgroups test is failing. Simply put:
>
> gid_t gidset[NGROUPS_MAX];
> getgroups(-1, gidset);
>
> This was fixed on NetBSD 8 years ago:
> http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/kern/kern_prot.c
> > if (SCARG(uap, gidsetsize) < (int)*retval)
> > return EINVAL;
>
> While here, also remove the u_int in setgroups. POSIX does't say a lot
> about setgroups and therefore return EINVAL.
> https://pubs.opengroup.org/onlinepubs/9699919799/

That all makes sense to me.  Checking for ngrp<0 in setgroups()
makes things easier to understand compared to the implicit cast to
uint and relying on the result to be >NGROUPS_MAX.

 - todd