fix: restrict EFI partition permissions with fmask/dmask=0077#4506
fix: restrict EFI partition permissions with fmask/dmask=0077#45060xdeadd wants to merge 3 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?
There was a problem hiding this comment.
Fair — it's a defensive copy to avoid mutating the caller's list, but since options is reassigned on the next line rather than mutated in place, the copy isn't doing anything load-bearing. Will drop the wrap.
| 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']
There was a problem hiding this comment.
The += form works, but it drops the dedup @Torxed asked for in his earlier comment. Two cases that motivated keeping it:
- If a disk config already sets
fmaskon the ESP (e.g. a profile withfmask=0022),+=producesfmask=0022,fmask=0077in/etc/fstab. vfat picks the last one so the result is still restrictive — but the line is untidy. - On a re-install where
fmask=0077is already inmount_options,+=writes it twice.
dict.fromkeys handles both cleanly while preserving insertion order. If the read is that both edge cases are too unlikely to be worth the extra logic, I'm fine simplifying. @Torxed, your call?
The list() copy on line 378 was load-bearing only if options were mutated downstream, but the EFI branch reassigns options via dict.fromkeys() (line 381) and the non-EFI branch passes through to mount() without mutating. Drop the copy.
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