linux-kernel - Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

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 for Android: free password hash cracker in your pocket

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

Date:	Tue, 28 Apr 2015 09:42:38 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Al Viro <viro@...iv.linux.org.uk>
Cc:	Fabian Frederick <fabf@...net.be>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

On Tue, Apr 28, 2015 at 9:05 AM, Al Viro <viro@...iv.linux.org.uk> wrote:
>
> Unfortunately, we _can't_ make strlcpy() never look past src + size - 1 -
> not without changing its semantics.

Yeah, strlcpy is actually nasty. I don't understand why people like it
so much. It's a crap interface, and completely unsuitable for
untrusted sources because of the overrun issue.

Now, strncpy is nasty too, because of the two main flaws:
 - no guaranteed NUL at the end
 - crazy "fill with NUL" at the end

the first of which causes security issues, and the second of which
causes performance issues when you have small strings and size your
buffers generously.

Generally, what we want is "strncpy()" that forces a _single_ NUL at
the end. Basically, what we want is that

     good_strncpy(dst, src, n);
     assert(strlen(dst) < n);

always just works, but it doesn't try to pad the 'dst' to zero.

Alternatively, the return value should be really obvious and
unambiguous. That's what our "strncpy_from_user()" does: it is a but
more complex because it needs to show three cases (ok, too long, and
EFAULT), so the semantics for 'strncpy_from_user() is that you have to
just always check the return value, but at least it's simple:

 - negative means error
 - >= n means "too long"
 - 0..n-1 means "ok" and is the size of the string.

for a normal in-kernel strncpy(), we'd likely be better off just
returing "we truncated" as an error.

Then you could just do

    mystrncpy(dst, src, sizeof(dst));

and the result would be a valid string (possibly truncated), and if
you care about truncation you just do

   if (good_strncpy(dst, src, sizeof(dst)) < 0)
       return -ETOOLONG;

both of which are just very *obvious* things, and neither of which
leans a possibly unsafe string in "dst", nor look past the end of
'src'.

Oh well.

I can certainly imagine other more complex forms too (max length of
destination _and_ separately max length of source). But strncpy and
strlcpy are both horrible nasty error-prone crap.

                               Linus
--
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/