[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] libs/light: make it build without setresuid()
On Wed, Jan 20, 2021 at 03:32:29PM +0000, Ian Jackson wrote: > Manuel Bouyer writes ("Re: [PATCH] libs/light: make it build without > setresuid()"): > > On Wed, Jan 20, 2021 at 02:52:06PM +0000, Ian Jackson wrote: > > > I don't think setuid is safe - at least, if we are trying to restrict > > > the dm. Since I think after the libxl child is forked, and has called > > > > What is the dm in this case ? qemu ? On NetBSD qemu runs as root AFAIK, > > so there isn't much to protect. > > Yes, the dm is qemu. If qemu restriction is not supported, that makes > a big difference. The complex situation here is to do with trying to > kill a possibly hostile qemu. Hum, I'll have to check this (how to check, BTW ?). I assumed qemu was running as root but it may not be completely true. Especially as I notice, now that I'm re-reading the patch, that we're doing a kill to -1. If we were doing so as root, user processes would be killed. > > > > setuid, it might be traceable (by NetBSD's equivalent of ptrace) by > > > the dm. The dm could puppet it into pretending it had succeeded, but > > > then hang around until the domid is reused. > > > > I don't understand. We're talking about a simple kill(2) syscall here. > > If we're not trying to restrict qemu's privilege at all, then I think > the setuid is fine. > There are then only two remaining concerns I have > with this patch: > > Firstly, we try to avoid #ifdefs like this. It tends to make the code > rather tangled, especially over time. Instead we prefer to move the > non-portable code into its own file, eg *_linux.c. > > Secondly, I think we should check that dm_restrict is not enabled. > I think an assert would do since I think we believe this is already > prevented elsewhere ? > > (One option for making this work would be to simply disable the > killing by uid on NetBSD. But I don't think that's a good answer > because killing by uid after eg setuid is more reliable even if it is > not 100% bulletproof. So switching to setuid or maybe setreuid is the > right answer.) This would have to be checked, but I don't think a non-root process can ptrace a process whose saved-user-id is root. Actually I think I could mimic the setresuid() with setreuid() and seteuid(). > > > OK so if I understand properly, you say Xen should not be used on NetBSD ? > > I'm sorry to have offended and discouraged you. That was not my > intention. My apologies for sending an off-putting message. For the > avoidance of any doubt, definitely don't think that. We should make > this work properly. > > Would you be willing to look into the two points I mention above and > send a revised version of the patch ? If you find the refactoring > awkward I or Roger can help. Actually I don't see how I could split this in a different file, without lot of duplicate code (even in just kill_device_model_uid_child(), we're talking of about 7 lines of code out of 75). So some guidance here would be welcome. -- Manuel Bouyer <bouyer@xxxxxxxxxxxxxxx> NetBSD: 26 ans d'experience feront toujours la difference --
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |