Date:   Mon, 19 Oct 2020 16:19:43 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Francis Laniel <laniel_francis@...vacyrequired.com>
Cc:     linux-hardening@...r.kernel.org, Daniel Axtens <dja@...ens.net>
Subject: Re: [RFC][PATCH v2] Fortify string function strscpy.

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!

> > > +/*
> > > + * 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] = ...

> > > +
> > > +	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.

> > > +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().

> > > +	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.

> > 
> > > +		/*
> > > +		 * 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"

> 
> > > +		/*
> > > +		 * 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.

-- 
Kees Cook

Powered by blists - more mailing lists