nixos/nsd: atomically create state directories with appropriate mode

[picnoir]

Related to #121293

I'm not 100% sure this is an actual vulnerability, the statedir being
700, I'm not really sure how we could exploit this to extract secrets.

Better be safe than sorry though. Replacing the file creation followed
by chmod/chown with atomic install statements.

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.

@GrahamcOfBorg test nsd

[dotlambda]

I'm not 100% sure this is an actual vulnerability, the statedir being
700, I'm not really sure how we could exploit this to extract secrets.

Only copyKeys was problematic.
EDIT: Never mind, it wasn't.

[endgame]

I don't have merge rights, but I do have a query. I thought about suggesting (umask 377 && ...) instead of mktemp/install, but since you need to set owner and group I like your way better.

[endgame]

Two lines above, you create ${stateDir}/var with the right owner and group. Is there a risk that this script is going to run against a state dir that already exists? If so, maybe it's worth adding a comment to explain this chown?

Also, this chown is happening before the cat below. Does that mean ${stateDir}/don't touch anything in here will have the wrong owner/group?

[picnoir]

Two lines above, you create ${stateDir}/var with the right owner and group. Is there a risk that this script is going to run against a state dir that already exists? If so, maybe it's worth adding a comment to explain this chown?

This chown here is applied recursively to the files inside $state/var which might already exist. The install statement serve a different purpose, it's here to create the directory on the first run.

---

Also, this chown is happening before the cat below. Does that mean ${stateDir}/don't touch anything in here will have the wrong owner/group?

You're right, my bad. In the grand scheme of things, this comment is part of nixpkgs, a public git repo though. I'm not sure we can consider that as a secret.

[picnoir]

I'll address your comments when I'll be in front of my desktop.

Note: I'm not using NSD anymore, I'm not the maintainer of this module and I don't think there was a vulnerability to start with here. I'm not willing to nitpick this to death.

Feel free to push to this branch or create a subsequent PR if you want to clean this service further.

I don't think setting the permissions as part of startPre is the right approach for the record. If we really care about dnssec secrets being stolen, we should start using systemd's StateDirectory + StateDirectoryMode and harden the systemd service.

[picnoir]

ptal

Edit: doh, somehow I managed to fail a copy paste >< I'll fix it in 1 sec.

[endgame]

Seems fine. I have no power to merge it, though.

[hrdinka]

Hi, thanks for your changes. They do however not improve much. My feedback:

• The secret key directory is created with mod 0700 as root so attackers cannot access the files between them being written and the following chmod/chown.
• With your patch every key is first copied to /tmp where they are staying till the next reboot or whenever /tmp gets cleared on that system.
• Is install really atomically? The man page does not give any guarantees in that manner. It is a single command but it does still do all syscalls sequentially. It might be using umask, but we could do this as well with the bonus of knowing what is happening under the hood for sure (+ portability with different install commands and versions).
• On the plus side, creating the empty state directories is just one line instead of two/three per directory.

So I am for using install where it saves lines of code but I am against using it for storing the keys, since it creates tmp files.

[hrdinka]

@NinjaTrappeur, we can still go forward with this PR, because besides less lines there is one more thing install does. It ensures 0700 on existing ${stateDir}/var directories, since there is no chmod at the end, just the mkdir -m. It does not help with the other two dirs, because they get deleted first, but it still is shorter.

I am just strictly against copying them into /tmp. The module does a good job at only storing the keys at expected locations. Users do not expect this to happen and we cannot make any assertions about /tmp. It could be a ramdisk (vulnerable to RAM readout attacks), could be a container and be stored on a network drive or on the host machine or whatever.

[picnoir]

Re-reading this thread, I realize closing the PR without any comments was rude for both you and endgame who take some time to review the PR. It was not my initial intention, all my apologies about this poor communication from my end.

---

Is install really atomically?

Actually, I was wrong, it is not. At least in gnu coreutils. See https://github.com/coreutils/coreutils/blob/master/src/install.c#L473 and https://github.com/coreutils/coreutils/blob/master/src/install.c#L670 (we're copying the file before changing the ownership attributes).

This is a falsehood I've been cargo-culting for a while. Thanks for questioning this and indirectly nerd-sniping me :)

---

As I mentioned earlier, I do not use NSD anymore and I don't see much value in this patch. I'm not really willing to invest more time into this. I'm not deleting this branch on purpose: feel free to take any part of this commit you need if you feel like it.

---

Again, apologies for the poor communication.

[hrdinka]

@NinjaTrappeur don’t worry, everything is fine and I am happy for your participation. PRs and Issues exist to discuss exactly this kind of stuff, so thanks for raising concerns. I will be making some changes to NSD soon, so I will likely include a few improvements from this then. Have a nice day.