[PATCH] addgroup could assign already in use group

[tito]

On Monday 09 February 2015 01:30:53 you wrote:
> On Sat, Sep 20, 2014 at 4:45 PM, tito <farmatito at tiscali.it> wrote:
> > while looking at the long username stuff and malloced getpwxx functions
> > I've noticed a bug in addgroup resulting in assignement of a wrong
> > (already in use) gid. In the function xgroup_study:
> >
> > static void xgroup_study(struct group *g)
> > {
> >
> > snip
> >
> >         /* Check if the desired gid is free
> >          * or find the first free one */
> >         while (1) {
> >                 if (!getgrgid(g->gr_gid)) {
> >
> >
> >                         return; /* found free group: return */
> >                 }
> > snip
> >
> >                 g->gr_gid++;
> >         }
> > }
> >
> > The call to getgrgid can return NULL also in case of error:
> >
> >        0 or ENOENT or ESRCH or EBADF or EPERM or ...
> >               The given name or gid was not found.
> >
> >        EINTR  A signal was caught.
> >
> >        EIO    I/O error.
> >
> >        EMFILE The maximum number (OPEN_MAX) of files was open already in the calling process.
> >
> >        ENFILE The maximum number of files was open already in the system.
> >
> >        ENOMEM Insufficient memory to allocate group structure.
> >
> >        ERANGE Insufficient buffer space supplied.
> 
> Unfortunately, bbox and a lot of other code has tons of similar bugs.
> For example, fopen() returning NULL is usually treated as "file does
> not exist", and if this was an attempts to read an optional file (e.g.
> conf file...), the program happily continues, wrongly assuming that
> file does not exist, whereas it can be EMFILE error.

Hi,

still that bugs me every time i see this code and i think
that in case of whatever error and  getgrgid returning NULL
iterating through all gids until CONFIG_LAST_ID and erroring
out with a message is better than assign wrong (duplicated?) gids to a group.
But you are the boss....

BTW.: i suspect similar issues could exist also in adduser
but haave not checked yet.

Ciao,
Tito

> Mass fixing this probably would add too much code obfuscation compared
> to marginal gains in error handling.
> 
> In this particular case...
> 
> if you are getting EIO, addgroup not working right is not your biggest worry.
> 
> EINTR is unlikely or even impossible (we do not have any active signal
> handlers, so EINTR can only be generated by e.g. subtle bugs in
> ptrace).
> 
> EMFILE is possible if addgroup was deliberately run under ulimit -n 3.
> Don't do that :)
> 
> and so on...
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20150218/6fd981c2/attachment.html>