-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ghaf security hardening #661
base: main
Are you sure you want to change the base?
Conversation
- Added Apparmor configuration for Chromium and Firefox Signed-off-by: Ganga Ram <[email protected]>
Signed-off-by: Ganga Ram <[email protected]>
- A intrusion prevention software framework - Bans IP addresses conducting too many failed login attempts. Signed-off-by: Ganga Ram <[email protected]>
- Used to sandbox untrusted applications Signed-off-by: Ganga Ram <[email protected]>
Signed-off-by: Ganga Ram <[email protected]>
Signed-off-by: Ganga Ram <[email protected]>
Signed-off-by: Ganga Ram <[email protected]>
executable = "${lib.getBin pkgs.firefox}/bin/firefox"; | ||
profile = "${pkgs.firejail}/etc/firejail/firefox.profile"; | ||
extraArgs = [ | ||
"--whitelist=/run/current-system/sw/bin/firefox" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't both ${lib.getBin pkgs.firefox}/bin/firefox
and /run/current-system/sw/bin/firefox
link to the same binary?
Also on the main branch, firefox
is no longer added in the system path for the Orin, so it isn't found in /run/current-system/sw/bin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/run/current-system/sw/bin/firefox is jailed firefox, wraped by firejail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But /run/current-system/sw/bin/firefox
doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you enable jailed firefox then it exists:
ghaf.security.firejail.enable = true;
ghaf.security.firejail.apps.firefox.enable = true;
It is not recommended to have have Apparmor and Firejail both enabled for same application. For Lenovo X1 we have Apparmor security for Chromium. Jailed firefox and chromium I had tested on vm and generic x86.
config.boot.kernel.sysctl = lib.mkMerge [ | ||
(lib.mkIf cfg.ipsecurity.enable { | ||
# Disable IPv6 | ||
"net.ipv6.conf.all.disable_ipv6" = lib.mkForce 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disabling IPv6 benefit seems to be for making implementing filtering policies easier.
If IPv6 needs to be disabled for filtering, shouldn't that be done on corporate VPN side rather than on the OS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disabled IPv6 to reduce attack surface. I didn't think from VPN perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would IPv6 be an attack surface? Is the kernel code for IPv6 implementation buggy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't know what is buggy, what is not. More code means larger attack surface. Many security audits recommend to disable IPv6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at NIST, they have guidelines on secure deployment of IPv6 with NIST 800-119^1. Also NIST 800-53 did use an example of IPv4 as older technology and IPv6 as newer technology, but I can't seem to find any control or control enhancement that suggests disabling IPv6.
Is there some kind of guideline that I have missed that suggests disabling IPv6?
1: https://csrc.nist.gov/pubs/sp/800/119/final
2: https://csrc.nist.gov/pubs/sp/800/53/r5/upd1/final
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.tenable.com/audits/items/search?q=ipv6&sort=&page=1
https://www.safedns.com/userfiles/uploads/checklist%20linux.pdf
https://www.cyberciti.biz/tips/linux-security.html [section: 23]
For sure if we are going to use IPv6 then we need to enable this.
## Linux security | ||
(lib.mkIf cfg.system-security.enable { | ||
services.openssh = { | ||
extraConfig = lib.optionalString config.ghaf.profiles.release.enable '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If release build doesn't enable openssh service, why are we configuring it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be openssh will be enabled to login in net-vm from different machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a use case documented anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure but this is not going to harm. If openssh is enabled then this will enforce security otherwise it will not have any effect.
Hi! I have a few questions about testing:
|
SCUDO_OPTIONS = lib.mkDefault "ZeroContents=1"; | ||
}; | ||
|
||
boot.tmp.useTmpfs = lib.mkForce true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to use tmpfs in all VMs? Since it is using RAM and that can be scarce.
Maybe boot.tmp.cleanOnBoot
is enough, and we can mount /tmp
with noexec
flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is in our TODO list to mount some directories with appropriate permission. We'll revisit it when we'll do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I'm worried enabling tmpfs would cause the VMs to run out of RAM or /tmp
is filled, causing bad side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would enabling tmpfs cause VMs to run out of RAM? By default, the size of tmpfs in Nix is set to 50% of the available memory. I believe the opposite is true: if tmpfs is not enabled, all temporary storage will still occupy RAM without any limits imposed on it, potentially causing the VMs to run out of RAM. In the first scenario, an application may fail due to limited space in tmpfs, but the VM itself will remain responsive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example. AppFlowy has only 512MB of RAM: https://github.com/tiiuae/ghaf/blob/main/modules/reference/appvms/appflowy.nix#L12
Is 256MB of /tmp
enough? Can AppFlowy run with only 256MB of free RAM? Have we tested this?
a5ffa7b
to
f2162e3
Compare
@milva-unikie, Please see my comments inline:
==> I have done few functionality tests for newly added security features. Please test overall functionality of the system to make sure nothing is broken in this PR. Also do performance testing, as security features may have some performance impact.
==> You need not to do security testing, we are working on security test plan.
==> On other platforms like Orin and Polarfire, please make sure nothing is broken. |
f2162e3
to
a4481e6
Compare
(lib.mkIf cfg.system-security.enable { | ||
services.openssh = { | ||
extraConfig = lib.optionalString config.ghaf.profiles.release.enable '' | ||
AllowTcpForwarding yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed for release build?
services.openssh = { | ||
extraConfig = lib.optionalString config.ghaf.profiles.release.enable '' | ||
AllowTcpForwarding yes | ||
X11Forwarding no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
X11 would not be included in Ghaf's reference implementation, then why are we configuring it?
"kernel.kexec_load_disabled" = lib.mkForce config.ghaf.profiles.release.enable; | ||
|
||
# Disable ptrace | ||
"kernel.yama.ptrace_scope" = lib.mkForce 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default is 1, which is already restrictive. Restricting it further may break sandboxing applications?
https://www.kernel.org/doc/html/v4.15/admin-guide/LSM/Yama.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if we are not breaking any thing, then shouldn't we keep restriction at highest level? We can reduce it to 2 or even 1 if it is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we need to document this for the option system-security.enable
, so whoever uses Ghaf library would know this setting is changed? Also instead of mkForce, does it make sense to use mkDefault or mkOverride (setting a priority)?
modules/common/security/system.nix
Outdated
boot.kernelParams = | ||
[ | ||
# Fill freed pages and heap objects with zeroes | ||
"init_memory=0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to find out what parameter this is, my search engine only returns one result on medium.com. I checked Linux kernel source, also couldn't find anything. Could you please help me find where I can find a reference to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I don't know how I messed this up. I was supposed to use 'init_on_alloc' and 'init_on_free'.
"init_memory=0" | ||
|
||
# Panic on any uncorrectable errors through the machine check exception system | ||
"mce=0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to only be applicable for systems that use ECC memory, none of our reference devices have ECC memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Machine Check Exceptions (MCEs) are not exclusive to ECC memory; they can occur in various hardware scenarios beyond ECC memory alone. For reference platform it should be fine to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, is it only applicable for x86 systems?
https://www.kernel.org/doc/html/latest/admin-guide/kernel-parameters.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DDR5 will have some light form of ECC by default.
] | ||
++ lib.optionals config.ghaf.security.network.ipsecurity.enable [ | ||
# Disable IPv6 to reduce attack surface. | ||
"ipv6.disable=1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the previous comment on disabling IPv6, I don't see why IPv6 is a vulnerability. This is going to be an issue as we slowly move away from IPv4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check if we really need it. I have seen in security audits this is recommended to disable IPv6 to reduce attack vector until unless we are not using this.
modules/common/security/system.nix
Outdated
|
||
# This will avoid unintentional writes to an attacker-controlled FIFO/Regular file. | ||
# Extend the restriction to group sticky directories | ||
"fs.protected_fifos" = lib.mkForce 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't setting this to 0 (unrestricted) be counter-intuitive?
https://github.com/torvalds/linux/blob/2ccbdf43d5e758f8493a95252073cf9078a5fea5/Documentation/admin-guide/sysctl/fs.rst#protected_fifos
}) | ||
]; | ||
|
||
launchers = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the launchers be defined inside of the reference apps module?
launchers = | ||
lib.optional cfg.apps.firefox.enable { | ||
name = "Firefox-safe"; | ||
path = "/run/current-system/sw/bin/firefox"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path doesn't exist on the Orin. Shouldn't this be something like:
${pkgs.firefox}/bin/firefox
Or if we have our own version, we can directly refer to it ${firejailFirefox}/bin/firefox-safe
} | ||
++ lib.optional cfg.apps.chromium.enable { | ||
name = "Chromium-safe"; | ||
path = "/run/current-system/sw/bin/chromium"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with above issue with firefox, we should refer directly to the path of the chromium binary. As this file might not exist.
sudo = { | ||
enable = lib.mkOption { | ||
type = lib.types.bool; | ||
default = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already enabled by default, why is it redefined here?
And do we want sudo for release builds?
type = lib.types.bool; | ||
default = true; | ||
}; | ||
memory-allocator = lib.mkOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option already exists upstream with environment.memoryAllocator.provider
.
If we provide option for alternative allocators, it means we are supporting it. Were these tested? Is there reason behind the omission of graphene-hardened-light
?
security.unprivilegedUsernsClone = lib.mkDefault false; | ||
|
||
# Disable Hyperthreading (To reduce risk of side channel attack) | ||
security.allowSimultaneousMultithreading = lib.mkDefault (!(cfg.system-security.misc.disableHyperthreading || cfg.system-security.misc.enable-all)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we benefit the users of Ghaf framework by providing this alias? And are we supporting this use case?
The end user can simply use security.allowSimultaneousMultithreading
if they really want to do this. Even NixOS option documentation states:
[...] potentially at significant performance cost. [...] This attack vector is unproven.
}; | ||
## Apparmor profile for Chromium | ||
config.security.apparmor.policies."bin.chromium" = lib.mkIf cfg.apps.chromium.enable { | ||
profile = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this compare to: https://gitlab.com/apparmor/apparmor/-/blob/master/profiles/apparmor/profiles/extras/chromium_browser?ref_type=heads
Which is already packaged in NixOS (apparmor-profiles
).
And if we have improvements to the Chromium apparmor policy, should it be upstreamed or included as a patch to apparmor-profiles
package?
config.security.apparmor.policies."bin.firefox" = lib.mkIf cfg.apps.firefox.enable { | ||
profile = | ||
'' | ||
abi <abi/3.0>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like Chromium, apparmor-profiles
package also contains policy for firefox: https://gitlab.com/apparmor/apparmor/-/blob/master/profiles/apparmor/profiles/extras/firefox?ref_type=heads
I think if we package our own version, it should be clear how it is different than the profile that is packaged in NixOS?
|
||
# Always block cloaked URLs, even if URL isn't in database. | ||
# This can lead to false positives. | ||
PhishingAlwaysBlockCloak = "no"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already default set to "no".
HeuristicScanPrecedence = "yes"; | ||
|
||
# Enable the Data Loss Prevention module | ||
StructuredDataDetection = "yes"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.clamav.net/manual/Development/libclamav.html#data-loss-prevention
detect the following credit card issuers: AMEX, VISA, MasterCard, Discover, Diner’s Club, and JCB and U.S. social security numbers inside text files.
How would this be beneficial to the end user? Would false positives mean that files that contain that pattern be quarantined? How would the user know?
StructuredDataDetection = "yes"; | ||
|
||
# Stop daemon when libclamav reports out of memory condition. | ||
ExitOnOOM = "yes"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be the job of systemd-oomd
to determine whether it reaches out of memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It does stop the whole service in this case.
|
||
# With this option clamav will try to detect broken executables (both PE and | ||
# ELF) and mark them as Broken.Executable. | ||
DetectBrokenExecutables = "yes"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user-writable directories are all mounted with noexec
, this would be redundant?
}; | ||
updater = lib.mkIf cfg.live-update { | ||
# Enable live update of virus database | ||
enable = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would this behave on a system with impermanence? What if the "storage VM" have no internet access?
|
||
## Enable fail2ban sandboxing | ||
config = { | ||
services.fail2ban = lib.mkIf cfg.enable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need fail2ban for debug builds, especially with default password as ghaf
?
If we disable password authentication, wouldn't that be enough?
Also why are we enabling fail2ban for release build, which has ssh disabled?
- use icons for jailed firefox and chromium from icon pack - users options set to default in all VMs - KASLR option removed as it is by default enabled in kernel - sysrq disabled only in release build - fail2ban enabled in net-vm only Signed-off-by: Ganga Ram <[email protected]>
a4481e6
to
74afc89
Compare
I propose we make this a series of PRs, with individual purposes. That makes it easier
For system, kernel parameters/sysctl settings, let's re-visit when we have the performance |
## Enable Firejail sandboxing | ||
config = lib.mkIf cfg.enable { | ||
ghaf.graphics = lib.mkIf config.ghaf.profiles.graphics.enable { | ||
demo-apps = lib.mkMerge [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the NixOS ecosystem uses camel case for options i.e.:
demo-apps = lib.mkMerge [ | |
demoApps = lib.mkMerge [ |
It looks like ghaf is doing the same in most places.
|
||
config = lib.mkIf cfg.enable { | ||
security.apparmor.enable = true; | ||
security.apparmor.killUnconfinedConfinables = lib.mkDefault true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was always a bit skeptical about this option when it was added to NixOS. Does firefox/chromium properly restarts itself afterwards?
Hardening of Ghaf security
Checklist for things done
x86_64
aarch64
riscv64
nix flake check --accept-flake-config
and it passesTesting