linux-hardening - Re: [RFC][PATCH v3 3/5] Fortify string function strscpy.

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: <202010232017.14FB7034@keescook>
Date:   Fri, 23 Oct 2020 22:04:04 -0700
From:   Kees Cook <keescook@...omium.org>
To:     laniel_francis@...vacyrequired.com
Cc:     linux-hardening@...r.kernel.org, dja@...ens.net
Subject: Re: [RFC][PATCH v3 3/5] Fortify string function strscpy.

On Wed, Oct 21, 2020 at 05:06:06PM +0200, laniel_francis@...vacyrequired.com wrote:
> From: Francis Laniel <laniel_francis@...vacyrequired.com>
> 
> Fortified strscpy detects write overflows to dest.

This commit log needs to be expanded to explain the "why" of the change
with some detail.

Also, please look at the git commit history of include/linux/string.h
for a sense of the kinds of Subject prefixes being used. I think this
Subject should be something like:

	Subject: string.h: Add FORTIFY coverage for strscpy()

> 
> Signed-off-by: Francis Laniel <laniel_francis@...vacyrequired.com>
> ---
>  include/linux/string.h | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 46e91d684c47..add7426ff718 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -6,6 +6,8 @@
>  #include <linux/compiler.h>	/* for inline */
>  #include <linux/types.h>	/* for size_t */
>  #include <linux/stddef.h>	/* for NULL */
> +#include <linux/bug.h>		/* for WARN_ON_ONCE */

^^^ no longer needed

> +#include <linux/errno.h>	/* for E2BIG */
>  #include <stdarg.h>
>  #include <uapi/linux/string.h>
>  
> @@ -357,6 +359,42 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
>  	return ret;
>  }
>  
> +/* defined after fortified strnlen to reuse it */
> +extern ssize_t __real_strscpy(char *, const char *, size_t) __RENAME(strscpy);
> +__FORTIFY_INLINE ssize_t strscpy(char *p, const char *q, size_t size)
> +{
> +	size_t len;
> +	/*
> +	 * Use 1 as second argument to guess only p size even and not the

This isn't a "guess". Perhaps just say:

/* Use string size rather than possible enclosing struct size. */

> +	 * surrounding struct size (in case it is embedded inside a struct).
> +	 */
> +	size_t p_size = __builtin_object_size(p, 1);

A short-circuit path should be added here:

	size_t q_size = __builtin_object_size(q, 1);
        if (p_size == (size_t)-1 && q_size == (size_t)-1)
                return __real_strscpy(p, q, size);

> +	/*
> +	 * If size can be known at compile time and is greater than
> +	 * p_size, generate a compile time write overflow error.
> +	 */
> +	if (__builtin_constant_p(size) && size > p_size)
> +		__write_overflow();

Compile-time size > p_size is caught here.

Compile-time over-read on strings isn't catchable.

> +
> +	len = strnlen(q, size);

Runtime over-read is caught here.

> +	/*
> +	 * strscpy handles read overflows by stop reading q when '\0' is
> +	 * met.
> +	 * We stick to this behavior here.
> +	 */
> +	len = (len >= size) ? size : len;

This test isn't needed: strnlen(q, size) will never return larger than
size. (i.e. len has already been reduced to size.)

> +
> +	/* Otherwise generate a runtime write overflow error. */
> +	if (len > p_size)
> +		fortify_panic(__func__);

Runtime over-write is caught here, though I think this needs to be:

	if (len + 1 > p_size)
		fortify_panic(__func__);

since we need to fit "len" plus NUL in p_size. len + 1 == p_size is ok.

> +	/*
> +	 * Still use size as third argument to correctly compute max
> +	 * inside strscpy.
> +	 */

If we do this, then we run the risk of doing the over-read anyway. For
this call, we need to use len in most cases.

> +	return __real_strscpy(p, q, size);

In the unfortified case, -E2BIG will happen when strlen(q) >= size. When
fortified, strnlen is capped at min(size, q_size), so -E2BIG could only
happen when len == size. If len < size, there will never be -E2BIG.

So I think this means we need the __real_strscpy() call to look like this:

	// All safe from over-read  (len + 1 <= min(size, q_size): strnlen() above).
	// All safe from over-write (len + 1 <= p_size: explicit test above).
	if (len < size)
		return __real_strscpy(p, q, len + 1); // not E2BIG
	else if (len + 1 == size)
		return __real_strscpy(p, q, len + 1); // not E2BIG
	else if (len == size) // therefore size < min(size, q_size, p_size)
		return __real_strscpy(p, q, size);    // E2BIG
	else //  len + 1 > size
		return __real_strscpy(p, q, len + 1); // E2BIG

or simply:

	return __real_strscpy(p, q, len == size ? size : len + 1);

But the complexity here clearly calls for test cases.

> +}
> +
>  /* defined after fortified strlen and strnlen to reuse them */
>  __FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
>  {
> -- 
> 2.20.1
> 

-- 
Kees Cook