stackprotector: ascii armor the stack canary
v2 cover stackprotector: ascii armor the stack canary
Zero out the first byte of the stack canary value on 64 bit systems, in order to mitigate unterminated C string overflows.
The null byte both prevents C string functions from reading the canary, and from writing it if the canary value were guessed or obtained through some other means.
Reducing the entropy by 8 bits is acceptable on 64-bit systems, which will still have 56 bits of entropy left, but not on 32 bit systems, so the "ascii armor" canary is only implemented on 64-bit systems.
Inspired by the "ascii armor" code in execshield and Daniel Micay's linux-hardened tree.
Also see https://github.com/thestinger/linux-hardened/
v2:
- improve changelogs
- address Ingo's coding style comments
PATCH v2 1/5 random,stackprotect: introduce get_random_canary function
From: Rik van Riel <[email protected]>
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.
The null byte both prevents C string functions from reading the canary, and from writing it if the canary value were guessed or obtained through some other means.
Reducing the entropy by 8 bits is acceptable on 64-bit systems, which will still have 56 bits of entropy left, but not on 32 bit systems, so the "ascii armor" canary is only implemented on 64-bit systems.
Inspired by the "ascii armor" code in the old execshield patches, and Daniel Micay's linux-hardened tree.
--- include/linux/random.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
include/linux/random.h +21 -0
@@ -57,6 +57,27 @@ 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 );
Signature · 1 line
2.9.3
PATCH v2 2/5 fork,random: use get_random_canary to set tsk->stack_canary
In reply to [email protected]From: Rik van Riel <[email protected]>
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 Daniel Micay's linux-hardened tree.
--- kernel/fork.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/fork.c +1 -1
@@ -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 /*
Signature · 1 line
2.9.3
PATCH v2 3/5 x86: ascii armor the x86_64 boot init stack canary
In reply to [email protected]From: Rik van Riel <[email protected]>
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 Daniel Micay's linux-hardened tree.
--- arch/x86/include/asm/stackprotector.h | 1 + 1 file changed, 1 insertion(+)
arch/x86/include/asm/stackprotector.h +1 -0
@@ -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
Signature · 1 line
2.9.3
PATCH v2 4/5 arm64: ascii armor the arm64 boot init stack canary
In reply to [email protected]From: Rik van Riel <[email protected]>
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 Daniel Micay's linux-hardened tree.
--- arch/arm64/include/asm/stackprotector.h | 1 + 1 file changed, 1 insertion(+)
arch/arm64/include/asm/stackprotector.h +1 -0
@@ -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;
Signature · 1 line
2.9.3
PATCH v2 5/5 sh64: ascii armor the sh64 boot init stack canary
In reply to [email protected]From: Rik van Riel <[email protected]>
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 Daniel Micay's linux-hardened tree.
--- arch/sh/include/asm/stackprotector.h | 1 + 1 file changed, 1 insertion(+)
arch/sh/include/asm/stackprotector.h +1 -0
@@ -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;
Signature · 1 line
2.9.3
Re: [PATCH 1/5] random,stackprotect: introduce get_random_canary function
In reply to [email protected]Quoted 16 lines · On Wed, May 24, 2017 at 8:57 AM, <[email protected]> wrote:
From: Rik van Riel <[email protected]>
Introduce the
get_random_canaryfunction, 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.The null byte both prevents C string functions from reading the canary, and from writing it if the canary value were guessed or obtained through some other means.
Reducing the entropy by 8 bits is acceptable on 64-bit systems, which will still have 56 bits of entropy left, but not on 32 bit systems, so the "ascii armor" canary is only implemented on 64-bit systems.
Quoted 4 lines
Inspired by the "ascii armor" code in the old execshield patches, and Daniel Micay's linux-hardened tree.
Signed-off-by Rik van Riel <[email protected]>
Since the other recent canary fix went via -mm, perhaps these should go that way too? If not, I can take them via my KSPP tree.
-Kees
Quoted 39 lines
--- include/linux/random.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)include/linux/random.h +21
-1@@ -57,6 +57,27 @@ 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
Signature · 2 lines
Kees Cook Pixel Security
Re: [PATCH 2/5] fork,random: use get_random_canary to set tsk->stack_canary
In reply to [email protected]Quoted 9 lines · On Wed, May 24, 2017 at 8:57 AM, <[email protected]> wrote:
From: Rik van Riel <[email protected]>
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 Daniel Micay's linux-hardened tree.
Signed-off-by Rik van Riel <[email protected]>
-Kees
Quoted 20 lines
--- kernel/fork.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)kernel/fork.c +1
-2@@ -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
Signature · 2 lines
Kees Cook Pixel Security
Re: [PATCH 3/5] x86: ascii armor the x86_64 boot init stack canary
In reply to [email protected]Quoted 9 lines · On Wed, May 24, 2017 at 8:57 AM, <[email protected]> wrote:
From: Rik van Riel <[email protected]>
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 Daniel Micay's linux-hardened tree.
Signed-off-by Rik van Riel <[email protected]>
-Kees
Quoted 19 lines
--- arch/x86/include/asm/stackprotector.h | 1 + 1 file changed, 1 insertion(+)arch/x86/include/asm/stackprotector.h +1
-1@@ -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
Signature · 2 lines
Kees Cook Pixel Security
Re: [PATCH 4/5] arm64: ascii armor the arm64 boot init stack canary
In reply to [email protected]Quoted 9 lines · On Wed, May 24, 2017 at 8:57 AM, <[email protected]> wrote:
From: Rik van Riel <[email protected]>
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 Daniel Micay's linux-hardened tree.
Signed-off-by Rik van Riel <[email protected]>
-Kees
Quoted 19 lines
--- arch/arm64/include/asm/stackprotector.h | 1 + 1 file changed, 1 insertion(+)arch/arm64/include/asm/stackprotector.h +1
-1@@ -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
Signature · 2 lines
Kees Cook Pixel Security
PATCH v2 5/5 sh64: ascii armor the sh64 boot init stack canary
In reply to [email protected]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 Daniel Micay's linux-hardened tree.
--- arch/sh/include/asm/stackprotector.h | 1 + 1 file changed, 1 insertion(+)
arch/sh/include/asm/stackprotector.h +1 -0
@@ -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;
Re: [PATCH 5/5] sh64: ascii armor the sh64 boot init stack canary
Quoted 7 lines · Rik van Riel
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 Daniel Micay's linux-hardened tree.
Signed-off-by Rik van Riel <[email protected]>
-Kees
Quoted 17 lines
--- arch/sh/include/asm/stackprotector.h | 1 + 1 file changed, 1 insertion(+)arch/sh/include/asm/stackprotector.h +1
-0@@ -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;
Signature · 2 lines
Kees Cook Pixel Security
Quoted 12 lines · On Wed, May 24, 2017 at 11:57:46AM -0400, [email protected] wrote:
Zero out the first byte of the stack canary value on 64 bit systems, in order to mitigate unterminated C string overflows.
The null byte both prevents C string functions from reading the canary, and from writing it if the canary value were guessed or obtained through some other means.
Reducing the entropy by 8 bits is acceptable on 64-bit systems, which will still have 56 bits of entropy left, but not on 32 bit systems, so the "ascii armor" canary is only implemented on 64-bit systems.
Quoted 4 lines
Inspired by the "ascii armor" code in execshield and Daniel Micay's linux-hardened tree.
Brad trolls us all lightly with this trivia question:
https://twitter.com/grsecurity/status/905246423591084033
"For #trivia can you describe one scenario where this change actually helps exploitation using similar C string funcs?"
I suppose the expected answer is:
The change helps exploitation when the overwriting string ends just before the canary. Its NUL overwriting the NUL byte in the canary would go undetected. Before this change, there would be a 255/256 chance of detection.
I hope this was considered. The change might still be a good tradeoff, or it might not, depending on which scenarios are more likely (leak of canary value or the string required in an exploit ending at that exact byte location), and we probably lack such statistics.
I am not proposing to revert the change. I had actually contemplated speaking up when this was discussed, but did not for lack of a better suggestion. We could put/require a NUL in the middle of the canary, but with the full canary being only 64-bit at most that would also make some attacks easier.
So this is JFYI. No action needed on it, I think.
Alexander
Quoted 37 lines · Solar Designer
Quoted 12 lines · On Wed, May 24, 2017 at 11:57:46AM -0400, [email protected] wrote:
Zero out the first byte of the stack canary value on 64 bit systems, in order to mitigate unterminated C string overflows.
The null byte both prevents C string functions from reading the canary, and from writing it if the canary value were guessed or obtained through some other means.
Reducing the entropy by 8 bits is acceptable on 64-bit systems, which will still have 56 bits of entropy left, but not on 32 bit systems, so the "ascii armor" canary is only implemented on 64-bit systems.
Quoted 4 lines
Inspired by the "ascii armor" code in execshield and Daniel Micay's linux-hardened tree.
Brad trolls us all lightly with this trivia question:
https://twitter.com/grsecurity/status/905246423591084033
"For #trivia can you describe one scenario where this change actually helps exploitation using similar C string funcs?"
I suppose the expected answer is:
The change helps exploitation when the overwriting string ends just before the canary. Its NUL overwriting the NUL byte in the canary would go undetected. Before this change, there would be a 255/256 chance of detection.
I hope this was considered. The change might still be a good tradeoff, or it might not, depending on which scenarios are more likely (leak of canary value or the string required in an exploit ending at that exact byte location), and we probably lack such statistics.
Quoted 7 lines
I am not proposing to revert the change. I had actually contemplated speaking up when this was discussed, but did not for lack of a better suggestion. We could put/require a NUL in the middle of the canary, but with the full canary being only 64-bit at most that would also make some attacks easier.
So this is JFYI. No action needed on it, I think.
Agreed. And prior to this:
https://git.kernel.org/linus/5ea30e4e58040cfd6434c2f33dc3ea76e2c15b05
the same kernels had 4 NULL bytes in a row. ;) So it's all an improvement, IMO.
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Quoted 3 lines
Brad trolls us all lightly with this trivia question:
I'll respond to your proposed scenario rather than guessing at what is being suggested there and if it's actually the same thing as what you've brought up.
They've stated many times that stack canaries are near useless and enabling PaX UDEREF on i386 actually disabled stack canaries and presumably still does after the last public patches:
+ select HAVE_CC_STACKPROTECTOR if X86_64 || !PAX_MEMORY_UDEREF
Making the kernel more vulnerable to remote stack overflow exploits like this month's overhyped Bluetooth bug to improve security against local exploits seems like much more of a compromise. Not that i386 has any real relevance anymore, but I think that's some interesting context...
It's not like upstream has a monopoly on needing to make hard choices for security features, I don't see this as one of those hard choices like whether sacrificing availability for SIZE_OVERFLOW makes sense.
Quoted 11 lines
"For #trivia can you describe one scenario where this change actually helps exploitation using similar C string funcs?"
I suppose the expected answer is:
The change helps exploitation when the overwriting string ends just before the canary. Its NUL overwriting the NUL byte in the canary would
go undetected. Before this change, there would be a 255/256 chance of detection.
Quoted 5 lines
I hope this was considered. The change might still be a good tradeoff,
or it might not, depending on which scenarios are more likely (leak of canary value or the string required in an exploit ending at that exact byte location), and we probably lack such statistics.
It was considered that guaranteeing some forms of string overflows are contained within the frame can have a negative impact but I don't think it's significant.
There would need to be something worth overwriting between the source of the overflow and the canary. Since SSP reorders local variables within the stack frame based on risk of overflow, the targets would be locals that Clang/GCC considers to have a higher risk of overflow. An array can only have other arrays between it and the canary and there's ordering by size (small vs. large at least) within the array section. If there was a zero byte in any of that other data between the source of the overflow and the canary, this wouldn't make a difference anyway.
Since the canary is only checked on return, it's also only relevant if control flow still makes it to the function epilogue and it still matters after the attacker has accomplished their goal.
The chance of there being a zero as a leading byte is already 1/256, but there could also be a NUL elsewhere in the canary compromising a subset of the entropy for this kind of scenario too so it's a tiny bit higher than a 1/256 chance already. Not much resistance to brute force if it really does ever end up mattering, unless canaries with NUL in leading positions were actually *rejected*...
There's a more substantial compromise between zero and non-zero poison on free but for production use. This was part of a set of changes where CopperheadOS switched from non-zero fill -> zero fill on free for the kernel and userspace and added a leading zero byte on 64-bit for heap and stack canaries in the production builds while keeping around the old way of doing things for some forms of debug builds. Non-zero fill on free ended up causing too much harm by turning benign bugs into bugs that might have been exploitable even beyond DoS in some cases tied to non-NUL-terminated strings where non-zero poisoning got rid of zeroes that were otherwise all over the heap containing them or when it broke code wrongly depending on uninitialized data being zero in practice. I think there's more of an argument to be had about poison-on-free as a mitigation. This leading zero byte? Not so much, beyond perhaps wanting the option to turn it off for bug finding... but there are better tools for that now.
Quoted 9 lines
I am not proposing to revert the change. I had actually contemplated speaking up when this was discussed, but did not for lack of a better suggestion. We could put/require a NUL in the middle of the canary, but
with the full canary being only 64-bit at most that would also make some
attacks easier.So this is JFYI. No action needed on it, I think.
It might make sense to put it in the middle, but a scenario where it matters seems unlikely and as you mention it would make it weaker it some other cases. I think it's already the best choice right now.
I'd choose XOR canaries over a leading zero byte if it could only be one or the other, but I'm not really convinced that there's any significant loss compared to the normal canaries.
Hi,
Quoted 5 lines · Solar Designer
We could put/require a NUL in the middle of the canary, but with the full canary being only 64-bit at most that would also make some attacks easier.
Are you suggesting to randomly select which byte to set to 0 in each canary ?
Regards.
Signature · 2 lines
Yann Droneaud OPTEYA
Quoted 8 lines · Yann Droneaud
Le mardi 19 septembre 2017 ?? 19:16 +0200, Solar Designer a ??crit :
Quoted 4 lines
We could put/require a NUL in the middle of the canary, but with the full canary being only 64-bit at most that would also make some attacks easier.
Are you suggesting to randomly select which byte to set to 0 in each canary ?
Definitely not. That's only 8 different possibilities per canary, and the weakest one will affect exploitability in each scenario. So that would be a fairly clear change to the worse.
I suggest that we make no further changes at this time, unless someone comes up with an idea that would clearly hurt exploitation more than it helps exploitation, overall across different scenarios.
Alexander