Re: Re: Security advisory for musl libc - stack-based buffer overflow in ipv6 literal parsing [CVE-2015-1817]

[Matt Johnston <matt@ucc.asn.au>]

> 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