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