search

rhboot / shim

UEFI shim loader
Other
848 stars 290 forks source link

Discard load-options that start with a NUL #505

Closed frozencemetery closed 2 years ago

frozencemetery commented 2 years ago

In 6c8d08c0af4768c715b79c8ec25141d56e34f8b4 ("shim: Ignore UEFI LoadOptions that are just NUL characters."), a check was added to discard load options that are entirely NUL. We now see some firmwares that start LoadOptions with a NUL, and then follow it with garbage (path to directory containing loaders). Widen the check to just discard anything that starts with a NUL.

Resolves: #490 Related: #95 See-also: https://bugzilla.redhat.com/show_bug.cgi?id=2113005 Signed-off-by: Robbie Harwood rharwood@redhat.com

julian-klode commented 2 years ago

I thought my fix went in in 15.5 or so to make it ignore load options when running from removable media path so I'm confused how that happens; or confused re having applied the patch

frozencemetery commented 2 years ago

While it's always possible that my ability to read m4 is failing, your changes were gated behind DISABLE_REMOVABLE_LOAD_OPTIONS being defined, and by default it's not.

frozencemetery commented 2 years ago

More generally, that kind of divergence is why I don't like having things gated by options in shim. I think there's a case to be made that the behavior of DISABLE_REMOVABLE_LOAD_OPTIONS should be the default, or at least that DISABLE_REMOVABLE_LOAD_OPTIONS should be default-on, if you'd prefer that.

julian-klode commented 2 years ago

Ah right, sorry. We enabled that in Ubuntu and I feel like that's sensible as it avoids these issues entirely, but @vathpela had some concerns IIRC.

There were a whole bunch more of these issues but we stopped debugging that after disabling it.

dennis-tseng99 commented 2 years ago

It could be straightforward if changing UINT16 to CHAR16 although both are uint16_t : if (((UINT16 *)li->LoadOptions)[0] == 0) --> if (((CHAR16 *)li->LoadOptions)[0] == 0)

frozencemetery commented 2 years ago

That's fine, updated.

frozencemetery commented 2 years ago

Thanks for the testing. For those hitting this issue before their distros update the officially signed shim: in my opinion, it is better from a security perspective to run the older, signed shim rather than disabling secureboot to run the patched shim above from #505.

(If you're able to enroll your own keys, of course you can have the best of both worlds; and if you're just testing fixes, none of this applies anyway.)