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

[Rich Felker <dalias@libc.org>]

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