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()); > + fd = xopen(argv[1], O_RDONLY); > + setfsuid(geteuid()); > + setfsgid(getegid()); > + }
That is fine in general, but has issues with files in /proc on Linux which use "%pK". See: http://marc.info/?l=linux-kernel&m=138049414321387&w=2 Doing: wall /proc/kallsyms Will allow you to dump the kernel symbol table addresses as a normal user even if kptr_restrict=1. I am trying to get the kernel fixed in this regard though, in which case your version of the patch is okay. I assume that xsetfsuid, etc variants should be added to make sure the calls are all successful? ~Ryan _______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
