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
Closed

nixos/nginx: Sandbox the service using systemd #60646

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 nginx: do not run anything as root #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
Member

Lassulus commented May 1, 2019

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

@dasJ
Copy link
Member Author

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
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible to leave use custom folder?

@dasJ
Copy link
Member Author

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
Member

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 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;
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 the stateDir configurable but the runtimeDir not?

@aanderse
Copy link
Member

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@lopsided98 lopsided98 left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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
Member

@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

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
Member

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.

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

Successfully merging this pull request may close these issues.

None yet

8 participants