[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] efi: Initialize canary to non-zero value
From: |
Glenn Washburn |
Subject: |
Re: [PATCH] efi: Initialize canary to non-zero value |
Date: |
Mon, 13 Nov 2023 21:17:48 -0600 |
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