* [PATCH] efi: Initialize canary to non-zero value @ 2023-11-12 3:23 Glenn Washburn 2023-11-12 7:22 ` Heinrich Schuchardt 0 siblings, 1 reply; 7+ messages in thread From: Glenn Washburn @ 2023-11-12 3:23 UTC (permalink / raw) To: grub-devel, Daniel Kiper; +Cc: Chris Coulson, Glenn Washburn The canary, __stack_chk_guard, is in the BSS and so will get initialized to zero if it is not explicitly initialized. If the UEFI firmware does not support the RNG protocol, then the canary will not be randomized and will be used as zero. This seems like a possibly easier value to write by an attacker. Initialize canary to static random bytes, so that it is still random when there is not RNG protocol. Signed-off-by: Glenn Washburn <development@efficientek.com> --- grub-core/kern/efi/init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c index 0e28bea17a76..b85d98ca47fd 100644 --- a/grub-core/kern/efi/init.c +++ b/grub-core/kern/efi/init.c @@ -41,7 +41,7 @@ static grub_guid_t rng_protocol_guid = GRUB_EFI_RNG_PROTOCOL_GUID; static grub_efi_uint8_t stack_chk_guard_buf[32]; -grub_addr_t __stack_chk_guard; +grub_addr_t __stack_chk_guard = (grub_addr_t) 0x92f2b7e2f193b25c; void __attribute__ ((noreturn)) __stack_chk_fail (void) -- 2.34.1 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] efi: Initialize canary to non-zero value 2023-11-12 3:23 [PATCH] efi: Initialize canary to non-zero value Glenn Washburn @ 2023-11-12 7:22 ` Heinrich Schuchardt 2023-11-13 16:18 ` Daniel Kiper 0 siblings, 1 reply; 7+ messages in thread From: Heinrich Schuchardt @ 2023-11-12 7:22 UTC (permalink / raw) To: Glenn Washburn; +Cc: Chris Coulson, The development of GNU GRUB, Daniel Kiper On 11/12/23 04:23, Glenn Washburn wrote: > The canary, __stack_chk_guard, is in the BSS and so will get initialized to > zero if it is not explicitly initialized. If the UEFI firmware does not > support the RNG protocol, then the canary will not be randomized and will > be used as zero. This seems like a possibly easier value to write by an > attacker. Initialize canary to static random bytes, so that it is still > random when there is not RNG protocol. > > Signed-off-by: Glenn Washburn <development@efficientek.com> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > grub-core/kern/efi/init.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c > index 0e28bea17a76..b85d98ca47fd 100644 > --- a/grub-core/kern/efi/init.c > +++ b/grub-core/kern/efi/init.c > @@ -41,7 +41,7 @@ static grub_guid_t rng_protocol_guid = GRUB_EFI_RNG_PROTOCOL_GUID; > > static grub_efi_uint8_t stack_chk_guard_buf[32]; > > -grub_addr_t __stack_chk_guard; > +grub_addr_t __stack_chk_guard = (grub_addr_t) 0x92f2b7e2f193b25c; > > void __attribute__ ((noreturn)) > __stack_chk_fail (void) _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi: Initialize canary to non-zero value 2023-11-12 7:22 ` Heinrich Schuchardt @ 2023-11-13 16:18 ` Daniel Kiper 2023-11-14 3:17 ` Glenn Washburn 2023-11-18 21:01 ` Glenn Washburn 0 siblings, 2 replies; 7+ messages in thread From: Daniel Kiper @ 2023-11-13 16:18 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Glenn Washburn, Chris Coulson, The development of GNU GRUB On Sun, Nov 12, 2023 at 08:22:42AM +0100, Heinrich Schuchardt wrote: > On 11/12/23 04:23, Glenn Washburn wrote: > > The canary, __stack_chk_guard, is in the BSS and so will get initialized to > > zero if it is not explicitly initialized. If the UEFI firmware does not > > support the RNG protocol, then the canary will not be randomized and will > > be used as zero. This seems like a possibly easier value to write by an > > attacker. Initialize canary to static random bytes, so that it is still > > random when there is not RNG protocol. > > > > Signed-off-by: Glenn Washburn <development@efficientek.com> > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> > > --- > > grub-core/kern/efi/init.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c > > index 0e28bea17a76..b85d98ca47fd 100644 > > --- a/grub-core/kern/efi/init.c > > +++ b/grub-core/kern/efi/init.c > > @@ -41,7 +41,7 @@ static grub_guid_t rng_protocol_guid = GRUB_EFI_RNG_PROTOCOL_GUID; > > > > static grub_efi_uint8_t stack_chk_guard_buf[32]; > > > > -grub_addr_t __stack_chk_guard; > > +grub_addr_t __stack_chk_guard = (grub_addr_t) 0x92f2b7e2f193b25c; I would add last sentence from the commit message before this line. I can do it for you before push... > > > > void __attribute__ ((noreturn)) > > __stack_chk_fail (void) Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi: Initialize canary to non-zero value 2023-11-13 16:18 ` Daniel Kiper @ 2023-11-14 3:17 ` Glenn Washburn 2023-11-14 4:05 ` Dimitri John Ledkov 2023-11-18 21:01 ` Glenn Washburn 1 sibling, 1 reply; 7+ messages in thread From: Glenn Washburn @ 2023-11-14 3:17 UTC (permalink / raw) To: Daniel Kiper Cc: Heinrich Schuchardt, Chris Coulson, The development of GNU GRUB On Mon, 13 Nov 2023 17:18:50 +0100 Daniel Kiper <dkiper@net-space.pl> wrote: > On Sun, Nov 12, 2023 at 08:22:42AM +0100, Heinrich Schuchardt wrote: > > On 11/12/23 04:23, Glenn Washburn wrote: > > > The canary, __stack_chk_guard, is in the BSS and so will get initialized to > > > zero if it is not explicitly initialized. If the UEFI firmware does not > > > support the RNG protocol, then the canary will not be randomized and will > > > be used as zero. This seems like a possibly easier value to write by an > > > attacker. Initialize canary to static random bytes, so that it is still > > > random when there is not RNG protocol. Ugh. s/not/no/ > > > > > > Signed-off-by: Glenn Washburn <development@efficientek.com> > > > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> > > > > --- > > > grub-core/kern/efi/init.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c > > > index 0e28bea17a76..b85d98ca47fd 100644 > > > --- a/grub-core/kern/efi/init.c > > > +++ b/grub-core/kern/efi/init.c > > > @@ -41,7 +41,7 @@ static grub_guid_t rng_protocol_guid = GRUB_EFI_RNG_PROTOCOL_GUID; > > > > > > static grub_efi_uint8_t stack_chk_guard_buf[32]; > > > > > > -grub_addr_t __stack_chk_guard; > > > +grub_addr_t __stack_chk_guard = (grub_addr_t) 0x92f2b7e2f193b25c; Just now reflecting on this I had the idea that perhaps even better is to have this randomly generated at build time and inserted as an autoconf variable. Then every build would have a different canary. I think I'll send a patch for this soon. > > I would add last sentence from the commit message before this line. > I can do it for you before push... Sure. And please remove the 't' mentioned above in the commit message. Glenn > > > > > > > void __attribute__ ((noreturn)) > > > __stack_chk_fail (void) > > Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi: Initialize canary to non-zero value 2023-11-14 3:17 ` Glenn Washburn @ 2023-11-14 4:05 ` Dimitri John Ledkov 2023-11-18 21:02 ` Glenn Washburn 0 siblings, 1 reply; 7+ messages in thread From: Dimitri John Ledkov @ 2023-11-14 4:05 UTC (permalink / raw) To: The development of GNU GRUB Cc: Daniel Kiper, Heinrich Schuchardt, Chris Coulson [-- Attachment #1.1: Type: text/plain, Size: 2682 bytes --] On Tue, 14 Nov 2023, 03:19 Glenn Washburn, <development@efficientek.com> wrote: > On Mon, 13 Nov 2023 17:18:50 +0100 > Daniel Kiper <dkiper@net-space.pl> wrote: > > > On Sun, Nov 12, 2023 at 08:22:42AM +0100, Heinrich Schuchardt wrote: > > > On 11/12/23 04:23, Glenn Washburn wrote: > > > > The canary, __stack_chk_guard, is in the BSS and so will get > initialized to > > > > zero if it is not explicitly initialized. If the UEFI firmware does > not > > > > support the RNG protocol, then the canary will not be randomized and > will > > > > be used as zero. This seems like a possibly easier value to write by > an > > > > attacker. Initialize canary to static random bytes, so that it is > still > > > > random when there is not RNG protocol. > > Ugh. s/not/no/ > > > > > > > > > Signed-off-by: Glenn Washburn <development@efficientek.com> > > > > > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > > > Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> > > > > > > --- > > > > grub-core/kern/efi/init.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c > > > > index 0e28bea17a76..b85d98ca47fd 100644 > > > > --- a/grub-core/kern/efi/init.c > > > > +++ b/grub-core/kern/efi/init.c > > > > @@ -41,7 +41,7 @@ static grub_guid_t rng_protocol_guid = > GRUB_EFI_RNG_PROTOCOL_GUID; > > > > > > > > static grub_efi_uint8_t stack_chk_guard_buf[32]; > > > > > > > > -grub_addr_t __stack_chk_guard; > > > > +grub_addr_t __stack_chk_guard = (grub_addr_t) 0x92f2b7e2f193b25c; > > Just now reflecting on this I had the idea that perhaps even better is > to have this randomly generated at build time and inserted as an > autoconf variable. Then every build would have a different canary. > I think I'll send a patch for this soon. > But please insure it is reproducible (as in reproducible builds initiative). But also, one already has time and date macros that are unique and reproducible in most distros. Can you use TIME macro as either the random value or the seed to make a suitable random number? https://reproducible-builds.org/docs/source-date-epoch/ and all the ways it is already available at cpp time. > > > > I would add last sentence from the commit message before this line. > > I can do it for you before push... > > Sure. And please remove the 't' mentioned above in the commit message. > > Glenn > > > > > > > > > > > void __attribute__ ((noreturn)) > > > > __stack_chk_fail (void) > > > > Daniel > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > [-- Attachment #1.2: Type: text/html, Size: 4405 bytes --] [-- Attachment #2: Type: text/plain, Size: 141 bytes --] _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi: Initialize canary to non-zero value 2023-11-14 4:05 ` Dimitri John Ledkov @ 2023-11-18 21:02 ` Glenn Washburn 0 siblings, 0 replies; 7+ messages in thread From: Glenn Washburn @ 2023-11-18 21:02 UTC (permalink / raw) To: Dimitri John Ledkov Cc: The development of GNU GRUB, Daniel Kiper, Heinrich Schuchardt, Chris Coulson On Tue, 14 Nov 2023 04:05:00 +0000 Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote: > On Tue, 14 Nov 2023, 03:19 Glenn Washburn, <development@efficientek.com> > wrote: > > > On Mon, 13 Nov 2023 17:18:50 +0100 > > Daniel Kiper <dkiper@net-space.pl> wrote: > > > > > On Sun, Nov 12, 2023 at 08:22:42AM +0100, Heinrich Schuchardt wrote: > > > > On 11/12/23 04:23, Glenn Washburn wrote: > > > > > The canary, __stack_chk_guard, is in the BSS and so will get > > initialized to > > > > > zero if it is not explicitly initialized. If the UEFI firmware does > > not > > > > > support the RNG protocol, then the canary will not be randomized and > > will > > > > > be used as zero. This seems like a possibly easier value to write by > > an > > > > > attacker. Initialize canary to static random bytes, so that it is > > still > > > > > random when there is not RNG protocol. > > > > Ugh. s/not/no/ > > > > > > > > > > > > Signed-off-by: Glenn Washburn <development@efficientek.com> > > > > > > > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > > > > > Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> > > > > > > > > --- > > > > > grub-core/kern/efi/init.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c > > > > > index 0e28bea17a76..b85d98ca47fd 100644 > > > > > --- a/grub-core/kern/efi/init.c > > > > > +++ b/grub-core/kern/efi/init.c > > > > > @@ -41,7 +41,7 @@ static grub_guid_t rng_protocol_guid = > > GRUB_EFI_RNG_PROTOCOL_GUID; > > > > > > > > > > static grub_efi_uint8_t stack_chk_guard_buf[32]; > > > > > > > > > > -grub_addr_t __stack_chk_guard; > > > > > +grub_addr_t __stack_chk_guard = (grub_addr_t) 0x92f2b7e2f193b25c; > > > > Just now reflecting on this I had the idea that perhaps even better is > > to have this randomly generated at build time and inserted as an > > autoconf variable. Then every build would have a different canary. > > I think I'll send a patch for this soon. > > > > > But please insure it is reproducible (as in reproducible builds initiative). This is a good idea. > But also, one already has time and date macros that are unique and > reproducible in most distros. Can you use TIME macro as either the random > value or the seed to make a suitable random number? > https://reproducible-builds.org/docs/source-date-epoch/ and all the ways it > is already available at cpp time. This link seems to be suggesting to use the SOURCE_DATE_EPOCH environment variable if it exists. Is this what you mean by "TIME macro"? If not, could you explain further? I'm wondering if it might be better to provide a --enable-stack-protector-init= configure option instead. I like the idea of not having this and everything be automated based on the reproducible value, which would create less work for those using this feature. However, I'm concerned that the reproducible value itself may not be suitable as a canary and that other tools/dependencies might be required to make it more suitable (eg. sha1sum). Do you have any thoughts on this? Glenn > > > > > > > > > I would add last sentence from the commit message before this line. > > > I can do it for you before push... > > > > Sure. And please remove the 't' mentioned above in the commit message. > > > > Glenn > > > > > > > > > > > > > > > void __attribute__ ((noreturn)) > > > > > __stack_chk_fail (void) > > > > > > Daniel > > > > _______________________________________________ > > Grub-devel mailing list > > Grub-devel@gnu.org > > https://lists.gnu.org/mailman/listinfo/grub-devel > > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi: Initialize canary to non-zero value 2023-11-13 16:18 ` Daniel Kiper 2023-11-14 3:17 ` Glenn Washburn @ 2023-11-18 21:01 ` Glenn Washburn 1 sibling, 0 replies; 7+ messages in thread From: Glenn Washburn @ 2023-11-18 21:01 UTC (permalink / raw) To: Daniel Kiper Cc: Heinrich Schuchardt, Chris Coulson, The development of GNU GRUB On Mon, 13 Nov 2023 17:18:50 +0100 Daniel Kiper <dkiper@net-space.pl> wrote: > On Sun, Nov 12, 2023 at 08:22:42AM +0100, Heinrich Schuchardt wrote: > > On 11/12/23 04:23, Glenn Washburn wrote: > > > The canary, __stack_chk_guard, is in the BSS and so will get initialized to > > > zero if it is not explicitly initialized. If the UEFI firmware does not > > > support the RNG protocol, then the canary will not be randomized and will > > > be used as zero. This seems like a possibly easier value to write by an > > > attacker. Initialize canary to static random bytes, so that it is still > > > random when there is not RNG protocol. > > > > > > Signed-off-by: Glenn Washburn <development@efficientek.com> > > > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> > > > > --- > > > grub-core/kern/efi/init.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c > > > index 0e28bea17a76..b85d98ca47fd 100644 > > > --- a/grub-core/kern/efi/init.c > > > +++ b/grub-core/kern/efi/init.c > > > @@ -41,7 +41,7 @@ static grub_guid_t rng_protocol_guid = GRUB_EFI_RNG_PROTOCOL_GUID; > > > > > > static grub_efi_uint8_t stack_chk_guard_buf[32]; > > > > > > -grub_addr_t __stack_chk_guard; > > > +grub_addr_t __stack_chk_guard = (grub_addr_t) 0x92f2b7e2f193b25c; I've come across more information[1] on this subject that leads me to believe that at least the byte with the lowest memory address should be a NULL byte (eg. grub_cpu_to_be64_compile_time(0x00f2b7e2f193b25c)) to make the common case of string buffer overflows non-bypassable. And in the absence of random bytes at runtime, it might be a good idea to xor with the current time and perhaps the EFI app base load address. So I suggest not merging this yet and I'll have a v2 coming soon. Glenn [1] https://www.sans.org/blog/stack-canaries-gingerly-sidestepping-the-cage/ > > I would add last sentence from the commit message before this line. > I can do it for you before push... > > > > > > > void __attribute__ ((noreturn)) > > > __stack_chk_fail (void) > > Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-11-18 21:02 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-11-12 3:23 [PATCH] efi: Initialize canary to non-zero value Glenn Washburn 2023-11-12 7:22 ` Heinrich Schuchardt 2023-11-13 16:18 ` Daniel Kiper 2023-11-14 3:17 ` Glenn Washburn 2023-11-14 4:05 ` Dimitri John Ledkov 2023-11-18 21:02 ` Glenn Washburn 2023-11-18 21:01 ` Glenn Washburn
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.