| 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
| ||
|
Date: Sat, 24 Oct 2020 12:19:44 +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 4/5] Add new file in LKDTM to test fortified strscpy.
Le samedi 24 octobre 2020, 07:23:01 CEST Kees Cook a écrit :
> 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.
I will drop it for v4.
> > + 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");
I was not sure if using WARN_* lile macro was a good idea in LKDTM since if I
understood correctly the goal of a LKDTM test is to fail.
I will rewrite it the way you suggested it for the v4.
> > +
> > + /* 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.
I will try to add these cases and other tests, if I think to more, for the
next version so the tests cover as most case as possible.
> > +
> > + pr_info("Fail: No overflow in above strscpy call!\n");
>
> pr_warn("FAIL: ...
>
Will be modified for next release!
> > +
> > + 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
Powered by blists - more mailing lists