| 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: <CAMuHMdX1H_ssPDJH47kcXhmoAZzYEgJC2zaMp-d_2+VriZYAoA@mail.gmail.com>
Date: Thu, 1 Sep 2022 10:39:19 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Kees Cook <keescook@...omium.org>
Cc: Wolfram Sang <wsa+renesas@...g-engineering.com>,
Nick Desaulniers <ndesaulniers@...gle.com>,
Guenter Roeck <linux@...ck-us.net>,
Paolo Abeni <pabeni@...hat.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-hardening@...r.kernel.org,
Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] string: Introduce strtomem() and strtomem_pad()
Hi Kees,
CC torvalds
Thanks for your patch!
On Thu, Sep 1, 2022 at 1:00 AM Kees Cook <keescook@...omium.org> wrote:
> One of the "legitimate" uses of strncpy() is copying a NUL-terminated
> string into a fixed-size non-NUL-terminated character array. To avoid
> the weaknesses and ambiguity of intent when using strncpy(), provide
> replacement functions that explicitly distinguish between trailing
> padding and not, and require the destination buffer size be discoverable
> by the compiler.
>
> For example:
>
> struct obj {
> int foo;
> char small[4] __nonstring;
> char big[8] __nonstring;
> int bar;
> };
>
> struct obj p;
>
> /* This will truncate to 4 chars with no trailing NUL */
> strncpy(p.small, "hello", sizeof(p.small));
> /* p.small contains 'h', 'e', 'l', 'l' */
>
> /* This will NUL pad to 8 chars. */
> strncpy(p.big, "hello", sizeof(p.big));
> /* p.big contains 'h', 'e', 'l', 'l', 'o', '\0', '\0', '\0' */
>
> When the "__nonstring" attributes are missing, the intent of the
> programmer becomes ambiguous for whether the lack of a trailing NUL
> in the p.small copy is a bug. Additionally, it's not clear whether
> the trailing padding in the p.big copy is _needed_. Both cases
> become unambiguous with:
>
> strtomem(p.small, "hello");
> strtomem_pad(p.big, "hello");
strtomem_pad(p.big, "hello", 0);
> See also https://github.com/KSPP/linux/issues/90
>
> Expand the memcpy KUnit tests to include these functions.
>
> Cc: Wolfram Sang <wsa+renesas@...g-engineering.com>
> Cc: Nick Desaulniers <ndesaulniers@...gle.com>
> Cc: Geert Uytterhoeven <geert@...ux-m68k.org>
> Cc: Guenter Roeck <linux@...ck-us.net>
> Signed-off-by: Kees Cook <keescook@...omium.org>
The idea looks good to me, but I guess Linus has something to
say, too.
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -260,6 +260,49 @@ static inline const char *kbasename(const char *path)
> void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
> int pad);
>
> +/**
> + * strtomem_pad - Copy NUL-terminated string to non-NUL-terminated buffer
> + *
> + * @dest: Pointer of destination character array (marked as __nonstring)
> + * @src: Pointer to NUL-terminated string
> + * @pad: Padding character to fill any remaining bytes of @dest after copy
> + *
> + * This is a replacement for strncpy() uses where the destination is not
> + * a NUL-terminated string, but with bounds checking on the source size, and
> + * an explicit padding character. If padding is not required, use strtomem().
> + *
> + * Note that the size of @dest is not an argument, as the length of @dest
> + * must be discoverable by the compiler.
> + */
> +#define strtomem_pad(dest, src, pad) do { \
> + const size_t _dest_len = __builtin_object_size(dest, 1); \
> + \
> + BUILD_BUG_ON(!__builtin_constant_p(_dest_len) || \
> + _dest_len == (size_t)-1); \
I think you want to include __must_be_array(dest) here.
> + memcpy_and_pad(dest, _dest_len, src, strnlen(src, _dest_len), pad); \
> +} while (0)
> +
> +/**
> + * strtomem - Copy NUL-terminated string to non-NUL-terminated buffer
> + *
> + * @dest: Pointer of destination character array (marked as __nonstring)
> + * @src: Pointer to NUL-terminated string
> + *
> + * This is a replacement for strncpy() uses where the destination is not
> + * a NUL-terminated string, but with bounds checking on the source size, and
> + * without trailing padding. If padding is required, use strtomem_pad().
> + *
> + * Note that the size of @dest is not an argument, as the length of @dest
> + * must be discoverable by the compiler.
> + */
> +#define strtomem(dest, src) do { \
> + const size_t _dest_len = __builtin_object_size(dest, 1); \
> + \
> + BUILD_BUG_ON(!__builtin_constant_p(_dest_len) || \
> + _dest_len == (size_t)-1); \
I think you want to include __must_be_array(dest) here.
> + memcpy(dest, src, min(_dest_len, strnlen(src, _dest_len))); \
> +} while (0)
> +
> /**
> * memset_after - Set a value after a struct member to the end of a struct
> *
> diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
> index 62f8ffcbbaa3..2aeb8643e1b0 100644
> --- a/lib/memcpy_kunit.c
> +++ b/lib/memcpy_kunit.c
> @@ -272,10 +272,63 @@ static void memset_test(struct kunit *test)
> #undef TEST_OP
> }
>
> +static void strtomem_test(struct kunit *test)
> +{
> + static const char input[] = "hi";
> + static const char truncate[] = "this is too long";
> + struct {
> + unsigned long canary1;
> + unsigned char output[sizeof(unsigned long)] __nonstring;
> + unsigned long canary2;
> + } wrap;
> +
> + memset(&wrap, 0xFF, sizeof(wrap));
> + KUNIT_EXPECT_EQ_MSG(test, wrap.canary1, -1UL,
-1L or ULONG_MAX (everywhere)
> + "bad initial canary value");
> + KUNIT_EXPECT_EQ_MSG(test, wrap.canary2, -1UL,
> + "bad initial canary value");
> +
> + /* Check unpadded copy leaves surroundings untouched. */
> + strtomem(wrap.output, input);
> + KUNIT_EXPECT_EQ(test, wrap.canary1, -1UL);
> + KUNIT_EXPECT_EQ(test, wrap.output[0], input[0]);
> + KUNIT_EXPECT_EQ(test, wrap.output[1], input[1]);
> + for (int i = 2; i < sizeof(wrap.output); i++)
unsigned int i (everywhere)
> + KUNIT_EXPECT_EQ(test, wrap.output[i], 0xFF);
> + KUNIT_EXPECT_EQ(test, wrap.canary2, -1UL);
> +
> + /* Check truncated copy leaves surroundings untouched. */
> + memset(&wrap, 0xFF, sizeof(wrap));
> + strtomem(wrap.output, truncate);
> + KUNIT_EXPECT_EQ(test, wrap.canary1, -1UL);
> + for (int i = 0; i < sizeof(wrap.output); i++)
> + KUNIT_EXPECT_EQ(test, wrap.output[i], truncate[i]);
> + KUNIT_EXPECT_EQ(test, wrap.canary2, -1UL);
> +
> + /* Check padded copy leaves only string padded. */
> + memset(&wrap, 0xFF, sizeof(wrap));
> + strtomem_pad(wrap.output, input, 0xAA);
> + KUNIT_EXPECT_EQ(test, wrap.canary1, -1UL);
> + KUNIT_EXPECT_EQ(test, wrap.output[0], input[0]);
> + KUNIT_EXPECT_EQ(test, wrap.output[1], input[1]);
> + for (int i = 2; i < sizeof(wrap.output); i++)
> + KUNIT_EXPECT_EQ(test, wrap.output[i], 0xAA);
> + KUNIT_EXPECT_EQ(test, wrap.canary2, -1UL);
> +
> + /* Check truncated padded copy has no padding. */
> + memset(&wrap, 0xFF, sizeof(wrap));
> + strtomem(wrap.output, truncate);
> + KUNIT_EXPECT_EQ(test, wrap.canary1, -1UL);
> + for (int i = 0; i < sizeof(wrap.output); i++)
> + KUNIT_EXPECT_EQ(test, wrap.output[i], truncate[i]);
> + KUNIT_EXPECT_EQ(test, wrap.canary2, -1UL);
> +}
> +
> static struct kunit_case memcpy_test_cases[] = {
> KUNIT_CASE(memset_test),
> KUNIT_CASE(memcpy_test),
> KUNIT_CASE(memmove_test),
> + KUNIT_CASE(strtomem_test),
> {}
> };
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Powered by blists - more mailing lists