Kicksecure / security-misc

Kernel Hardening; Protect Linux User Accounts against Brute Force Attacks; Improve Entropy Collection; Strong Linux User Account Separation; Enhances Misc Security Settings - https://www.kicksecure.com/wiki/Security-misc
https://www.kicksecure.com/wiki/Impressum
Other
517 stars 51 forks source link

Refactor `/etc/default/grub.d/*` #233

Closed raja-grewal closed 4 months ago

raja-grewal commented 4 months ago

Refactor existing grub kernel parameters for clarity, ease-of-use, and future-proofing.

Add two (deactivated) kernel parameters to further reduce attack surface.

Changes

Merge 40_distrust_bootloader.cfg, 40_distrust_cpu.cfg, 40_enable_iommu.cfg, and 40_kernel_hardening.cfg into a single organised file 40_kernel_hardening.cfg.

Rename 40_only_allow_signed_modules.cfg to 40_signed_modules.cfg.

Rename 40_remmount-secure.cfg to 40_remount_secure.cfg.

Rename 41_quiet.cfg to 41_quiet_boot.cfg.

There are no changes to the actual functionality of the code.

Mandatory Checklist

Terms of Service, Privacy Policy, Cookie Policy, E-Sign Consent, DMCA, Imprint

Optional Checklist

The following items are optional but might be requested in certain cases.

raja-grewal commented 4 months ago

https://forums.whonix.org/t/kernel-hardening-security-misc/7296/552

adrelanos commented 4 months ago

If renaming or deleting a file under /etc these need to be added to https://github.com/Kicksecure/security-misc/blob/master/debian/security-misc.maintscript - otherwise, these will not be removed for users that upgrade this package.

raja-grewal commented 4 months ago

Fixed, thanks for the quick review.

adrelanos commented 4 months ago

This is a bit difficult to review that only comments were changed and no functionality was changed. The diff looks difficult to review. Found 1 bug above.

cd etc/default/grub.d
grep -r --invert-match '^#\|^$'

(Add -h to hide grep prepending the filename.)

Repeat the same command for the other branch. Then compare the two outputs with a diff viewer and make sure there are only expected changes as in splitting two options into multiple lines.

Could you please recheck using this or similar methodology that nothing was changed?

raja-grewal commented 4 months ago

Fixed the bug.

I agree that this is a lot to review given the quite substantial changes.

It also did not help that git did not properly recognise that several files were renamed and not deleted.

Thanks for the suggestions, I will go through it again to confirm.

adrelanos commented 4 months ago

Also helpful might be to append | sort.

grep -h -r --invert-match '^#\|^$' | sort
adrelanos commented 4 months ago

I am not sure about https://github.com/Kicksecure/security-misc/pull/233/commits/f4d652fa7b5dd350b577521c6bba22c9eb3c13f1.

Some (Debian?) default configuration file adds quiet by default. (Or has this been changed?) If we don't remove that first quiet but only append our own quiet this will result in the kernel command line having quiet twice. Hence the idea of

GRUB_CMDLINE_LINUX_DEFAULT="$(echo "$GRUB_CMDLINE_LINUX_DEFAULT" | LANG=C str_replace "quiet" "")"

was to remove the default quiet and add a new one which then will be easier for the user to locate and disable.

raja-grewal commented 4 months ago

I did not know that was the case with some default configuration files.

Added back in with appropriate reference.

raja-grewal commented 4 months ago

Below is the standard diff after using the sorting command you provided.

Seems alright to me but a reviews by independent eyes would be far more helpful.

[b@y] diff old.txt new.txt
0a1
> GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX amd_iommu=force_isolation"
5,7c6,11
< GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX init_on_alloc=1 init_on_free=1"
< GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX intel_iommu=on amd_iommu=force_isolation"
< GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX iommu=force iommu.passthrough=0 iommu.strict=1"
---
> GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX init_on_alloc=1"
> GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX init_on_free=1"
> GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX intel_iommu=on"
> GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX iommu.passthrough=0"
> GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX iommu.strict=1"
> GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX iommu=force"
24,25c28,31
< GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX spectre_v2=on spectre_bhi=on"
< GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX tsx=off tsx_async_abort=full,nosmt"
---
> GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX spectre_bhi=on"
> GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX spectre_v2=on"
> GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX tsx=off"
> GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX tsx_async_abort=full,nosmt"
28c34,35
< GRUB_CMDLINE_LINUX_DEFAULT="$GRUB_CMDLINE_LINUX_DEFAULT quiet loglevel=0"
---
> GRUB_CMDLINE_LINUX_DEFAULT="$GRUB_CMDLINE_LINUX_DEFAULT loglevel=0"
> GRUB_CMDLINE_LINUX_DEFAULT="$GRUB_CMDLINE_LINUX_DEFAULT quiet"

This is also why I think my proposed refactoring splitting each kernel parameter to its own separate line is a nice idea moving forward since it will make these sorts of reviews far easier.