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

nixos/nginx: use sandboxing mode #85567

Merged
merged 6 commits into from May 13, 2020
Merged

nixos/nginx: use sandboxing mode #85567

merged 6 commits into from May 13, 2020

Conversation

@Izorkin
Copy link
Contributor

Izorkin commented Apr 19, 2020

Motivation for this change

PR based on #60646
Enable run nginx web service in sandboxing mode.

cc @dasJ @flokli @aanderse @Mic92

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@dasJ
Copy link
Member

dasJ commented Apr 19, 2020

A fully up-to-date list of hardening options our sandbox module applies:

{
            # Filesystem stuff
            ProtectSystem = "strict"; # Prevent writing to most of /
            ProtectHome = true; # Prevent accessing /home and /root
            PrivateTmp = true; # Give an own directory under /tmp
            PrivateDevices = true; # Deny access to most of /dev
            ProtectKernelTunables = true; # Protect some parts of /sys
            ProtectControlGroups = true; # Remount cgroups read-only
            RestrictSUIDSGID = true; # Prevent creating SETUID/SETGID files
            PrivateMounts = true; # Give an own mount namespace

            # Capabilities
            CapabilityBoundingSet = ""; # Allow no capabilities at all
            NoNewPrivileges = true; # Disallow getting more capabilities. This is also implied by other options.

            # Kernel stuff
            ProtectKernelModules = true; # Prevent loading of kernel modules
            SystemCallArchitectures = "native"; # Usually no need to disable this
            ProtectKernelLogs = true; # Prevent access to kernel logs
            ProtectClock = true; # Prevent setting the RTC

            # Networking
            RestrictAddressFamilies = ""; # Example: "AF_UNIX AF_INET AF_INET6"
            PrivateNetwork = true; # Isolate the entire network

            # Misc
            LockPersonality = true; # Prevent change of the personality
            ProtectHostname = true; # Give an own UTS namespace
            RestrictRealtime = true; # Prevent switching to RT scheduling
            MemoryDenyWriteExecute = true; # Maybe disable this for interpreters like python
            PrivateUsers = true; # If anything randomly breaks, it's mostly because of this
}

I understand some of these are not useful at all (like PrivateNetwork), but maybe you find some of these useful.

Also, ProtectClock and ProtectKernelLogs are not yet supported by the NixOS systemd.

@Izorkin
Copy link
Contributor Author

Izorkin commented Apr 19, 2020

With PrivateUsers = true; error:

nginx.service: Failed to set up user namespacing: No space left on device
nginx.service: Failed at step USER spawning /nix/store/snsli9q204d8xhiwx8vr5faq5snd153c-unit-script-nginx-pre-start: No space left on device

With ProtectClock = true; and ProtectKernelLogs = true; warning:

systemd[1]: /nix/store/bsjqsa1jpy2kgxr5h0y177hcn7wpv9df-unit-nginx.service/nginx.service:26: Unknown key name 'ProtectClock' in section 'Service', ignoring.
systemd[1]: /nix/store/bsjqsa1jpy2kgxr5h0y177hcn7wpv9df-unit-nginx.service/nginx.service:30: Unknown key name 'ProtectKernelLogs' in section 'Service', ignoring.

With CapabilityBoundingSet = "";

nginx.service: Failed to apply ambient capabilities (before UID change): Operation not permitted
nginx.service: Failed at step CAPABILITIES spawning /nix/store/snsli9q204d8xhiwx8vr5faq5snd153c-unit-script-nginx-pre-start: Operation not permitted
@dasJ
Copy link
Member

dasJ commented Apr 19, 2020

Our nginx module uses these for the capabilities:

      CapabilityBoundingSet = [ "CAP_NET_BIND_SERVICE" "CAP_SYS_RESOURCE" ];
      AmbientCapabilities = [ "CAP_NET_BIND_SERVICE" "CAP_SYS_RESOURCE" ];
@Izorkin Izorkin force-pushed the Izorkin:nginx-sandbox branch from 995c1e8 to 8a0a2e6 Apr 19, 2020
@Izorkin
Copy link
Contributor Author

Izorkin commented Apr 19, 2020

Updated.

@dasJ
Copy link
Member

dasJ commented Apr 19, 2020

Are you sure it needs AF_NETLINK? It's weird because it's lacking from our module, but is then allowed in AppArmor later on 🤔 @ajs124 any idea?

@ajs124
Copy link
Member

ajs124 commented Apr 19, 2020

I have no recollection of anything related to this, so you might as well dig through the code yourself @dasJ.

@dasJ
Copy link
Member

dasJ commented Apr 19, 2020

