Date: Tue, 19 Jul 2016 08:32:38 -0500 From: ebiederm@...ssion.com (Eric W. Biederman) To: Sebastian Krahmer <krahmer@...e.com> Cc: oss-security@...ts.openwall.com, pkg-shadow-devel@...ts.alioth.debian.org, "Serge E. Hallyn" <serge@...lyn.com> Subject: Re: subuid security patches for shadow package Adding the shadow-development list, so there is a chance other people familiar with the code can comment as well. Sebastian Krahmer <krahmer@...e.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
Powered by blists - more mailing lists
Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.
Powered by Openwall GNU/*/Linux - Powered by OpenVZ