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: <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
Powered by blists - more mailing lists