fix: restrict EFI partition permissions with fmask/dmask=0077#4506
fix: restrict EFI partition permissions with fmask/dmask=0077#45060xdeadd wants to merge 2 commits into
Conversation
Mount the ESP with fmask=0077 and dmask=0077 to prevent world-readable files like /efi/loader/random-seed. Closes archlinux#4241
|
Have you tried the patch ? @0xdeadd Does |
|
Yep, tested it. Mount mechanics + genfstab (FAT32 loop image in a container, same image mounted both ways):
Unit suite passes on the branch ( On the pacstrap question: I haven't done a full bare-metal / QEMU install yet, so I can't speak to that first-hand. But One thing I noticed while testing: the guard is an exact-string check ( |
|
Thanks for the details, I already had patched this in my fork and I'm guessing I had made a mistake originally:
About a QEMU/VM test is always worth it just for sanity check and check end result / expected outcome(s) in target. |
|
QEMU install run complete. UEFI + GPT, ESP at /boot, systemd-boot, archinstall 4.3 with the PR's
One thing worth flagging though: your memory of the The If you want to suppress it that'd be a separate change (and it'd be cosmetic — chmod doesn't stick on vfat anyway, so the warning is honest about a real-but-harmless mismatch). My read is: leave it, the warning is accurate and the security win is worth it. |
|
I know it's a cheap "out" to comment this, but would it be worth additionally to this — supply a patch to |
|
|
||
| if part_mod.is_efi(): | ||
| for opt in ('fmask=0077', 'dmask=0077'): | ||
| if opt not in options: |
There was a problem hiding this comment.
Coding on my phone here so apologies if syntax is borked.. but could we not do something like:
list(set(part_mod.mount_options + ('fmask=0077', 'dmask=0077')))To avoid the loop and if checks. If all we want to do is make we don't add duplicate entries?
There was a problem hiding this comment.
Pushed 6ed42e3 with this idea. Used dict.fromkeys instead of set so insertion order is preserved (sets have implementation-defined iteration order, which would make the resulting /etc/fstab entry order undefined). Same dedup semantics as the loop for the dup-string case.
if part_mod.is_efi():
options = list(dict.fromkeys(options + ['fmask=0077', 'dmask=0077']))Note it still doesn't dedup by key, so a pre-existing fmask=0022 in mount_options would coexist with the appended fmask=0077 (vfat picks the last one, so the result is still restrictive). I think that's fine for this PR but happy to layer on a parse-by-key version if you'd prefer.
Per @Torxed's review feedback. Same semantics as the previous loop (dedupe by exact-string match) but shorter. dict.fromkeys preserves insertion order, where set() would not.
|
Worth doing, I think. Happy to open an MR against the |
| if part_mod.mountpoint: | ||
| target = self.target / part_mod.relative_mountpoint | ||
| mount(part_mod.dev_path, target, options=part_mod.mount_options) | ||
| options = list(part_mod.mount_options) |
There was a problem hiding this comment.
mount_options is already a list?
| options = list(part_mod.mount_options) | ||
|
|
||
| if part_mod.is_efi(): | ||
| options = list(dict.fromkeys(options + ['fmask=0077', 'dmask=0077'])) |
There was a problem hiding this comment.
change to options += ['fmask=0077', 'dmask=0077']
Summary
fmask=0077anddmask=0077to prevent world-readable files like/efi/loader/random-seed_mount_partition()and carried into the installed system viagenfstabCloses #4241
Test plan
/efi/loader/random-seedis not world-readablefstabentry for ESP includesfmask=0077,dmask=0077