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: <202010232204.9DCF5501DA@keescook> Date: Fri, 23 Oct 2020 22:23:01 -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 4/5] Add new file in LKDTM to test fortified strscpy. On Wed, Oct 21, 2020 at 05:06:07PM +0200, laniel_francis@...vacyrequired.com wrote: > From: Francis Laniel <laniel_francis@...vacyrequired.com> > > This new test generates a crash at runtime because there is a write overflow in > destination string. > > Signed-off-by: Francis Laniel <laniel_francis@...vacyrequired.com> > --- > drivers/misc/lkdtm/Makefile | 1 + > drivers/misc/lkdtm/core.c | 1 + > drivers/misc/lkdtm/fortify.c | 47 +++++++++++++++++++++++++ > drivers/misc/lkdtm/lkdtm.h | 3 ++ > tools/testing/selftests/lkdtm/tests.txt | 1 + > 5 files changed, 53 insertions(+) > create mode 100644 drivers/misc/lkdtm/fortify.c > > diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile > index c70b3822013f..d898f7b22045 100644 > --- a/drivers/misc/lkdtm/Makefile > +++ b/drivers/misc/lkdtm/Makefile > @@ -10,6 +10,7 @@ lkdtm-$(CONFIG_LKDTM) += rodata_objcopy.o > lkdtm-$(CONFIG_LKDTM) += usercopy.o > lkdtm-$(CONFIG_LKDTM) += stackleak.o > lkdtm-$(CONFIG_LKDTM) += cfi.o > +lkdtm-$(CONFIG_LKDTM) += fortify.o > > KASAN_SANITIZE_stackleak.o := n > KCOV_INSTRUMENT_rodata.o := n > diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c > index a002f39a5964..4326e2d09870 100644 > --- a/drivers/misc/lkdtm/core.c > +++ b/drivers/misc/lkdtm/core.c > @@ -180,6 +180,7 @@ static const struct crashtype crashtypes[] = { > #ifdef CONFIG_X86_32 > CRASHTYPE(DOUBLE_FAULT), > #endif > + CRASHTYPE(FORTIFIED_STRSCPY), > }; > > > diff --git a/drivers/misc/lkdtm/fortify.c b/drivers/misc/lkdtm/fortify.c > new file mode 100644 > index 000000000000..cecdfbb8ba55 > --- /dev/null > +++ b/drivers/misc/lkdtm/fortify.c > @@ -0,0 +1,47 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2020 Francis Laniel <laniel_francis@...vacyrequired.com> > + * > + * Add tests related to fortified functions in this file. > + */ > +#include <linux/string.h> > +#include <linux/slab.h> > +#include "lkdtm.h" > + > + > +/* > + * Calls fortified strscpy to test that it returns the same result as vanilla > + * strscpy and generate a panic because there is a write overflow (i.e. src > + * length is greater than dst length). > + */ > +void lkdtm_FORTIFIED_STRSCPY(void) > +{ > +#if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE) I would drop the #if: just let it run and freak out on non-fortified kernels. > + char *src; > + char dst[3]; > + > + src = kstrdup("foobar", GFP_KERNEL); > + > + if (src == NULL) > + return; > + > + /* Vanilla strscpy returns -E2BIG if size is 0. */ > + WARN_ON(strscpy(dst, src, 0) != -E2BIG); For LKDTM, we have different reporting that "normal", in the sense that usually the WARN/BUG outcomes are _desirable_ (i.e. "freak out on stack overflow"). So, I would write this as: if (strscpy(dst, src, 0) != -E2BIG) pr_warn("FAIL: strscpy() of 0 length did not return -E2BIG\n"); > + > + /* Vanilla strscpy returns -E2BIG if src is truncated. */ > + WARN_ON(strscpy(dst, src, sizeof(dst)) != -E2BIG); Same. > + > + /* After above call, dst must contain "fo" because src was truncated. */ > + WARN_ON(strncmp(dst, "fo", sizeof(dst)) != 0); Same. > + > + /* > + * Use strlen here so size cannot be known at compile time and there is > + * a runtime overflow. > + */ > + strscpy(dst, src, strlen(src)); I think we'll need a couple more corner cases, and any that need to Oops separately can be separate functions. Here's a corner case to test to strnlen(): struct { union { char big[10]; char src[5]; }; } weird = { .big = "hello!" }; char dst[sizeof(weird.src) + 1]; strscpy(dst, weird.src, sizeof(dst)); if (strcmp(dst, "hello") != 0) pr_warn("FAIL ... But each of the cases being tested in the fortified strscpy() should be exercised. > + > + pr_info("Fail: No overflow in above strscpy call!\n"); pr_warn("FAIL: ... > + > + kfree(src); > +#endif > +} > diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h > index 70c8b7c9460f..29c12dcdeab1 100644 > --- a/drivers/misc/lkdtm/lkdtm.h > +++ b/drivers/misc/lkdtm/lkdtm.h > @@ -106,4 +106,7 @@ void lkdtm_STACKLEAK_ERASING(void); > /* cfi.c */ > void lkdtm_CFI_FORWARD_PROTO(void); > > +/* fortify.c */ > +void lkdtm_FORTIFIED_STRSCPY(void); > + > #endif > diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt > index 9d266e79c6a2..4234109579eb 100644 > --- a/tools/testing/selftests/lkdtm/tests.txt > +++ b/tools/testing/selftests/lkdtm/tests.txt > @@ -70,3 +70,4 @@ USERCOPY_KERNEL > USERCOPY_KERNEL_DS > STACKLEAK_ERASING OK: the rest of the thread stack is properly erased > CFI_FORWARD_PROTO > +FORTIFIED_STRSCPY > \ No newline at end of file > -- > 2.20.1 > -- Kees Cook
Powered by blists - more mailing lists