linux-kernel - Re: [PATCH] isdnloop: NUL-terminate strings from userspace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140401123534.GE18506@mwanda>
Date:	Tue, 1 Apr 2014 15:35:34 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Vegard Nossum <vegard.nossum@...cle.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>, stable@...r.kernel.org
Subject: Re: [PATCH] isdnloop: NUL-terminate strings from userspace

On Tue, Apr 01, 2014 at 01:02:55PM +0200, Hannes Frederic Sowa wrote:
> On Tue, Apr 01, 2014 at 12:46:37PM +0200, Vegard Nossum wrote:
> > On 04/01/2014 12:30 PM, Hannes Frederic Sowa wrote:
> > >Looking down the problem, it seems the problem is that the strlen in 
> > >strlcpy
> > >could read beyond the input buffer?
> > >
> > >To prevent this problem in other parts of the kernel wouldn't it be better 
> > >to
> > >replace the strlen with strnlen in strlcpy?
> > 
> > Sorry, I should have included the link to the previous thread: 
> > https://lkml.org/lkml/2014/3/7/712
> > 
> > I only resent (adding netdev to Cc) to get this into David Miller's 
> > patch queue.
> 
> Ah ok, sorry I don't follow lkml as closely as netdev@.
> 
> > As you can see from the previous discussion, we _could_ change the Linux 
> > kernel's definition of strlcpy(), but I wouldn't recommend it for the 
> > following reasons:
> > 
> > 1. Both BSD man page and BSD implementation _require_ the source string 
> > to be 0-terminated. Changing the semantics of strlcpy() in the Linux 
> > kernel would probably be a bad idea and cause even more confusion that 
> > what we already have.
> 
> Sure, we shouldn't change the documented semantics. If at all it would
> be an additional safety net. Your patch would still be needed.
> 

Guys, really?  How would the patch "still be needed"?  I feel like if
someone said we had to rub a chicken head on this code we do it in the
name of security...

I don't understand what you think the point of strlcpy() is, if it's not
to deal with source strings which aren't NUL terminated.

I still maintain that the since the stack is full of NUL characters the
current implimentation of strlcpy() is ok for this isdn_loop function
and the patch is not needed at all without the strnlen() change.

However for other heap allocated variables then I could imagine that
the strlen() might be a problem.  I have two theories why we have never
seen problems with this in running code. 1) The string would have to be
at the end of a struct allocated at the end of a page.  You have to be
very unlucky to hit this requirement.  2) Most people pass valid data.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/