Message-ID: <11802252.guoqaGusNR@machine>
Date:   Wed, 21 Oct 2020 16:49:21 +0200
From:   Francis Laniel <laniel_francis@...vacyrequired.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-hardening@...r.kernel.org, Daniel Axtens <dja@...ens.net>
Subject: Re: [RFC][PATCH v2] Fortify string function strscpy.

Le mardi 20 octobre 2020, 01:19:43 CEST Kees Cook a écrit :
> On Sat, Oct 17, 2020 at 11:22:04AM +0200, Francis Laniel wrote:
> > 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?
> 
> Yup; I would rebase his, and then have your patches follow.
> 
> > > 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.
> 
> Right -- I'd like to see the tests be separate. Also, probably the new
> test should get added to tools/testing/selftests/lkdtm/tests.txt. I
> forgot to suggest that last time!

I separated my commit in three different for the v3.

> 
> > > > +/*
> > > > + * 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()?
> 
> Sorry, I meant that instead open-coding the assignment, you could use
> __underlying_strcpy(). Better yet, now that I look at it, would be:
> 
> 	src = strdup("foobar", GFP_KERNEL);
> 
> Instead of the kmalloc and src[0], src[1] = ...

I did not think about strdup... It is now used in v3.

> > > > +
> > > > +	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.
> 
> Yeah, this isn't required for the series; it's just me thinking out
> loud. It'd be really nice to validate the fortification on the compile
> side. Though, in following Linus's guidelines, it may need to be a
> warning, not a hard failure of the build. Hmm.

Should we redeclare __write_overflow() as a warning instead of an error?

> > > > +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)
> > > > +{
> > > > +	size_t len;
> > > > +	size_t p_size = __builtin_object_size(p, 0);
> > > > +	size_t q_size = __builtin_object_size(q, 0);
> > > > +	/*
> > > > +	 * 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);
> 
> I realize again that this needs to be strnlen(q, size), I think?
> Otherwise we're just repeating the flaws of strlcpy().

Using strnlen would save us when src does not contain '\0' so I replaced 
strlen by strnlen for v3.

> > > > +	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().
> 
> I think if you avoid modifying size, it'll work out correctly.

I removed all about modifying size for v3.
When I implemented that I though "you should recompute size here because 
strscpy can save you because it exits when '\0' is met".
Then I though about it and changed my mind because we need safety inside 
fortified version of strscpy and the maximum safety.

> > > > +		/*
> > > > +		 * 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?
> 
> Hm, so, if p_size is 4 and size is __builtin_constant_p(), and p_size
> < size, it should fail to compile. (The wrong max size is proposed.)
> But yes, you're right, for the runtime case, if len < p_size, we shouldn't
> panic. But that means the test needs to be "len >= p_size"

I do not agree on the fact that the condition should be "len >= p_size".
Indeed, there is not write overflow when len equals p_size, src can just be 
truncated when written to dst.
When len is strictly greater than p_size, there is, for sure, a write overflow.

> > > > +		/*
> > > > +		 * 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().
> 
> There are enough changes pending here that I'll wait for the next
> version to look at this part again. Regardless, we should at least have
> the tests described even if the compile-time ones can't be tested yet.

Normally the review for the next version should be easier.

Powered by blists - more mailing lists