[Pkg-shadow-devel] subuid security patches for shadow package

[Eric W. Biederman]

Adding the shadow-development list, so there is a chance other people
familiar with the code can comment as well.

Sebastian Krahmer <krahmer at suse.com> writes:

> On Tue, Jul 19, 2016 at 11:39:15AM +0200, Sebastian Krahmer wrote:
>> Hi
>> 
>> The shadow package contains newuidmap and newgidmap suid
>> binaries in order to allow users to take advantage of the
>> userns feature of uid-mappings.
>> 
>> I added patches here:
>> 
>> https://bugzilla.suse.com/show_bug.cgi?id=979282
>> 
>> they consist of:
>> 
>> 1) Removing getlogin() to find out about users.
>>    It relies on utmp, which is not a trusted base of info (group writable).
>> 
>> 2) Cleaning up UID retrieval and computation. The 'long long' code was
>>    totally unclear to me, as the numbers are converted to ulong right
>>    afterwards anyway. Additionally there was a *int overflow*, which can be
>>    tested via 'newuidmap $$ 0 10000 -1' (given that 10000 is listed as allowed)
>>    which produces no error but tries to write large "count" values to the uid_map
>>    file. Kernel may check for overflows itself, but it should not be allowed
>>    by a suid binary to be written in the first place.
>
> After checking some kernels, it looks like this int wrap is exploitable as a LPE,
> as kernel is using 32bit uid's that are truncated from unsigned longs (64bit on x64)
> as returned by simple_strtoul() [map_write()]. So newuidmap and kernel have an entire
> different view on the upper and lower bounds, making newuidmap overflow (and pass)
> and still being in bounds inside the kernel.
>
> Maybe it would be wise to align integer widths of kernel and the userspace
> tools.
>
> So everyone shipping newuidmap as mode 04755 should fix it. :)

Thank you for the review and looking at this.  I agree that the integer
size issues should all be locked down and handled more clearly.

I think it should be code in have_sub_uids and have_sub_gids that should
be catching overflows and the like.  Limiting things to what is actually
allowed by the subuid file.

I also agree that the kernel is permitting more than it needs to which
in case like this is not helpful.

The issues with the library functions get_my_pwent and getulong I will
have to come up to speed on before I comment knowledgably, but they
definitely appear to be worth looking at.

Eric