Date: Sat, 18 Apr 2015 11:25:43 -0400 From: Rich Felker <dalias@...c.org> To: musl@...ts.openwall.com Cc: Matt Johnston <matt@....asn.au> Subject: Re: Re: Security advisory for musl libc - stack-based buffer overflow in ipv6 literal parsing [CVE-2015-1817] On Sat, Apr 18, 2015 at 09:32:02PM +0800, Matt Johnston wrote: > > On Fri, Apr 17, 2015 at 02:03:25PM -0400, Rich Felker wrote: > > > On Fri, Apr 17, 2015 at 01:23:27PM -0400, Rich Felker wrote: > > > > And wow, this is an utter mess. Not only does dropbear fail to drop > > > > root before processing forwards; it NEVER drops root at all. The > [snip] > > > > Is there any reason for not performing the setgroups/setgid/setuid > > > > immediately after authentication succeeds? Have you looked at whether > > > > it would be easy to patch that in? > > > > > > Simply copying/moving the code from svr-chansession.c's execchild to > > > svr-auth.c's send_msg_userauth_success seems to fix the entire issue. > > > I had to disable the (useless on systems with proper pty support) > > > chown/chmod for the pty, but otherwise it seems to be working fine. > > > > I guess changes like these should be upstream'ed. Matt? > > Hi Rich, > > Thanks for the suggestion. When the code was originally > written quite a few systems had to use older pty methods > though that has improved now, it is worth revisiting. > Some systems still require --disable-openpty (old uClibc or > built with strange configurations, I think). The "clean" (albeit ugly if you don't like suids) solution to this problem on legacy systems was to have a setuid pt_chown binary to implement the grantpt function. No idea if the systems you care about have such a utility. Short of that you could fork a second time prior to dropping privs and have the new forked child do nothing but listen on a pipe for pty-control commands. Of course this is problematic for NOMMU, and it's also wasteful if the process has to wait around to cleanup the pty at exit rather than just doing its thing and exiting at startup. > utmp/wtmp logs require privileges to write, Dropbear could > add utmp group on systems using that (not sure how > widespread it is). Yes, I neglected to mention that (and didn't even notice it at first) because I don't use utmp and musl nops it out. There are various solutions to the problem with external utmp utilities/daemons but I'm not really familiar with them. In any case the current code does not error out when utmp update fails; it just silently fails to produce an entry. So a "drop privs early" build or runtime option would be practical without any change to that code as long as users can live with no utmp. > The server hostkey will remain in process > memory since it's required for rekeying - not as bad as root > code execution though. Ugly. I don't see how this can be solved without a more advanced privsep model. I agree it's lower-severity though. Based on this: http://www.snailbook.com/faq/no-rekeying.auto.html rekeying is unreliable since the client or server may fail to support it. I have no idea if that information is still at all relevant, but if rekeying is not widely in use it might be worthwhile to provide an option for paranoid users to disable it and clear the key in the child. (Of course there are lots of obstacles to effectively eliminating all traces of it.) > Proper separation with a privileged supervisor is > on my todo list, I'll have a look how feasible a simpler > approach is. Part of the approach I would recommend is having a separate executable for the session process and invoking it via posix_spawn if possible. This makes it possible to run on NOMMU without having to rely on vfork hacks which almost certainly introduce vulnerabilities via program state corruption. If you're interested in discussing design ideas for it I'd be happy to. > > > musl's network-facing DNS code seems a bit precarious with > > > pointer arithmetic? > > > > Which code are you talking about? There was a previous > > problem with > > dn_expand, which is the main function that comes to mind for > > me, and > > the code was fixed and heavily reviewed at the time. > > Yes, dn_expand was what I was thinking of - at the time I > looked at the fix and it seemed like difficult code to > reason about. Overall the code style of musl seems quite > nice, I guess the parsing type code is just fairly terse. > The masking with ip[i&7]=0 in the recent fix was a bit > non-obvious without looking at the commit message - makes > sense as a safety mechanism. Yes, a lot of the justifications for various things are in the git log rather than comments. I've found this produces a more useful narrative of the motivations for changes, but there are places where we could use more comments, and gradually I'm trying to add them where I find people are getting confused. The ip[i&7] trick is a place that could use one, I think. Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.