stackprotector: ascii armor the stack canary

stackprotector: ascii armor the stack canary

[riel at redhat.com]

Zero out the first byte of the stack canary value on 64 bit systems,
in order to prevent unterminated C string overflows from being able
to successfully overwrite the canary, even if an attacker somehow
guessed or obtained the canary value.

Inspired by execshield ascii-armor and PaX/grsecurity.

Thanks to Daniel Micay for extracting code of similar functionality
from PaX/grsecurity and making it easy to find in his linux-hardened
git tree on https://github.com/thestinger/linux-hardened/

[PATCH 1/5] random, stackprotect: introduce get_random_canary function

[riel at redhat.com]

From: Rik van Riel <riel@redhat.com>

Introduce the get_random_canary function, which provides a random
unsigned long canary value with the first byte zeroed out on 64
bit architectures, in order to mitigate non-terminated C string
overflows.

Inspired by the "ascii armor" code in the old execshield patches,
and the current PaX/grsecurity code base.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 include/linux/random.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/random.h b/include/linux/random.h
index ed5c3838780d..765a992c6774 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -57,6 +57,26 @@ static inline unsigned long get_random_long(void)
 #endif
 }
 
+/*
+ * On 64 bit architectures, protect against non-terminated C string overflows
+ * by zeroing out the first byte of the canary; this leaves 56 bits of entropy.
+ */
+#ifdef CONFIG_64BIT
+#ifdef __LITTLE_ENDIAN
+#define CANARY_MASK 0xffffffffffffff00UL
+#else /* big endian 64 bits */
+#define CANARY_MASK 0x00ffffffffffffffUL
+#endif
+#else /* 32 bits */
+#define CANARY_MASK 0xffffffffUL
+#endif
+static inline unsigned long get_random_canary(void)
+{
+	unsigned long val = get_random_long();
+
+	return val & CANARY_MASK;
+}
+
 unsigned long randomize_page(unsigned long start, unsigned long range);
 
 u32 prandom_u32(void);
-- 
2.9.3

[PATCH 1/5] random,stackprotect: introduce get_random_canary function

[Ingo Molnar]

* riel at redhat.com <riel@redhat.com> wrote:

> From: Rik van Riel <riel@redhat.com>
> 
> Introduce the get_random_canary function, which provides a random
> unsigned long canary value with the first byte zeroed out on 64
> bit architectures, in order to mitigate non-terminated C string
> overflows.
> 
> Inspired by the "ascii armor" code in the old execshield patches,
> and the current PaX/grsecurity code base.
> 
> Signed-off-by: Rik van Riel <riel@redhat.com>
> ---
>  include/linux/random.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/include/linux/random.h b/include/linux/random.h
> index ed5c3838780d..765a992c6774 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -57,6 +57,26 @@ static inline unsigned long get_random_long(void)
>  #endif
>  }
>  
> +/*
> + * On 64 bit architectures, protect against non-terminated C string overflows

s/64 bit/64-bit

> + * by zeroing out the first byte of the canary; this leaves 56 bits of entropy.
> + */
> +#ifdef CONFIG_64BIT
> +#ifdef __LITTLE_ENDIAN
> +#define CANARY_MASK 0xffffffffffffff00UL
> +#else /* big endian 64 bits */
> +#define CANARY_MASK 0x00ffffffffffffffUL
> +#endif
> +#else /* 32 bits */
> +#define CANARY_MASK 0xffffffffUL
> +#endif
> +static inline unsigned long get_random_canary(void)
> +{
> +	unsigned long val = get_random_long();
> +
> +	return val & CANARY_MASK;
> +}

Please separate function definitions from macros by a separate empty line.

Also, a bit of structure and organization would make the macro easier to read:

#ifdef CONFIG_64BIT
# ifdef __LITTLE_ENDIAN
#  define CANARY_MASK 0xffffffffffffff00UL
# else /* big endian, 64 bits: */
#  define CANARY_MASK 0x00ffffffffffffffUL
# endif
#else /* 32 bits: */
# define  CANARY_MASK 0x00000000ffffffffUL
#endif

Thanks,

	Ingo

[PATCH 2/5] fork, random: use get_random_canary to set tsk->stack_canary

[riel at redhat.com]

From: Rik van Riel <riel@redhat.com>

Use the ascii-armor canary to prevent unterminated C string overflows
from being able to successfully overwrite the canary, even if they
somehow obtain the canary value.

Inspired by execshield ascii-armor and PaX/grsecurity.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 kernel/fork.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index aa1076c5e4a9..b3591e9250a8 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -560,7 +560,7 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
 	set_task_stack_end_magic(tsk);
 
 #ifdef CONFIG_CC_STACKPROTECTOR
