Re: [PATCH v2 1/3] efi: Initialize canary to non-zero value

From: Glenn Washburn <development@efficientek.com>
To: Daniel Kiper <dkiper@net-space.pl>
Cc: The development of GNU GRUB <grub-devel@gnu.org>,
	Heinrich Schuchardt <xypron.glpk@gmx.de>,
	Dimitri John Ledkov <dimitri.ledkov@canonical.com>
Subject: Re: [PATCH v2 1/3] efi: Initialize canary to non-zero value
Date: Tue, 19 Dec 2023 00:14:37 -0600	[thread overview]
Message-ID: <20231219001437.265b4b6f@crass-HP-ZBook-15-G2> (raw)
In-Reply-To: <20231213202407.47iwp24tujvyfe5c@tomti.i.net-space.pl>

On Wed, 13 Dec 2023 21:24:07 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Mon, Dec 11, 2023 at 01:27:48PM -0600, 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 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 no RNG protocol. Set at least one byte to NULL to protect against
> 
> s/NULL/NUL/? If yes then please fix other places too.

I've always written it as NULL which corresponds to the C macro, so it
was not a typo. I don't really care though, so I'll make the change.

> > string buffer overflow attacks.
> 
> I think I can imagine how it works but instead of guessing I would
> prefer to have this written down in the commit message.

I'll make some additions.

> Additionally, to have consistent behavior over the code I would zero out
> highest order byte when they come from RNG too.

I wasn't sure this was desirable for run-time random canaries. Adding a
NULL byte decreases entropy of the canary while making canary forgery
impossible in code that terminates on NULL. I'm thinking now that maybe
it is desirable. An article from SANS[1] suggests indirectly that it
should be used, especially if guessing the canary byte by byte can not
be done (which seems to be true for the run-time random canary, but
not the build-time canary).

Another thing I'm just now thinking of is that a single NULL will not
terminate UTF-16 strings, so we don't get protection of UTF-16 string
operations. I haven't audited the code to determine if that might be a
likely or common attack vector, nor have I seen this referenced in the
literature, but it seems like it might be considering UEFI uses UTF-16
everywhere. Adding two NULL bytes might still provide enough entropy to
make guessing the canary unlikely for 64-bit systems, but severely
decreases the entropy on 32-bit systems. Maybe we really shouldn't care
so much about 32-bit systems.

> ... and it seems to me this will not work for big endian CPUs.
> grub_be_to_cpu64_compile_time()?

So I had mentioned previously about using
grub_cpu_to_be64_compile_time() previously to have the NULL byte be in
the lowest byte address of the in memory byte string representation of
the canary. I had implemented this, but then realized that it does not
work for GRUB's arm64 architecture. Arm64 uses BE-32[3] endian, which
is word invariant, causing the upper and lower words to be swapped, but
not the bytes within the words. So the in memory representation did not
have the lowest byte address as NULL. This would require special
macros, which I didn't care to add. Also, the macro will not compile on
64-bit architectures to initialize global variables (why is the 64-bit
version not written like the 32 and 16-bit versions?). Its pretty
trivial to fix the macro, but I didn't think that kind of change would
be acceptable for the release.

Furthermore, I now don't think it really matters which byte is NULL. If
the attacker is exploiting a buffer overflow in a string operation that
terminates on NULL, then a canary forgery is impossible no matter which
byte of the canary is NULL.

> Last but not least, I think it would be nice to have this feature
> available on non-EFI platforms too. It would help us faster detect
> various overwrites in the code which may slip through cracks.

This seems like a good idea.

Another idea, that's just come to me but I haven't seen discussed
anywhere (probably because not much is focused on stack protection
outside of the operating system) is falling back to a canary value
based on a clock value if the RNG is not available. This would seem
better than a build-time canary because it would be more difficult for
the attacker to guess.

> Anyway, I would want to have this patch set in the release. So, please
> address first two comments ASAP (if nothing blows up again I want to
> cut the release at the begging of next week). The other two things can
> be addressed after the release.

I count 5 distinct things.

 * s/NULL/NUL/
 * More explanation in commit message
 * Add NUL byte to RNG created canary
 * use grub_be_to_cpu64_compile_time()
 * Add stack protection on non-EFI platforms

Glenn

> 
> Daniel

[1]
https://www.sans.org/blog/stack-canaries-gingerly-sidestepping-the-cage/
[2] https://s3.eurecom.fr/docs/ifip18_bierbaumer.pdf#page=3
[3] https://stackoverflow.com/a/4287792

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel