Bug 8586 - adduser without -s (shell) creates invalid /etc/passwd entry
Summary: adduser without -s (shell) creates invalid /etc/passwd entry
Status: RESOLVED FIXED
Alias: None
Product: Busybox
Classification: Unclassified
Component: Other (show other bugs)
Version: 1.22.x
Hardware: All Linux
: P5 normal
Target Milestone: ---
Assignee: unassigned
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-05 13:40 UTC by Jonathon Reinhart
Modified: 2019-01-12 10:53 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:


Attachments
Patch to fix this issue (1.33 KB, patch)
2019-01-11 23:09 UTC, 5cli67sg4k
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathon Reinhart 2016-01-05 13:40:17 UTC
Running busybox from the following docker image:

$ docker images
docker.io/busybox                latest              8c2e06607696        8 months ago        2.43 MB

$ docker run --rm -it busybox
/ # adduser -D foouser
/ # cat /etc/passwd
root:x:0:0:root:/root:/bin/sh
daemon:x:1:1:daemon:/usr/sbin:/bin/sh
bin:x:2:2:bin:/bin:/bin/sh
sys:x:3:3:sys:/dev:/bin/sh
sync:x:4:100:sync:/bin:/bin/sync
mail:x:8:8:mail:/var/spool/mail:/bin/sh
proxy:x:13:13:proxy:/bin:/bin/sh
www-data:x:33:33:www-data:/var/www:/bin/sh
backup:x:34:34:backup:/var/backups:/bin/sh
operator:x:37:37:Operator:/var:/bin/sh
haldaemon:x:68:68:hald:/:/bin/sh
dbus:x:81:81:dbus:/var/run/dbus:/bin/sh
ftp:x:83:83:ftp:/home/ftp:/bin/sh
nobody:x:99:99:nobody:/home:/bin/sh
sshd:x:103:99:Operator:/var:/bin/sh
default:x:1000:1000:Default non-root user:/home/default:/bin/sh
foouser:x:1001:1001:Linux User,,,:/home/foouser:fault non-root user
/ # busybox
BusyBox v1.22.1 (2014-05-22 23:22:11 UTC) multi-call binary.
...

You can see that foouser has a bogus shell.

This appears to be fixed in v1.24.1, but I can find no indication at https://www.busybox.net/news.html, or in Git as to what was broken.
Comment 1 Dennis Schridde 2017-01-25 19:37:50 UTC
I can still confirm this in Alpine Linux 3.5 with busybox v1.25.1:
```
$ docker run -ti alpine:3.5 /bin/sh
/ # addgroup -g 1000 xgroup
/ # adduser -u 1012 -G xgroup -h /home/user -D xuser
/ # grep xuser /etc/passwd
xuser:x:1012:1000:Linux User,,,:/home/user:,,,:/home/
/ # grep another /etc/passwd
another-user:x:1013:1000:Linux User,,,:/home/another:/sbin/halt
```
Comment 2 Dennis Schridde 2017-01-25 19:38:56 UTC
I can still confirm this in Alpine Linux 3.5 with busybox v1.25.1:
```
$ docker run -ti alpine:3.5 /bin/sh
/ # busybox | head -n1
BusyBox v1.25.1 (2016-10-26 16:15:20 GMT) multi-call binary.
/ # addgroup -g 1000 xgroup
/ # adduser -u 1012 -G xgroup -h /home/user -D xuser
/ # grep xuser /etc/passwd
xuser:x:1012:1000:Linux User,,,:/home/user:,,,:/home/
/ # grep another /etc/passwd
another-user:x:1013:1000:Linux User,,,:/home/another:/sbin/halt
```
Comment 3 Denys Vlasenko 2017-01-26 17:53:35 UTC
adduser.c:

pw.pw_shell = (char *)get_shell_name();

get_shell_name(void)
{
        struct passwd *pw;
        char *shell;
        shell = getenv("SHELL");
        if (shell && shell[0])
                return shell;
        pw = getpwuid(getuid());
        if (pw && pw->pw_shell && pw->pw_shell[0])
                return pw->pw_shell;
        return DEFAULT_SHELL;
}

DEFAULT_SHELL is "/bin/sh"

What you see can be explained by $SHELL or current user's shell being set to a weird value.
Comment 4 Dennis Schridde 2017-01-26 21:20:51 UTC
(In reply to Denys Vlasenko from comment #3)
`$SHELL` is not set at all:
```
# docker run -ti alpine:3.5 /bin/sh
/ # env
HOSTNAME=59df3c3f58de
SHLVL=1
HOME=/root
TERM=xterm
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/
```
Comment 5 Dennis Schridde 2017-01-26 21:29:25 UTC
Interestingly, I cannot reproduce this with the busybox:1.25.1-musl container image:
```
$ docker run -ti busybox:1.25.1-musl /bin/sh
/ # busybox | head -n1
BusyBox v1.25.1 (2016-10-07 18:16:05 UTC) multi-call binary.
/ # addgroup -g 1000 xgroup
/ # adduser -u 1012 -G xgroup -h /home/user -D xuser
/ # grep xuser /etc/passwd
xuser:x:1012:1000:Linux User,,,:/home/user:/bin/sh
/ # adduser -u 1013 -G xgroup -h /home/another -D another-user
/ # grep another /etc/passwd
another-user:x:1013:1000:Linux User,,,:/home/another:/bin/sh
```
Comment 6 Denys Vlasenko 2017-01-29 18:04:50 UTC
(In reply to Dennis Schridde from comment #4)
> `$SHELL` is not set at all:

I did not say that $SHELL is set.
Look at the code in comment #3.
If $SHELL is not set, what does it do?
It uses current user's shell (if pwd database is accessible and if shell is not empty).
I guess that's what is being used in your case.
Comment 7 Dennis Schridde 2017-01-30 19:58:57 UTC
(In reply to Denys Vlasenko from comment #6)
Thanks for clarifying this. /etc/passwd appears to be correct, though.

Also I cannot reproduce this in the official Busybox Docker image, but only with the version used in Alpine Linux. Thus I think this bug can be closed now, unless Jonathon Reinhart or someone else can still reproduce it.

The downstream bugreport for Alpine Linux lives at: https://bugs.alpinelinux.org/issues/5083
Comment 8 Jonathon Reinhart 2017-01-31 01:56:49 UTC
My original inquiry was "What was broken, and when/where did it get fixed?" I was already aware that it appeared to be fixed by v1.24.1. Considering we still don't know what exactly was broken, and that https://bugs.alpinelinux.org/issues/5083 is still without a real cause, I'm inclined to leave this open until we've figured it out.
Comment 9 5cli67sg4k 2019-01-11 23:09:24 UTC
Created attachment 7926 [details]
Patch to fix this issue

This is a bug caused by the fact that getpwnam(3) returns a pointer to static data and is used twice in adduser.c

1. Indirectly by get_shell_name()
2. Directly by passwd_study()

This causes the getpwnam(3) call in passwd_study() to "overwrite" the memory location `get_shell_name()` returns a pointer two. There are obviously a variety of ways to address this issue (e.g. making get_shell_name() use getpwnam_r). However, the most simple fix (which also seems to be used elsewhere) is using xstrdup() on the return value of get_shell_name().
Comment 10 Denys Vlasenko 2019-01-12 10:53:51 UTC
Fixed in git, thanks!