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: Sandbox the service using systemd #60646

Closed
wants to merge 1 commit into from

Conversation

@dasJ
Copy link
Member

@dasJ dasJ commented May 1, 2019

  • nginx is not running as root anymore (Closes #56255)
  • Prevents nginx from accessing the filesystem
  • Enables most sandboxing options systemd offers
  • Moves the state directory to /var/lib

The only downside is that the worker processes are now able to read the certificates.

Motivation for this change

nginx is running on most of my servers on a well-known port and is therefore a great attack vector. Restricting it as much as possible should help around some zero-day exploits.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

- nginx is not running as root anymore (Closes #56255)
- Prevents nginx from accessing the filesystem
- Enables most sandboxing options systemd offers
- Moves the state directory to /var/lib
@Lassulus
Copy link
Contributor

@Lassulus Lassulus commented May 1, 2019

is this similair to #56255 ?
Edit: ah nvm, you already referenced it

@dasJ
Copy link
Member Author

@dasJ dasJ commented May 1, 2019

Oh, just in case the question arises. I switched to systemd's StateDirectory because systemd will handle existence, ownership, and permissions for me that way (also if one chooses to change the user).

Copy link
Contributor

@aanderse aanderse left a comment

I'm glad to see another attempt at this. @dasJ do you have a test plan you will document here? Do you have any volunteers to run this module in a variety of production environments? 19.09 is a bit off still so now is a great time to get documented testing underway. See my comments from the last attempt: #56255 (comment)

I still don't use nginx so am not sure how valid my comments/concerns are, but I'd like to be really sure that the testing involved with this PR is solid as it sets the precedent for apache httpd.

@@ -442,10 +437,10 @@ in
";
};

stateDir = mkOption {
default = "/var/spool/nginx";
stateDirName = mkOption {

This comment has been minimized.

@aanderse

aanderse May 1, 2019
Contributor

Is hard coding the state directory to /var/lib/nginx a bad idea?

This comment has been minimized.

@dasJ

dasJ May 2, 2019
Author Member

Systemd only gives me the possibility to name a directory under /usr/lib as state directory. Absolute paths and .. don't work. Using another directory means I'd have to take care about mkdir, chown and chmod myself. This way, systemd does all that for me.
The only tradeoff is that the directory is moved from /var/spool, but /var/lib is the more appropriate location imo. Maybe we can get rid of the logs directory as NixOS's nginx logs errors to stderr and has no access log enabled by default.

This comment has been minimized.

@aanderse

aanderse May 2, 2019
Contributor

Yes I'm all for locking the state directory down under /var/lib/ my question was asking if the directory name under /var/lib/ needs to be an option? Could the state directory not be hard coded to /var/lib/nginx (in other words StateDirectory = "nginx";? Is there value to the user in allowing choice of the directory name here?

This comment has been minimized.

@Izorkin

Izorkin May 16, 2019
Contributor

It is possible to leave use custom folder?

@dasJ
Copy link
Member Author

@dasJ dasJ commented May 2, 2019

@aanderse as I have never written any nixpkgs documentation, what would be an appropriate place? Just the changelog?

@aanderse
Copy link
Contributor

@aanderse aanderse commented May 2, 2019

@dasJ documentation regarding testing efforts wouldn't belong in the manual. Using this PR thread would be a sufficient place in my opinion. You could create a checkbox list of test scenarios and mark them as complete as people (yourself included) report back on this PR thread with what their test case is, how/when they ran it, and maybe some details relevant to showing how the test case worked/why the test case is positive evidence/what overall aspect is being tested. Just leave it up to the individual testing to write their report here and if anyone has more questions about the testing that occurred they could ask for clarification.

I might try to write up some examples of test cases I'd like to see in the apache scenario for reference/example when I get a chance.

NOTE: One more time I'd like to clarify that I have almost zero experience with nginx so I'm not sure if I'm going completely overboard here 😄 If we were talking about apacheHttpd then I know all this effort would be justified and I assume the same is try for nginx as this is at least the third time I've mentioned I feel thorough testing is required and no one has disagreed. If anyone knows better and can show proof that running nginx as non root is standard enough and I'm going overboard please feel free to say now.

@dasJ
Copy link
Member Author

@dasJ dasJ commented May 2, 2019

I'll do the writeup later. In the meantime, you can test this change using:

{
  services.nginx = {
    preStart = "";
    stateDir = "/var/lib/nginx";
    appendConfig = "pid /run/nginx/nginx.pid;";
  };

  systemd.services.nginx.serviceConfig = {
      # User and group
      User = "nginx";
      Group = "nginx";
      # Filesystem access
      ProtectSystem = "strict";
      ProtectHome = true;
      PrivateTmp = true;
      PrivateDevices = true;
      ProtectKernelTunables = true;
      ProtectKernelModules = true;
      ProtectControlGroups = true;
      StateDirectory = "nginx";
      StateDirectoryMode = 750;
      RuntimeDirectory = "nginx";
      RuntimeDirectoryMode = 750;
      # Capabilities
      CapabilityBoundingSet = [ "CAP_NET_BIND_SERVICE" ];
      AmbientCapabilities = [ "CAP_NET_BIND_SERVICE" ];
      NoNewPrivileges = true;
      # Misc.
      LockPersonality = true;
      RestrictRealtime = true;
      PrivateMounts = true;
      MemoryDenyWriteExecute = true;
}

nginx will warn that the user directive doesn't work when not running as root, but apart from that everything works.

@@ -45,7 +45,7 @@ let
''));

configFile = pkgs.writers.writeNginxConfig "nginx.conf" ''
user ${cfg.user} ${cfg.group};
pid /run/nginx/nginx.pid;

This comment has been minimized.

@Lassulus

Lassulus May 2, 2019
Contributor

why is the stateDir configurable but the runtimeDir not?

@aanderse
Copy link
Contributor

@aanderse aanderse commented May 3, 2019

As a quick example here are some of the things I would like to see tested for apacheHttpd to run as non root:

  • a wide variety of modules, both those included in nixpkgs and those not
    • according to wikipedia there are at least 157 modules
    • ideally you would get a good spread of these, including some proprietary modules
  • all supported mpm types tested thoroughly
  • performance and load testing in heavy environments along with metrics

While some of this might seem like overkill I think it is important to keep in mind that apache was designed to run as root and weird bugs can pop up in places you might never expect when you run software in ways which were not intended. We don't want to push a change like this into production and break a bunch of production machines when people upgrade to 19.09.

Please consider what the nginx equivalents of my points would be, if applicable.

Group = cfg.group;
# Filesystem access
ProtectSystem = "strict";
ProtectHome = true;

This comment has been minimized.

@aanderse

aanderse May 3, 2019
Contributor

What impact will this have on things like mod_userdir? Will this break any nginx modules?

Copy link
Contributor

@lopsided98 lopsided98 left a comment

I like using systemd sandboxing options and also not running nginx as root, but I think some of these changes will cause too much breakage. In particular, there are lots of reasons that nginx needs read and write access to arbitrary parts of the filesystem. Just looking through my configuration, I identified a couple of things that will break with no satisfactory workaround that I can see.

Also, these restrictions risk confusing users as they have to use nginx differently than they are used to on other operating systems.

Note that I have not actually tested this PR on any of my systems, so I could be wrong about some of the breakages.

# Capabilities
CapabilityBoundingSet = [ "CAP_NET_BIND_SERVICE" ];
AmbientCapabilities = [ "CAP_NET_BIND_SERVICE" ];
NoNewPrivileges = true;

This comment has been minimized.

@lopsided98

lopsided98 May 3, 2019
Contributor

Technically redundant, because NoNewPrivileges is implied by a number of the other options, but maybe it is better to be explicit.

ProtectSystem = "strict";
ProtectHome = true;
PrivateTmp = true;
PrivateDevices = true;

This comment has been minimized.

@lopsided98

lopsided98 May 3, 2019
Contributor

I think this will break my configuration which logs to journald using /dev/log.

This comment has been minimized.

@flokli

flokli May 17, 2019
Contributor

Shouldn't this still work with PrivateDevices=yes? https://github.com/systemd/systemd/blob/master/src/core/namespace.c#L734-L736

ProtectKernelTunables = true;
ProtectKernelModules = true;
ProtectControlGroups = true;
StateDirectory = [ "${cfg.stateDirName} ${cfg.stateDirName}/logs" ];

This comment has been minimized.

@lopsided98

lopsided98 May 3, 2019
Contributor

Shouldn't this be a list with two elements, rather than a single string with a space?

This comment has been minimized.

@flokli

flokli May 17, 2019
Contributor

We just pass that through to systemd, and systemd splits that with spaces.

User = cfg.user;
Group = cfg.group;
# Filesystem access
ProtectSystem = "strict";

This comment has been minimized.

@lopsided98

lopsided98 May 3, 2019
Contributor

This will break existing cache setups, including mine. I keep my cache in a folder under /var/cache (for no particular technical reason, but because it seemed like the proper place). One solution is to set CacheDirectory="nginx" and make users either place their caches in /var/cache/nginx or add another CacheDirectory to the service themselves. That seems better than putting caches in the state directory.

This comment has been minimized.

@lopsided98

lopsided98 May 3, 2019
Contributor

I usually use socket files in /var/run to communicate with services that I am reverse proxying with nginx and I believe this will break this functionality.

@chreekat
Copy link
Contributor

@chreekat chreekat commented May 16, 2019

Since it has been hard to implement this in the past and there are still a lot of question marks, what about making sandboxing opt-in behind some nginx.sandboxing.enable option? That way the brave can begin testing that config (the option should be copiously documented) without forcing all the problems to be found and fixed up front.

@aanderse
Copy link
Contributor

@aanderse aanderse commented May 16, 2019

@chreekat the brave are free to include this module in their own configurations and report results back here. Given how easy it is to replace a module in your own configuration.nix I fail to see the value of including such an experimental option on such an important piece of software. I'm happy to hear reasoning on differing opinions, though.

@chreekat
Copy link
Contributor

@chreekat chreekat commented May 16, 2019

That makes sense, people can indeed include this module. My reason for suggesting inclusion was knowing how important it is to break down patches into something that can get merged before they rot or the submitter runs out of steam. But of course the approach I suggested might not be workable. :)

One reason to add an option for such an experimental feature, though, would be to increase discoverability. I only found this PR because I'm taking a random walk through the repo; who else might be out there who could try this and report back if only they knew about it? Would the additional feedback justify giving people a sharp edge to play with?

@aanderse
Copy link
Contributor

@aanderse aanderse commented May 16, 2019

I think discourse as well as irc might be a better place to find people who will be willing to test this and report results back. Once an organized test plan which shows a fair bit of coverage for the spread of features available in nginx has been documented here I would suggest then trying to "recruit" people for testing on these mediums.

@dasJ dasJ closed this Feb 5, 2020
@dasJ dasJ deleted the dasJ:nginx-sandbox branch Feb 5, 2020
@Izorkin Izorkin mentioned this pull request Apr 19, 2020
3 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.