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

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

[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

[PATCH v2 1/3] 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 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

[PATCH v2 2/3] efi: Generate stack protector canary at build time if urandom is available

[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

[PATCH v2 3/3] efi: Add support for reproducible builds

[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

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

[Daniel Kiper]

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

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

[Glenn Washburn]

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