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

[Francis Laniel <laniel_francis@privacyrequired.com>]

Le samedi 24 octobre 2020, 07:04:04 CEST Kees Cook a écrit :
> On Wed, Oct 21, 2020 at 05:06:06PM +0200, laniel_francis@privacyrequired.com 
wrote:
> > From: Francis Laniel <laniel_francis@privacyrequired.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()
> 

I will expand the commit message for the next version and adapt the title to 
follow the history.

> > Signed-off-by: Francis Laniel <laniel_francis@privacyrequired.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

I will remove it!

> 
> > +#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. */
> 

Your comment is clearer than mine, I will add it for the next release.

> > +	 * 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);
> 

I thought I misunderstood one of your passed message because I understood that 
I should remove this snippet.
I will readd it and sorry for the misunderstanding.

> > +	/*
> > +	 * 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.)

Nice catch! I will remove this branch then.

> 
> > +
> > +	/* 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.
> 

I am not really sure about this but I need to think more about it after coding 
a new version and tested it.

> > +	/*
> > +	 * 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.

I had trouble when I used len as argument in previous version of this patch 
set.
But you are right we totally need to use len, moreover it avoids having 
write_overflow.
I will then switch to len and test the code with LKDTM and gdb.

> 
> > +	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.
> 

Modifying the last argument of __real_strscpy() can modify what it returns so 
fortified strscpy will not return the same as __real_strscpy().
I will stick for the behavior of __real__strscpy() for next release.
Maybe I will first write the test then the fortified function so I am sure there 
is no problem with the new version.

Thank you for the complete review though!

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