|
Message-ID: <20150418133202.GG17615@ucc.gu.uwa.edu.au> Date: Sat, 18 Apr 2015 21:32:02 +0800 From: Matt Johnston <matt@....asn.au> To: musl@...ts.openwall.com Cc: Rich Felker <dalias@...c.org> Subject: Re: Re: Security advisory for musl libc - stack-based buffer overflow in ipv6 literal parsing [CVE-2015-1817] > 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). utmp/wtmp logs require privileges to write, Dropbear could add utmp group on systems using that (not sure how widespread it is). The server hostkey will remain in process memory since it's required for rekeying - not as bad as root code execution though. Proper separation with a privileged supervisor is on my todo list, I'll have a look how feasible a simpler approach is. > > 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. Cheers, Matt
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.