Also it's worth mentioning in the changelog that MemoryDenyWriteExecute will probably break the nginx lua module 😩

@Izorkin
Copy link
Contributor Author

Izorkin commented Apr 19, 2020

Remove this option

Also it's worth mentioning in the changelog that MemoryDenyWriteExecute will probably break the nginx lua module 😩

Remove the option MemoryDenyWriteExecute?

@Izorkin Izorkin force-pushed the Izorkin:nginx-sandbox branch from 8a0a2e6 to 0cd0b4a Apr 19, 2020
@dasJ
Copy link
Member

dasJ commented Apr 19, 2020

It's a useful way of mitigating more severe problems when an attacker has already gained some access, so I'd keep it but people using the lua module should know that it breaks the module.

@Izorkin
Copy link
Contributor Author

Izorkin commented Apr 19, 2020

Add this wariant?
MemoryDenyWriteExecute = mkDefault true;

@dasJ
Copy link
Member

dasJ commented Apr 19, 2020

@Izorkin
Copy link
Contributor Author

Izorkin commented Apr 19, 2020

With MemoryDenyWriteExecute = true; nginx not worked, time-out open test page.

@Izorkin
Copy link
Contributor Author

Izorkin commented Apr 19, 2020

Not worked with pkgs.nginxModules.pagespeed

@Izorkin
Copy link
Contributor Author

Izorkin commented Apr 19, 2020

@dasJ need backport to 20-03?

@Izorkin Izorkin force-pushed the Izorkin:nginx-sandbox branch 2 times, most recently from 6a4b65f to 47f89ba Apr 19, 2020
@flokli
Copy link
Contributor

flokli commented Apr 19, 2020

This can't be backported to 20.03, which is almost released already.

I'm also a bit afraid about the things some of these options might break - do any other distros already have a nginx unit with some sandboxing options we could take some inspiration from?

@Ma27
Copy link
Member

Ma27 commented Apr 20, 2020

@GrahamcOfBorg build nginx
@GrahamcOfBorg test nginx

@Ma27 Ma27 requested review from globin and fpletz Apr 20, 2020
nixos/doc/manual/release-notes/rl-2009.xml Outdated Show resolved Hide resolved
nixos/doc/manual/release-notes/rl-2009.xml Outdated Show resolved Hide resolved
CapabilityBoundingSet = [ "CAP_NET_BIND_SERVICE" "CAP_SYS_RESOURCE" ];
# Security
NoNewPrivileges = true;
# Sandboxing
ProtectSystem = "strict";
ProtectHome = true;
PrivateTmp = true;
PrivateDevices = true;
ProtectHostname = true;
ProtectKernelTunables = true;
ProtectKernelModules = true;
ProtectControlGroups = true;
RestrictAddressFamilies = [ "AF_UNIX" "AF_INET" "AF_INET6" ];
LockPersonality = true;
MemoryDenyWriteExecute = mkDefault true;
RestrictRealtime = true;
RestrictSUIDSGID = true;
PrivateMounts = true;
# System Call Filtering
SystemCallArchitectures = "native";
Comment on lines 715 to 741

This comment has been minimized.

Copy link
@flokli

flokli Apr 20, 2020

Contributor

Again, I'd feel wayy more comfortable if we'd have more tests covering standard usecases, and see they're not broken.

This comment has been minimized.

Copy link
@Izorkin

Izorkin Apr 20, 2020

Author Contributor

Need create test to check sandbox mode with perl /lua scripts?

This comment has been minimized.

Copy link
@flokli

flokli Apr 21, 2020

Contributor

yes. We need more tests to ensure things are not broken.

@Izorkin Izorkin requested a review from flokli May 12, 2020
@flokli
Copy link
Contributor

flokli commented May 12, 2020

After some thinking, I agree with @aanderse and #85567 (comment).

We shouldn't push something as drastical as this to users without some broader community discussion on the topic.

Even if we decide to do this, it should probably be done in a much more gradual approach - providing an option to enable sandboxing, encouraging users to test it, and only default it (if at all) when it has seen enough feedback. All together with a lot of communication, release notes etc.

@Izorkin Izorkin force-pushed the Izorkin:nginx-sandbox branch 2 times, most recently from d4a6e97 to b8b1b0d May 12, 2020
@Izorkin
Copy link
Contributor Author

Izorkin commented May 12, 2020

@flokli @aanderse @Mic92 added option to enable sandbox mode. Sandbox mode is disabled by default.

@Izorkin Izorkin force-pushed the Izorkin:nginx-sandbox branch from b8b1b0d to 30f3b63 May 12, 2020
@Izorkin
Copy link
Contributor Author