-	tsk->stack_canary = get_random_long();
+	tsk->stack_canary = get_random_canary();
 #endif
 
 	/*
-- 
2.9.3

[PATCH 3/5] x86: ascii armor the x86_64 boot init stack canary

[riel at redhat.com]

From: Rik van Riel <riel@redhat.com>

Use the ascii-armor canary to prevent unterminated C string overflows
from being able to successfully overwrite the canary, even if they
somehow obtain the canary value.

Inspired by execshield ascii-armor and PaX/grsecurity.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/x86/include/asm/stackprotector.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/stackprotector.h b/arch/x86/include/asm/stackprotector.h
index dcbd9bcce714..8abedf1d650e 100644
--- a/arch/x86/include/asm/stackprotector.h
+++ b/arch/x86/include/asm/stackprotector.h
@@ -74,6 +74,7 @@ static __always_inline void boot_init_stack_canary(void)
 	get_random_bytes(&canary, sizeof(canary));
 	tsc = rdtsc();
 	canary += tsc + (tsc << 32UL);
+	canary &= CANARY_MASK;
 
 	current->stack_canary = canary;
 #ifdef CONFIG_X86_64
-- 
2.9.3

[PATCH 4/5] arm64: ascii armor the arm64 boot init stack canary

[riel at redhat.com]

From: Rik van Riel <riel@redhat.com>

Use the ascii-armor canary to prevent unterminated C string overflows
from being able to successfully overwrite the canary, even if they
somehow obtain the canary value.

Inspired by execshield ascii-armor and PaX/grsecurity.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/arm64/include/asm/stackprotector.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/include/asm/stackprotector.h b/arch/arm64/include/asm/stackprotector.h
index fe5e287dc56b..b86a0865ddf1 100644
--- a/arch/arm64/include/asm/stackprotector.h
+++ b/arch/arm64/include/asm/stackprotector.h
@@ -30,6 +30,7 @@ static __always_inline void boot_init_stack_canary(void)
 	/* Try to get a semi random initial value. */
 	get_random_bytes(&canary, sizeof(canary));
 	canary ^= LINUX_VERSION_CODE;
+	canary &= CANARY_MASK;
 
 	current->stack_canary = canary;
 	__stack_chk_guard = current->stack_canary;
-- 
2.9.3

[PATCH 5/5] sh64: ascii armor the sh64 boot init stack canary

[riel at redhat.com]

From: Rik van Riel <riel@redhat.com>

Use the ascii-armor canary to prevent unterminated C string overflows
from being able to successfully overwrite the canary, even if they
somehow obtain the canary value.

Inspired by execshield ascii-armor and PaX/grsecurity.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 arch/sh/include/asm/stackprotector.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/sh/include/asm/stackprotector.h b/arch/sh/include/asm/stackprotector.h
index d9df3a76847c..141515a43b78 100644
--- a/arch/sh/include/asm/stackprotector.h
+++ b/arch/sh/include/asm/stackprotector.h
@@ -19,6 +19,7 @@ static __always_inline void boot_init_stack_canary(void)
 	/* Try to get a semi random initial value. */
 	get_random_bytes(&canary, sizeof(canary));
 	canary ^= LINUX_VERSION_CODE;
+	canary &= CANARY_MASK;
 
 	current->stack_canary = canary;
 	__stack_chk_guard = current->stack_canary;
-- 
2.9.3

stackprotector: ascii armor the stack canary

[Kees Cook]

On Fri, May 19, 2017 at 2:26 PM,  <riel@redhat.com> wrote:
> Zero out the first byte of the stack canary value on 64 bit systems,
> in order to prevent unterminated C string overflows from being able
> to successfully overwrite the canary, even if an attacker somehow
> guessed or obtained the canary value.

This also stops string functions from being able to read the canary.

It might also be worth mentioning that the reduction in entropy for
64-bit to gain this corner-case protection is worth it, but on 32-bit,
it is not. (Which is especially true given that the 64-bit canary was
only 32-bits in some cases until recently.)

> Inspired by execshield ascii-armor and PaX/grsecurity.
>
> Thanks to Daniel Micay for extracting code of similar functionality
> from PaX/grsecurity and making it easy to find in his linux-hardened
> git tree on https://github.com/thestinger/linux-hardened/

Thanks!

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Pixel Security

stackprotector: ascii armor the stack canary

[Geert Uytterhoeven]

On Fri, May 19, 2017 at 11:32 PM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, May 19, 2017 at 2:26 PM,  <riel@redhat.com> wrote:
>> Zero out the first byte of the stack canary value on 64 bit systems,
>> in order to prevent unterminated C string overflows from being able
>> to successfully overwrite the canary, even if an attacker somehow
>> guessed or obtained the canary value.
>
> This also stops string functions from being able to read the canary.
>
> It might also be worth mentioning that the reduction in entropy for
> 64-bit to gain this corner-case protection is worth it, but on 32-bit,
> it is not. (Which is especially true given that the 64-bit canary was
> only 32-bits in some cases until recently.)

+1

It took me a while to deduce that myself, when I started wondering why
this was not done for 32-bit.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-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

stackprotector: ascii armor the stack canary

[Daniel Micay]

On Fri, 2017-05-19 at 17:26 -0400, riel at redhat.com wrote:
> Zero out the first byte of the stack canary value on 64 bit systems,
> in order to prevent unterminated C string overflows from being able
> to successfully overwrite the canary, even if an attacker somehow
> guessed or obtained the canary value.
> 
> Inspired by execshield ascii-armor and PaX/grsecurity.
> 
> Thanks to Daniel Micay for extracting code of similar functionality
> from PaX/grsecurity and making it easy to find in his linux-hardened
> git tree on https://github.com/thestinger/linux-hardened/

To clarify something this part isn't from PaX / grsecurity. I marked the
commits from PaX / grsecurity as such in their commit messages and these
are among the ones that aren't from there.

This is from a set of changes that I did for CopperheadOS and forward
ported to mainline recently to start linux-hardened. It was only arm64
for CopperheadOS. The overlap with PaX is that when adding the leading
zero byte for x86, I needed to first fix get_random_int being used for
the per-task canary value. I didn't know PaX fixed it way back in 2007.

I implemented heap canaries for our userspace malloc implementation and
then later did the same thing for slub in the kernel. I added a leading
zero byte to both of those heap canaries later on and then did the SSP
implementation in Bionic and the kernel's arm64 code. I took the idea
from glibc but limited it to 64-bit where there's entropy to spare. The
glibc leading zero might have come from execshield, but I don't know.