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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 28 Apr 2015 07:35:10 +0200 (CEST)
From:	Fabian Frederick <fabf@...net.be>
To:	Al Viro <viro@...IV.linux.org.uk>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"



> On 28 April 2015 at 05:48 Al Viro <viro@...IV.linux.org.uk> wrote:
>
>
> commit 39d7a29f867bd5a4a551fad6bb3812ceddb0bce1
> Author: Fabian Frederick <fabf@...net.be>
> Date:   Fri Jun 6 14:36:15 2014 -0700
>
>     fs/befs/linuxvfs.c: replace strncpy by strlcpy
>     
>     strncpy + end of string assignment replaced by strlcpy
>
> replaces perfectly safe code with undefined behaviour.  All in the name
> of "security hardening", presumably.  Folks, seeing the words "designed to be
> safer, more consistent, and less error prone replacement" in a manpage does
> *NOT* mean "OK, quit reading it - no need to go further, not even to the end
> of the paragraph".  Because in the end of that paragraph it says "This means
> that for strlcpy() src must be NUL-terminated".  And sure enough, our
> implementation relies on that - it starts with strlen().
>
> strncpy() is guaranteed not to look further than size.  strlcpy() is *NOT*.
> strncpy() on unvalidated source is safe, provided that you sanitize the copy;
> strlcpy() on anything like that is an invitation for nasal daemons.
>
> Sure, we can (and probably should) make strlcpy(dst, src, n) never access
> memory beyond src + n - 1, but this kind of cargo-culting is a Bad Thing(tm);
> mindless "security improvements" without so much as bothering to RTFM are
> asking for trouble.  And in userland code anything like that _can't_ be
> papered over afterwards - not unless you can patch every libc implementation
> out there.
>
> This particular code is completely pointless - if anything, it should've been
> memcpy() + nd_terminate_link()...
>
> Al, very unhappy about the prospect of looking through ~2000 calls of
> strlcpy()
> we have in the tree...

Sorry Al, I thought it was more secure.
I guess the 2 following patches should be reversed as well :

6cb103b6f45a
"fs/befs/btree.c: replace strncpy by strlcpy + coding style fixing"

69201bb11327
"fs/ocfs2/super.c: use OCFS2_MAX_VOL_LABEL_LEN and strlcpy"

Regards,
Fabian
--
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/