[PATCH] wall: Don't allow reading of files the real user might not have access to

[Ryan Mallon]

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 <rmallon at gmail.com>
>> ---
>>
>> 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