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