Izorkin commented May 12, 2020

@GrahamcOfBorg test nginx
@GrahamcOfBorg test nginx-sandbox

@dasJ dasJ mentioned this pull request May 12, 2020
0 of 10 tasks complete
} // optionalAttrs cfg.enableSandbox {
# Sandboxing
ProtectSystem = "strict";
ProtectHome = mkDefault true;

This comment has been minimized.

Copy link
@Mic92

Mic92 May 12, 2020

Contributor

How about moving ProtectSystem, ProtectHome and MemoryDenyWriteExecute to enableSandbox and move other options above the conditional? I think those 3 options are the controversial ones that could break with peoples setup, where we need more feedback.

This comment has been minimized.

Copy link
@Izorkin

Izorkin May 12, 2020

Author Contributor

This variant?

        # Security
        NoNewPrivileges = true;
        # Sandboxing
        PrivateTmp = true;
        PrivateDevices = true;
        ProtectHostname = true;
        ProtectKernelTunables = true;
        ProtectKernelModules = true;
        ProtectControlGroups = true;
        RestrictAddressFamilies = [ "AF_UNIX" "AF_INET" "AF_INET6" ];
        LockPersonality = true;
        RestrictRealtime = true;
        RestrictSUIDSGID = true;
        PrivateMounts = true;
        # System Call Filtering
        SystemCallArchitectures = "native";
      } // optionalAttrs cfg.enableSandbox {
        ProtectSystem = "strict";
        ProtectHome = mkDefault true;
        MemoryDenyWriteExecute = !(builtins.any (mod: (mod.allowMemoryWriteExecute or false)) pkgs.nginx.modules);
      };

This comment has been minimized.

Copy link
@Mic92

Mic92 May 12, 2020

Contributor

Yes. This looks good to me.

This comment has been minimized.

Copy link
@Izorkin

Izorkin May 12, 2020

Author Contributor

In this variant, it will not work to disable the options PrivateTmp and etc.

This comment has been minimized.

Copy link
@Mic92

Mic92 May 12, 2020

Contributor

Right. PrivateTmp should be also in the sandbox section. The rest looks safe to me, but correct me if I am wrong.

This comment has been minimized.

Copy link
@Izorkin

Izorkin May 12, 2020

Author Contributor

Or leave the current variant, or rename cfg.enableSandbox to cfg.enableSandboxStrict.

This comment has been minimized.

Copy link
@Izorkin

Izorkin May 12, 2020

Author Contributor

I prefer the current variant.

@nixbitcoin
Copy link
Contributor

nixbitcoin commented May 12, 2020

I integrated the earlier module from #60646 in https://github.com/nixbitcoin/nix-bitcoin/tree/harden-nginx and never had any problems. I would be very happy to see services.nginx.enableSandbox implemented in nixpkgs.

@nixbitcoin
Copy link
Contributor

nixbitcoin commented May 12, 2020

Consider adding RemoveIPC systemd hardening option.

@Izorkin Izorkin force-pushed the Izorkin:nginx-sandbox branch 2 times, most recently from 9838d49 to fc58cb5 May 12, 2020
@Izorkin
Copy link
Contributor Author

Izorkin commented May 12, 2020

Added RemoveIPC

@Izorkin
Copy link
Contributor Author

Izorkin commented May 12, 2020

@GrahamcOfBorg test nginx
@GrahamcOfBorg test nginx-sandbox

Izorkin added 6 commits Apr 13, 2020
@Izorkin Izorkin force-pushed the Izorkin:nginx-sandbox branch from fc58cb5 to 94391fc May 12, 2020
@Mic92 Mic92 merged commit 6c437ef into NixOS:master May 13, 2020
14 checks passed
14 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="94391fc"; rev="94391fce1d5c4580482271c2b49ecffdef38b017"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="94391fc"; rev="94391fce1d5c4580482271c2b49ecffdef38b017"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="94391fc"; rev="94391fce1d5c4580482271c2b49ecffdef38b017"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="94391fc"; rev="94391fce1d5c4580482271c2b49ecffdef38b017"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="94391fc"; rev="94391fce1d5c4580482271c2b49ecffdef38b017"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="94391fc"; rev="94391fce1d5c4580482271c2b49ecffdef38b017"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="94391fc"; rev="94391fce1d5c4580482271c2b49ecffdef38b017"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Izorkin Izorkin deleted the Izorkin:nginx-sandbox branch May 13, 2020
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request May 13, 2020
(cherry picked from commit 6c437ef)
@aanderse aanderse mentioned this pull request Jul 6, 2020
12 of 18 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.