Discussion:
[PATCH 1/5] random, stackprotect: introduce get_random_canary function
(too old to reply)
r***@redhat.com
2017-05-19 21:26:32 UTC
Permalink
From: Rik van 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 <***@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
r***@redhat.com
2017-05-19 21:26:34 UTC
Permalink
From: Rik van 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 <***@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
r***@redhat.com
2017-05-19 21:26:35 UTC
Permalink
From: Rik van 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 <***@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
r***@redhat.com
2017-05-19 21:26:33 UTC
Permalink
From: Rik van 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 <***@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
r***@redhat.com
2017-05-19 21:26:36 UTC
Permalink
From: Rik van 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 <***@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
Kees Cook
2017-05-19 21:32:22 UTC
Permalink
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 <***@chromium.org>

-Kees
--
Kees Cook
Pixel Security
Geert Uytterhoeven
2017-05-24 11:57:52 UTC
Permalink
Post by Kees Cook
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 -- ***@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
Daniel Micay
2017-05-19 23:57:07 UTC
Permalink
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.
Ingo Molnar
2017-05-24 08:30:43 UTC
Permalink
Post by r***@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.
---
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
Post by r***@redhat.com
+ * 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

Loading...