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]

Message-ID: <553FFDE9.9070700@ezchip.com>
Date:	Tue, 28 Apr 2015 17:38:49 -0400
From:	Chris Metcalf <cmetcalf@...hip.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
CC:	Al Viro <viro@...iv.linux.org.uk>,
	Fabian Frederick <fabf@...net.be>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Randy Dunlap <rdunlap@...radead.org>,
	Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
Subject: Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy"

On 04/28/2015 04:51 PM, Linus Torvalds wrote:
> On Tue, Apr 28, 2015 at 12:48 PM, Chris Metcalf <cmetcalf@...hip.com> wrote:
>> FWIW, I wanted to deal with some strncpy/strlcpy API issues last year
>> and just put a "strscpy()" function in arch/tile/gxio/mpipe.c,
> So quite frankly, I don't like that one either.
>
> Some people really *do* want truncation, and your strscpy() makes that
> impossible.

Perhaps the answer is to provide a strscpy() and a separate
strscpy_truncate() that explicitly enables truncation semantics.
I remain skeptical of providing a handy function with an error
return that people are free to ignore.  If one wants truncating
semantics, one should be obliged to say so.

> Also, your strscpy() implementation is actually not thread-safe: it
> can return an non-terminated string if the source string isn't stable.
> That can certainly be a design issue ("don't do that then"), but it
> *can* be a possible source of security issues, so it's a bad idea in
> something that is supposed to be secure.

To do that we'd probably want to provide a generic version that
just copied byte-by-byte, and encourage architecture variants
that were more efficient.  This would leave it less efficient in
general than strncpy/strlcpy (the former typically has efficient
arch versions already, and the latter, although not thread-safe
by your definition, is built on strlen/memcpy, which typically
have efficient arch versions).

I don't see a way to leverage existing efficient implementations,
since only strncpy likely has pre-existing efficient implementations,
and then we'd have to call strlen() on the destination just to
return the total length (after already calling strnlen() on the
source to figure out how long we thought it was).

I suppose if we want to just return a boolean saying "it fit" or
"it didn't fit" we could leverage strncpy, though if the buffer
changed out from under us to be just a single NUL instead of
a long string, we'd still do the silly NUL-fill behavior.  So maybe
it's not worth the contortions.

> And quite frankly, I think that the *only* valid reason to add another
> random string copy function is that you actually get it right. We
> don't need yet another half-arsed routine that can be easily misused.
> We have too many of those.

For sure.  Something like this untested code in lib/string.c, to be 
concrete?

#ifndef __HAVE_ARCH_STRSCPY
/**
  * strscpy_truncate - Copy a C-string into a sized buffer and return whether it fit
  * @dest: Where to copy the string to
  * @src: Where to copy the string from
  * @count: Size of destination buffer
  *
  * Copy the string, or as much of it as fits, into the dest buffer.
  * The routine returns the total number of bytes copied
  * (including the trailing NUL) or zero if the buffer wasn't
  * big enough.  The dest buffer is always NUL-terminated.
  */
static size_t strscpy_truncate(char *dest, const char *src, size_t count)
{
	char *tmp = dest;

	if (count == 0)
		return 0;  /* no NUL-termination possible */

	while ((*tmp++ = *src++) != '\0') {
		if (--count == 0) {
			*--tmp = '\0';
			return 0;
		}
	}

	return tmp - dest;
}
EXPORT_SYMBOL(strscpy_truncate);

/**
  * strscpy - Copy a C-string into a sized buffer, but only if it fits
  * @dest: Where to copy the string to
  * @src: Where to copy the string from
  * @count: Size of destination buffer
  *
  * Use this routine to avoid copying too-long strings.
  * The routine returns the total number of bytes copied
  * (including the trailing NUL) or zero if the buffer wasn't
  * big enough.  To ensure that programmers pay attention
  * to the return code, the destination has a single NUL
  * written at the front (if count is non-zero) when the
  * buffer is not big enough.
  */
static size_t strscpy(char *dest, const char *src, size_t count)
{
	size_t res = strscpy_truncate(dest, src, count);

	if (res == 0 && count != 0)
		dest[0] = '\0';
	return res;
}
EXPORT_SYMBOL(strscpy);
#endif

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

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