On Tue, Oct 08, 2013 at 10:57:58AM +1100, Ryan Mallon wrote: > On 07/10/13 00:13, Denys Vlasenko wrote: > > On Monday 30 September 2013 00:30, Ryan Mallon wrote: > >> The wall applet is setuid and currently does no checking of the real > >> user's read access to the message file. This allows the wall applet to be > >> used to display files which are not readable by an unprivileged user. For > >> example: > >> > >> $ wall /etc/shadow > >> $ wall /proc/vmallocinfo > >> > >> Fix this issue by introducing the same check as used by the util-linux > >> version of wall: only allow the file argument to be used if the user is > >> root, or the real and effective uid/gids are equal. > >> > >> Note, some files in /proc, etc do have global read permission, but may > >> have different contents if read as root (for example Linux's kptr_restrict > >> feature). User's may still run: > >> > >> $ wall < file > >> > >> If they do legitimately have read access to some file. > >> > >> Signed-off-by: Ryan Mallon <[email protected]> > >> --- > >> > >> diff --git a/miscutils/wall.c b/miscutils/wall.c > >> index 762f53b..f0a6cd4 100644 > >> --- a/miscutils/wall.c > >> +++ b/miscutils/wall.c > >> @@ -23,6 +23,20 @@ int wall_main(int argc UNUSED_PARAM, char **argv) > >> struct utmp *ut; > >> char *msg; > >> int fd = argv[1] ? xopen(argv[1], O_RDONLY) : STDIN_FILENO; > >> + uid_t ruid; > >> + > >> + /* > >> + * If we are not root, but are setuid or setgid, then don't allow > >> + * reading of any files the real uid might not have access to. The > >> + * user can do 'wall < file' instead of 'wall file' if this disallows > >> + * access to some legimate file. > >> + */ > >> + ruid = getuid(); > >> + if (fd != STDIN_FILENO && ruid != 0 && > >> + (ruid != geteuid() || getgid() != getegid())) { > >> + bb_error_msg_and_die("will not read %s - use stdin or '%s < > >> %s'", > >> + argv[1], argv[0], argv[1]); > >> + } > >> > >> msg = xmalloc_read(fd, NULL); > >> if (ENABLE_FEATURE_CLEAN_UP && argv[1]) > > > > > > How about the following patch? - > > > > > > --- busybox.3/miscutils/wall.c 2013-07-25 03:48:31.000000000 +0200 > > +++ busybox.4/miscutils/wall.c 2013-10-06 15:01:55.000000000 +0200 > > @@ -22,8 +34,19 @@ int wall_main(int argc UNUSED_PARAM, cha > > { > > struct utmp *ut; > > char *msg; > > - int fd = argv[1] ? xopen(argv[1], O_RDONLY) : STDIN_FILENO; > > + int fd; > > > > + fd = STDIN_FILENO; > > + if (argv[1]) { > > + /* The applet is setuid. > > + * Access to the file must be under user's uid/gid. > > + */ > > + setfsuid(getuid()); > > + setfsgid(getgid()); > > Also, the setfsuid()/setfsgid() functions are Linux specific and don't > appear to be able to return errors to the caller properly. The manpage > suggests that they are no longer necessary since the signalling issues > they were introduced for in Linux no longer exist. I think it is > safe/portable to set setegid()/seteuid().
They are still necessary, but for a different reason: euid/egid apply to the whole process, whereas fsuid/fsgid are thread-local. So in a multithreaded server attempting to access files while enforcing the permissions of a particular user, you may still want fsuid/fsgid. In the case of busybox, I think euid/egid are ok. Rich _______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
