Messages in this thread | | | Date | Tue, 28 Apr 2015 07:35:10 +0200 (CEST) | From | Fabian Frederick <> | Subject | Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy" |
| |
> On 28 April 2015 at 05:48 Al Viro <viro@ZenIV.linux.org.uk> wrote: > > > commit 39d7a29f867bd5a4a551fad6bb3812ceddb0bce1 > Author: Fabian Frederick <fabf@skynet.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
| |