Date: Wed, 20 Jul 2016 23:48:52 +0200 From: Nicolas François <nicolas.francois@...traliens.net> To: "Eric W. Biederman" <ebiederm@...ssion.com> Cc: Sebastian Krahmer <krahmer@...e.com>, oss-security@...ts.openwall.com, pkg-shadow-devel@...ts.alioth.debian.org Subject: Re: [Pkg-shadow-devel] subuid security patches for shadow package Hi, The first point looks like a non issue to me. getlogin() is used to differentiate users with the same UID. The result of getlogin() is checked: if it returns a username that do not have the UID returned by getuid(), it will be ignored. Best Regards, -- Nekral On Tue, Jul 19, 2016 at 08:32:38AM -0500, Eric W. Biederman wrote: > > 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 > > _______________________________________________ > Pkg-shadow-devel mailing list > Pkg-shadow-devel@...ts.alioth.debian.org > http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pkg-shadow-devel
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