* [PATCH v2 0/3] efi: Initialize canary to non-zero value
@ 2023-12-11 19:27 Glenn Washburn
2023-12-11 19:27 ` [PATCH v2 1/3] " Glenn Washburn
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Glenn Washburn @ 2023-12-11 19:27 UTC (permalink / raw)
To: The development of GNU GRUB, Daniel Kiper
Cc: Heinrich Schuchardt, Dimitri John Ledkov, Glenn Washburn
This series extends and improves the previous patch initializing the
stack guard canary. The first patch improves the previous patch by
setting the most significant byte to NULL, which will filter out
string buffer overflow attacks. The second patch allows creation of
the canary at build time from urandom if it exists. This change breaks
reproducible builds, so the third patch allows the canary to be set
from the environment variable SOURCE_DATE_EPOCH if its value is not
empty.
Glenn
Glenn Washburn (3):
efi: Initialize canary to non-zero value
efi: Generate stack protector canary at build time if urandom is
available
efi: Add support for reproducible builds
config.h.in | 2 ++
configure.ac | 22 ++++++++++++++++++++++
grub-core/kern/efi/init.c | 3 ++-
3 files changed, 26 insertions(+), 1 deletion(-)
--
2.34.1
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2 1/3] efi: Initialize canary to non-zero value 2023-12-11 19:27 [PATCH v2 0/3] efi: Initialize canary to non-zero value Glenn Washburn @ 2023-12-11 19:27 ` Glenn Washburn 2023-12-13 20:24 ` Daniel Kiper 2023-12-11 19:27 ` [PATCH v2 2/3] efi: Generate stack protector canary at build time if urandom is available Glenn Washburn 2023-12-11 19:27 ` [PATCH v2 3/3] efi: Add support for reproducible builds Glenn Washburn 2 siblings, 1 reply; 6+ messages in thread From: Glenn Washburn @ 2023-12-11 19:27 UTC (permalink / raw) To: The development of GNU GRUB, Daniel Kiper Cc: Heinrich Schuchardt, Dimitri John Ledkov, 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 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 string buffer overflow attacks. Signed-off-by: Glenn Washburn <development@efficientek.com> --- grub-core/kern/efi/init.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c index e759cc315b85..08e24d46fad9 100644 --- a/grub-core/kern/efi/init.c +++ b/grub-core/kern/efi/init.c @@ -45,7 +45,8 @@ 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; +/* Initialize canary in case there is no RNG protocol. */ +grub_addr_t __stack_chk_guard = (grub_addr_t) 0x00f2b7e2f193b25c; 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] 6+ messages in thread
* Re: [PATCH v2 1/3] efi: Initialize canary to non-zero value 2023-12-11 19:27 ` [PATCH v2 1/3] " Glenn Washburn @ 2023-12-13 20:24 ` Daniel Kiper 2023-12-19 6:14 ` Glenn Washburn 0 siblings, 1 reply; 6+ messages in thread From: Daniel Kiper @ 2023-12-13 20:24 UTC (permalink / raw) To: Glenn Washburn Cc: The development of GNU GRUB, Heinrich Schuchardt, Dimitri John Ledkov 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. > 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. Additionally, to have consistent behavior over the code I would zero out highest order byte when they come from RNG too. ... and it seems to me this will not work for big endian CPUs. grub_be_to_cpu64_compile_time()? 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. 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. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/3] efi: Initialize canary to non-zero value 2023-12-13 20:24 ` Daniel Kiper @ 2023-12-19 6:14 ` Glenn Washburn 0 siblings, 0 replies; 6+ messages in thread From: Glenn Washburn @ 2023-12-19 6:14 UTC (permalink / raw) To: Daniel Kiper Cc: The development of GNU GRUB, Heinrich Schuchardt, Dimitri John Ledkov 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/3] efi: Generate stack protector canary at build time if urandom is available 2023-12-11 19:27 [PATCH v2 0/3] efi: Initialize canary to non-zero value Glenn Washburn 2023-12-11 19:27 ` [PATCH v2 1/3] " Glenn Washburn @ 2023-12-11 19:27 ` Glenn Washburn 2023-12-11 19:27 ` [PATCH v2 3/3] efi: Add support for reproducible builds Glenn Washburn 2 siblings, 0 replies; 6+ messages in thread From: Glenn Washburn @ 2023-12-11 19:27 UTC (permalink / raw) To: The development of GNU GRUB, Daniel Kiper Cc: Heinrich Schuchardt, Dimitri John Ledkov, Glenn Washburn Generating the canary at build time allows the canary to be different for every build which could limit the effectiveness of certain exploits. Fallback to the statically generated random bytes if /dev/urandom is not readable (eg. Windows). Reduce the canary to 3 bytes with a NULL upper byte on 32-bit architectures, which use a 32-bit canary, to filter out string buffer overflow attacks. Signed-off-by: Glenn Washburn <development@efficientek.com> --- config.h.in | 2 ++ configure.ac | 20 ++++++++++++++++++++ grub-core/kern/efi/init.c | 2 +- 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/config.h.in b/config.h.in index 4d1e50eba79c..9b1d39971858 100644 --- a/config.h.in +++ b/config.h.in @@ -64,6 +64,8 @@ # define GRUB_TARGET_CPU "@GRUB_TARGET_CPU@" # define GRUB_PLATFORM "@GRUB_PLATFORM@" +# define GRUB_STACK_PROTECTOR_INIT @GRUB_STACK_PROTECTOR_INIT@ + # define RE_ENABLE_I18N 1 # define _GNU_SOURCE 1 diff --git a/configure.ac b/configure.ac index c19779c14d08..788f5f3206f1 100644 --- a/configure.ac +++ b/configure.ac @@ -1438,6 +1438,26 @@ else AC_MSG_ERROR([invalid value $enable_stack_protector for --enable-stack-protector]) fi TARGET_CPPFLAGS="$TARGET_CPPFLAGS -DGRUB_STACK_PROTECTOR=1" + + if test -r /dev/urandom; then + # Generate the 8 byte stack protector canary at build time if /dev/urandom + # is able to be read. The first byte should be NULL to filter out string + # buffer overflow attacks. + GRUB_STACK_PROTECTOR_INIT="$($PYTHON -c 'import codecs; rf=open("/dev/urandom", "rb"); print("0x00"+codecs.encode(rf.read(7), "hex").decode("ascii"))')" + else + # Some hosts may not have a urandom (eg. Windows), so use statically + # generated random bytes + GRUB_STACK_PROTECTOR_INIT="0x00f2b7e2f193b25c" + fi + + if test x"$target_m32" = x1 ; then + # Make sure that the canary default value is 24-bits by only using the + # lower 3 bytes on 32 bit systems. This allows the upper byte to be NULL + # to filter out string buffer overflow attacks. + GRUB_STACK_PROTECTOR_INIT="0x00$(echo "$GRUB_STACK_PROTECTOR_INIT" | sed 's/.*\(......\)$/\1/')" + fi + + AC_SUBST([GRUB_STACK_PROTECTOR_INIT]) fi CFLAGS="$TARGET_CFLAGS" diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c index 08e24d46fad9..6c54af6e79e5 100644 --- a/grub-core/kern/efi/init.c +++ b/grub-core/kern/efi/init.c @@ -46,7 +46,7 @@ static grub_guid_t rng_protocol_guid = GRUB_EFI_RNG_PROTOCOL_GUID; static grub_efi_uint8_t stack_chk_guard_buf[32]; /* Initialize canary in case there is no RNG protocol. */ -grub_addr_t __stack_chk_guard = (grub_addr_t) 0x00f2b7e2f193b25c; +grub_addr_t __stack_chk_guard = (grub_addr_t) GRUB_STACK_PROTECTOR_INIT; 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] 6+ messages in thread
* [PATCH v2 3/3] efi: Add support for reproducible builds 2023-12-11 19:27 [PATCH v2 0/3] efi: Initialize canary to non-zero value Glenn Washburn 2023-12-11 19:27 ` [PATCH v2 1/3] " Glenn Washburn 2023-12-11 19:27 ` [PATCH v2 2/3] efi: Generate stack protector canary at build time if urandom is available Glenn Washburn @ 2023-12-11 19:27 ` Glenn Washburn 2 siblings, 0 replies; 6+ messages in thread From: Glenn Washburn @ 2023-12-11 19:27 UTC (permalink / raw) To: The development of GNU GRUB, Daniel Kiper Cc: Heinrich Schuchardt, Dimitri John Ledkov, Glenn Washburn Having randomly generated bytes in the binary output breaks reproducible builds. Since build timestamps are usually the source of irreproducibility there is a standard which defines an environment variable SOURCE_DATE_EPOCH to be used when set for build timestamps. According to the standard[1], the value of SOURCE_DATE_EPOCH is a base-10 integer of the number of seconds since the UNIX epoch. Currently, this is a 10 digit number that fits into 32-bits, but will not shortly after the year 2100. So to be future-proof only use the least significant 32-bits. On 64-bit architectures, where the canary is also 64-bits, there is an extra 32-bits that can be filled to provide more entropy. The first byte is null to filter out string buffer overflow attacks and the remaining 24-bits are set to static random bytes. [1] https://reproducible-builds.org/specs/source-date-epoch Signed-off-by: Glenn Washburn <development@efficientek.com> --- configure.ac | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 788f5f3206f1..3d326b8725f2 100644 --- a/configure.ac +++ b/configure.ac @@ -1439,7 +1439,9 @@ else fi TARGET_CPPFLAGS="$TARGET_CPPFLAGS -DGRUB_STACK_PROTECTOR=1" - if test -r /dev/urandom; then + if test -n "$SOURCE_DATE_EPOCH"; then + GRUB_STACK_PROTECTOR_INIT="0x00f2b7e2$(printf "%x" "$SOURCE_DATE_EPOCH" | sed 's/.*\(........\)$/\1/')" + elif test -r /dev/urandom; then # Generate the 8 byte stack protector canary at build time if /dev/urandom # is able to be read. The first byte should be NULL to filter out string # buffer overflow attacks. -- 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] 6+ messages in thread
end of thread, other threads:[~2023-12-19 6:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-12-11 19:27 [PATCH v2 0/3] efi: Initialize canary to non-zero value Glenn Washburn 2023-12-11 19:27 ` [PATCH v2 1/3] " Glenn Washburn 2023-12-13 20:24 ` Daniel Kiper 2023-12-19 6:14 ` Glenn Washburn 2023-12-11 19:27 ` [PATCH v2 2/3] efi: Generate stack protector canary at build time if urandom is available Glenn Washburn 2023-12-11 19:27 ` [PATCH v2 3/3] efi: Add support for reproducible builds 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.