[PATCH] efi: Initialize canary to non-zero value

[PATCH] efi: Initialize canary to non-zero value

[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

Re: [PATCH] efi: Initialize canary to non-zero value

[Heinrich Schuchardt]

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

Re: [PATCH] efi: Initialize canary to non-zero value

[Daniel Kiper]

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

Re: [PATCH] efi: Initialize canary to non-zero value

[Glenn Washburn]

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

Re: [PATCH] efi: Initialize canary to non-zero value

[Dimitri John Ledkov]

[-- 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

Re: [PATCH] efi: Initialize canary to non-zero value

[Glenn Washburn]

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

Re: [PATCH] efi: Initialize canary to non-zero value

[Glenn Washburn]

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