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: <1789319.oY4W0zIQjc@machine> Date: Wed, 21 Oct 2020 14:43:00 +0200 From: Francis Laniel <laniel_francis@...vacyrequired.com> To: Daniel Axtens <dja@...ens.net> Cc: Kees Cook <keescook@...omium.org>, linux-hardening@...r.kernel.org Subject: Re: [RFC][PATCH v2] Fortify string function strscpy. Le lundi 19 octobre 2020, 13:51:26 CEST Daniel Axtens a écrit : > Francis Laniel <laniel_francis@...vacyrequired.com> writes: > > Le samedi 17 octobre 2020, 01:16:36 CEST Kees Cook a écrit : > >> On Fri, Oct 16, 2020 at 02:38:09PM +0200, > >> laniel_francis@...vacyrequired.com> > > wrote: > >> > From: Francis Laniel <laniel_francis@...vacyrequired.com> > >> > > >> > Thanks to kees advices (see: > >> > https://github.com/KSPP/linux/issues/96#issuecomment-709620337) I wrote > >> > a > >> > LKDTM test for the fortified version of strscpy I added in the v1 of > >> > this > >> > patch. The test panics due to write overflow. > >> > >> Ah nice, thanks! I am reminded about this series as well: > >> https://lore.kernel.org/lkml/20200120045424.16147-1-dja@axtens.net > >> I think we can likely do this all at the same time, merge the > >> complementary pieces, etc. > > > > You are welcome! > > Just to be sure I understand correctly: you want me to add work of Daniel > > Axtens to my local version, then add my modifications on top of his work > > and republish the whole patch set? > > That would make sense to me: apply my patches from the list and include > them in your next patch series. If you need to modify my patches, just > add your Signed-off-by: after mine. > > I don't know if my patches still apply to a current tree: let me know if > you need any help getting them up to date. > > Kind regards, > Daniel > I succeeded to apply your patches but I do not think the result is correct in terms of semantic (your second patch contains your changes plus lkdtm_CORRUPT_PAC which was on my HEAD). I will see this for the reviews of the next version. The important is that I can work on top of your modifications. > >> Notes below... > >> > >> > 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 | 37 +++++++++++++++++++++++++++++ > >> > drivers/misc/lkdtm/lkdtm.h | 17 ++++++++------ > >> > >> Yay tests! These should, however, be a separate patch. > > > > Ok, I will separate it. > > If I understand correctly: one semantic modification = one commit. > > > >> > include/linux/string.h | 45 ++++++++++++++++++++++++++++++++++++ > >> > 5 files changed, 94 insertions(+), 7 deletions(-) > >> > 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 a5e344df9166..979f9e3feefd 100644 > >> > --- a/drivers/misc/lkdtm/core.c > >> > +++ b/drivers/misc/lkdtm/core.c > >> > @@ -178,6 +178,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..0397d2def66d > >> > --- /dev/null > >> > +++ b/drivers/misc/lkdtm/fortify.c > >> > @@ -0,0 +1,37 @@ > >> > +// 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 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) + char *src; > >> > + char dst[3]; > >> > + > >> > + src = kmalloc(7, GFP_KERNEL); > >> > + src[0] = 'f'; > >> > + src[1] = 'o'; > >> > + src[2] = 'o'; > >> > + src[3] = 'b'; > >> > + src[4] = 'a'; > >> > + src[5] = 'r'; > >> > + src[6] = '\0'; > >> > >> Hah, yes, I guess we need to bypass the common utilities. ;) I wonder if > >> using __underlying_strcpy() might be easier. > > > > I am sorry but I did not understand. > > If we use here __underlying_strcpy() the function this will not profit > > from the protection added in fortified version of strscpy()? > > > >> > + > >> > + strscpy(dst, src, 1000); > >> > + > >> > + kfree(dst); > >> > + > >> > + pr_info("Fail: No overflow in above strscpy call!\n"); > >> > +#endif > >> > +} > >> > >> One thing I'd love to see is a _compile-time_ test too: but it needs to > >> be a negative failure case, which Makefiles are not well suited to > >> dealing with. e.g. something like: > >> > >> good.o: nop.c bad.c > >> > >> if $(CC) .... -o bad.o bad.c $< ; then exit 1; else $(CC) ... -o good.c > >> > >> nop.c ; fi > >> > >> I'm not sure how to do it. > > > > This is a good idea, I though to it but I did not see an easy way to deal > > with it. > > I will investigate one it, but I cannot guarantee the next version will > > come with this feature. > > > >> > diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h > >> > index 8878538b2c13..8e5e90eb0e00 100644 > >> > --- a/drivers/misc/lkdtm/lkdtm.h > >> > +++ b/drivers/misc/lkdtm/lkdtm.h > >> > @@ -6,7 +6,7 @@ > >> > > >> > #include <linux/kernel.h> > >> > > >> > -/* lkdtm_bugs.c */ > >> > +/* bugs.c */ > >> > >> oops, yes. Can you split change from the others, since it's an unrelated > >> clean-up. > > > > Understand, it will be done for next version! > > > >> > void __init lkdtm_bugs_init(int *recur_param); > >> > void lkdtm_PANIC(void); > >> > void lkdtm_BUG(void); > >> > > >> > @@ -34,7 +34,7 @@ void lkdtm_UNSET_SMEP(void); > >> > > >> > void lkdtm_DOUBLE_FAULT(void); > >> > void lkdtm_CORRUPT_PAC(void); > >> > > >> > -/* lkdtm_heap.c */ > >> > +/* heap.c */ > >> > > >> > void __init lkdtm_heap_init(void); > >> > void __exit lkdtm_heap_exit(void); > >> > void lkdtm_OVERWRITE_ALLOCATION(void); > >> > > >> > @@ -46,7 +46,7 @@ void lkdtm_SLAB_FREE_DOUBLE(void); > >> > > >> > void lkdtm_SLAB_FREE_CROSS(void); > >> > void lkdtm_SLAB_FREE_PAGE(void); > >> > > >> > -/* lkdtm_perms.c */ > >> > +/* perms.c */ > >> > > >> > void __init lkdtm_perms_init(void); > >> > void lkdtm_WRITE_RO(void); > >> > void lkdtm_WRITE_RO_AFTER_INIT(void); > >> > > >> > @@ -61,7 +61,7 @@ void lkdtm_EXEC_NULL(void); > >> > > >> > void lkdtm_ACCESS_USERSPACE(void); > >> > void lkdtm_ACCESS_NULL(void); > >> > > >> > -/* lkdtm_refcount.c */ > >> > +/* refcount.c */ > >> > > >> > void lkdtm_REFCOUNT_INC_OVERFLOW(void); > >> > void lkdtm_REFCOUNT_ADD_OVERFLOW(void); > >> > void lkdtm_REFCOUNT_INC_NOT_ZERO_OVERFLOW(void); > >> > > >> > @@ -82,10 +82,10 @@ void lkdtm_REFCOUNT_SUB_AND_TEST_SATURATED(void); > >> > > >> > void lkdtm_REFCOUNT_TIMING(void); > >> > void lkdtm_ATOMIC_TIMING(void); > >> > > >> > -/* lkdtm_rodata.c */ > >> > +/* rodata.c */ > >> > > >> > void lkdtm_rodata_do_nothing(void); > >> > > >> > -/* lkdtm_usercopy.c */ > >> > +/* usercopy.c */ > >> > > >> > void __init lkdtm_usercopy_init(void); > >> > void __exit lkdtm_usercopy_exit(void); > >> > void lkdtm_USERCOPY_HEAP_SIZE_TO(void); > >> > > >> > @@ -98,10 +98,13 @@ void lkdtm_USERCOPY_STACK_BEYOND(void); > >> > > >> > void lkdtm_USERCOPY_KERNEL(void); > >> > void lkdtm_USERCOPY_KERNEL_DS(void); > >> > > >> > -/* lkdtm_stackleak.c */ > >> > +/* stackleak.c */ > >> > > >> > void lkdtm_STACKLEAK_ERASING(void); > >> > > >> > /* cfi.c */ > >> > void lkdtm_CFI_FORWARD_PROTO(void); > >> > > >> > +/* fortify.c */ > >> > +void lkdtm_FORTIFIED_STRSCPY(void); > >> > + > >> > > >> > #endif > >> > > >> > diff --git a/include/linux/string.h b/include/linux/string.h > >> > index b1f3894a0a3e..b661863619e0 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 */ > >> > +#include <linux/errno.h> /* for E2BIG */ > >> > > >> > #include <stdarg.h> > >> > #include <uapi/linux/string.h> > >> > > >> > @@ -357,6 +359,49 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const > >> > char > >> > *q, size_t size)> > >> > > >> > return ret; > >> > > >> > } > >> > > >> > +/* defined after fortified strlen 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 count) > >> > >> I would name "count" as "size" to match the other helpers. > > > > I decided to keep count because it is the argument name in unfortified > > version of strscpy > > I will change the name for next version to stick with all the fortified > > functions arguments. > > > >> > +{ > >> > + size_t len; > >> > + size_t p_size = __builtin_object_size(p, 0); > >> > + size_t q_size = __builtin_object_size(q, 0); > >> > >> These can be using ", 1" instead of ", 0". And I'll grab the related > >> changes from the mentioned series above. > > > > I looked Daniel Axtens patch and understood why it is better to use 1 > > instead of 0 so I will add it for the next version. > > > >> > + /* > >> > + * If p_size and q_size cannot be known at compile time we just had > >> > to > >> > + * trust this function caller. > >> > + */ > >> > + if (p_size == (size_t)-1 && q_size == (size_t)-1) > >> > + return __real_strscpy(p, q, count); > >> > + len = strlen(q); > >> > + if (count) { > >> > >> This test isn't needed; it'll work itself out correctly. :P > > > > Indeed, if this condition is met, __real_strscpy will be called later. > > > >> > + /* If count is bigger than INT_MAX, strscpy returns -E2BIG. */ > >> > + if (WARN_ON_ONCE(count > INT_MAX)) > >> > + return -E2BIG; > >> > >> This is already handled in strscpy, I'd drop this here. > > > > I though of it at first, but since the patch modify count/size before > > giving it to __real_strscpy(), real one will never return -E2BIG due to > > that. So removing this modification will lead to difference between > > returned value of fortified strscpy() and __real_strscpy(). > > > >> > + /* > >> > + * strscpy handles read overflows by stop reading q when '\0' is > >> > + * met. > >> > + * We stick to this behavior here. > >> > + */ > >> > + len = (len >= count) ? count : len; > >> > + /* > >> > + * If len can be known at compile time and is greater than > >> > + * p_size, generate a compile time write overflow error. > >> > + */ > >> > + if (__builtin_constant_p(len) && len > p_size) > >> > >> This won't work (len wasn't an argument and got assigned); you need: > >> if (__builtin_constant_p(size) && p_size < size) > > > > You are right, len is unknown at compile time... So, I will correct it for > > next version! > > > >> > + __write_overflow(); > >> > + /* Otherwise generate a runtime write overflow error. */ > >> > + if (len > p_size) > >> > + fortify_panic(__func__); > >> > >> I think this just needs to be: > >> if (p_size < size) > >> > >> fortify_panic(__func__); > > > > I am not really sure. > > If p_size is 4, size is 1000 and q is "foo\0", then what you suggested > > will > > panic but there is not need to panic since __real_strscpy will truncate > > size and copy just 4 bytes into p (because of '\0' in q). > > Am I correct? > > > >> > + /* > >> > + * Still use count as third argument to correctly compute max > >> > + * inside strscpy. > >> > + */ > >> > + return __real_strscpy(p, q, count); > >> > + } > >> > + /* If count is 0, strscpy return -E2BIG. */ > >> > + return -E2BIG; > >> > >> I'd let __real_strscpy() handle this. > > > > See my three times above comment. > > __real_strscpy is called only if count > 0, so it will never return -E2BIG > > due to this. > > So it will lead to difference in returned value between fortified > > strscpy() and __real_strscpy(). > > > >> > +} > >> > + > >> > > >> > /* 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