On Wed, 13 Dec 2023 21:24:07 +0100 Daniel Kiper <dki...@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
