| 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
| ||
|
Message-ID: <1757511.6Qb6Y897ce@machine>
Date: Sat, 24 Oct 2020 12:36:46 +0200
From: Francis Laniel <laniel_francis@...vacyrequired.com>
To: Kees Cook <keescook@...omium.org>
Cc: linux-hardening@...r.kernel.org, dja@...ens.net
Subject: Re: [RFC][PATCH v3 3/5] Fortify string function strscpy.
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@...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()
>
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@...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
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) {
Powered by blists - more mailing lists