netdev - 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: <20140401110255.GH24150@order.stressinduktion.org>
Date:	Tue, 1 Apr 2014 13:02:55 +0200
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Vegard Nossum <vegard.nossum@...cle.com>
Cc:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Dan Carpenter <dan.carpenter@...cle.com>,
	"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 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.

> 2. Even if we changed strlcpy() to use strnlen(), it would still be 
> unsafe if the source string is not 0-terminated and the source buffer is 
> shorter than the destination buffer. That's because the size passed to 
> strlcpy() is conceptually the length of the _destination_ buffer, not 
> the source string.

Ack.

> I'm not against changing strlcpy() per se (changing to strnlen() might 
> be a performance improvement), but we shouldn't use that as an excuse to 
> use the interface incorrectly.

I am totally with you there.

Actually in some cases it could hinder finding such bugs as we're more
unlikely to hit a RED_ZONE which should crash the kernel (I actually
think crashes to find such bugs are good). But I guess the propability
is pretty high to hit another NUL byte before that and if at that point a
RED_ZONE is mapped.

Thanks,

  Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html