Skip to content
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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

gangaram-tii
Copy link
Contributor

Hardening of Ghaf security

  • Security modules
  • Integration of third party security modules (e.g. FireJail, ClamAV, Fail2Ban etc)
  • Options to enhance kernel security
  • Enabled basic security in VMs

Checklist for things done

  • Summary of the proposed changes in the PR description
  • More detailed description in the commit message(s)
  • Commits are squashed into relevant entities - avoid a lot of minimal dev time commits in the PR
  • Contribution guidelines followed
  • Ghaf documentation updated with the commit - https://tiiuae.github.io/ghaf/
  • PR linked to architecture documentation and requirement(s) (ticket id)
  • Test procedure described (or includes tests). Select one or more:
    • Tested on Lenovo X1 x86_64
    • Tested on Jetson Orin NX or AGX aarch64
    • Tested on Polarfire riscv64
  • Author has run nix flake check --accept-flake-config and it passes
  • All automatic Github Action checks pass - see actions
  • Author has added reviewers and removed PR draft status

Testing

  • Tested basic system and security functionality on Lenovo X1.
  • What to test?
    • Functional testing on all platforms
    • Impact on overall system performance

- Added Apparmor configuration for Chromium and Firefox

Signed-off-by: Ganga Ram <Ganga.Ram@tii.ae>
Signed-off-by: Ganga Ram <Ganga.Ram@tii.ae>
- A intrusion prevention software framework
- Bans IP addresses conducting too many failed login attempts.

Signed-off-by: Ganga Ram <Ganga.Ram@tii.ae>
- Used to sandbox untrusted applications

Signed-off-by: Ganga Ram <Ganga.Ram@tii.ae>
Signed-off-by: Ganga Ram <Ganga.Ram@tii.ae>
Signed-off-by: Ganga Ram <Ganga.Ram@tii.ae>
Signed-off-by: Ganga Ram <Ganga.Ram@tii.ae>
modules/common/security/firejail/default.nix Show resolved Hide resolved
executable = "${lib.getBin pkgs.firefox}/bin/firefox";
profile = "${pkgs.firejail}/etc/firejail/firefox.profile";
extraArgs = [
"--whitelist=/run/current-system/sw/bin/firefox"
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

@gangaram-tii gangaram-tii Jun 14, 2024

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;
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

@gangaram-tii gangaram-tii Jun 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modules/common/security/system.nix Outdated Show resolved Hide resolved
modules/common/security/system.nix Outdated Show resolved Hide resolved
modules/microvm/virtualization/microvm/adminvm.nix Outdated Show resolved Hide resolved
modules/common/security/system.nix Outdated Show resolved Hide resolved
## Linux security
(lib.mkIf cfg.system-security.enable {
services.openssh = {
extraConfig = lib.optionalString config.ghaf.profiles.release.enable ''
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

@milva-unikie
Copy link

Hi! I have a few questions about testing:

  1. Do some of the added security modules have concrete impact on the system that we should verify by testing? Could you please add instructions to the Testing-section.
  2. There are "Options to enhance kernel security" added. Should those be tested? If yes, could you also add how to enable them and their expected results to the Testing-section.
  3. Testing was also asked for Orin and Polarfire. Are there any changes to be expected on these platforms, or just to make sure nothing was broken?

SCUDO_OPTIONS = lib.mkDefault "ZeroContents=1";
};

boot.tmp.useTmpfs = lib.mkForce true;
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

@gangaram-tii
Copy link
Contributor Author

@milva-unikie, Please see my comments inline:

Hi! I have a few questions about testing:

1. Do some of the added security modules have concrete impact on the system that we should verify by testing? Could you please add instructions to the Testing-section.

==> 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.

2. There are "Options to enhance kernel security" added. Should those be tested? If yes, could you also add how to enable them and their expected results to the Testing-section.

==> You need not to do security testing, we are working on security test plan.

3. Testing was also asked for Orin and Polarfire. Are there any changes to be expected on these platforms, or just to make sure nothing was broken?

==> On other platforms like Orin and Polarfire, please make sure nothing is broken.

(lib.mkIf cfg.system-security.enable {
services.openssh = {
extraConfig = lib.optionalString config.ghaf.profiles.release.enable ''
AllowTcpForwarding yes
Copy link
Member

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
Copy link
Member

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;
Copy link
Member

@humaidq-tii humaidq-tii Jun 13, 2024

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

Copy link
Contributor Author

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.

Copy link
Member

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)?

boot.kernelParams =
[
# Fill freed pages and heap objects with zeroes
"init_memory=0"
Copy link
Member

@humaidq-tii humaidq-tii Jun 13, 2024

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?

Copy link
Contributor Author

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"
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

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"
Copy link
Member

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.

Copy link
Contributor Author

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.


# 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

})
];

launchers =
Copy link
Member

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";
Copy link
Member

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";
Copy link
Member

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;
Copy link
Member

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 {
Copy link
Member

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));
Copy link
Member

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 =
Copy link
Member

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>,
Copy link
Member

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";
Copy link
Member

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";
Copy link
Member

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";
Copy link
Member

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?

Copy link
Collaborator

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";
Copy link
Member

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;
Copy link
Member

@humaidq-tii humaidq-tii Jun 13, 2024

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 {
Copy link
Member

@humaidq-tii humaidq-tii Jun 13, 2024

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 <Ganga.Ram@tii.ae>
@mbssrc
Copy link
Collaborator

mbssrc commented Jun 14, 2024

I propose we make this a series of PRs, with individual purposes. That makes it easier
to discuss and refine. Perhaps we could start with:

  1. Apparmor: Profiles require testing for performance (and security)
  2. Fail2ban: At least some configurations will have sshd enabled, for administrator access.
    Can be enabled in release only, if sshd is enabled.
  3. Firejail: No a big fan but can be a reference service option. Opinions welcome.
  4. User configuration (basic)
    As the user setup and configuration is still work in progress, we can use the
    basic settings that will likely be required across all systems, e.g., root user configuration.
    We can also use the work that has been done already (users: creating login and system users #459).

For system, kernel parameters/sysctl settings, let's re-visit when we have the performance
data and security analysis ready, as well as the kernel parameter management to avoid double work.

## Enable Firejail sandboxing
config = lib.mkIf cfg.enable {
ghaf.graphics = lib.mkIf config.ghaf.profiles.graphics.enable {
demo-apps = lib.mkMerge [
Copy link
Collaborator

@Mic92 Mic92 Jun 24, 2024

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.:

Suggested change
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;
Copy link
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants