search

NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.43k stars 13.64k forks source link

NixOS modules: chmod leaves opportunity to leak secrets #121293

Open dotlambda opened 3 years ago

dotlambda commented 3 years ago

touch, cp, etc. followed by chmod leaves an opportunity to leak secrets as explained in https://github.com/NixOS/nixpkgs/issues/121288. install -m does the same, but in one command. Use umask instead. The following modules might be affected:

EDIT: wording, to make clear that I did not check whether this actually causes a problem in all of the above cases

dotlambda commented 3 years ago

I would say that mkdir followed by chmod is usually not problematic because no files will be created in between the two operations. However, it might be better style to use mkdir -m or install -dm.

bjornfor commented 3 years ago

However, it might be better style to use mkdir -m or install -dm.

I don't know about install, but I think mkdir -m will not set the permissions if the directory already exists (so configured vs actual permissions might diverge).

dotlambda commented 3 years ago

I don't know about install, but I think mkdir -m will not set the permissions if the directory already exists (so configured vs actual permissions might diverge).

Yes, if -p is used existing directories will not be changed. If it's not used, it will just cause an error. install -dm will change the permissions of an existing directory though.

cole-h commented 3 years ago

Even though the actual ping never went through, I think you meant @colemickens for nixos/modules/virtualisation/azure-image.nix ;)

dotlambda commented 3 years ago

@cole-h Sorry, I didn't mean to relate your name to any FUD nonsense ;-)

m1cr0man commented 3 years ago

Wrt acme, lego will write with 0600 permissions, which means that only the acme user can initially read any of the secure files it outputs. The chmod/chgrp/chowns scattered throughout acme.nix are there to either open or correct permissions such that the configured group will be able to read the certs. The acme-fixperms service is there solely to fix insecure permissions that may have existed in past iterations of the module (on upgrading systems). The only possible leak I can see is through the selfsigned service, which applies a chmod 600 after minica is called. I will set the umask on the selfsigned cert generator service instead.

nh2 commented 3 years ago

The chmod/chgrp/chowns scattered throughout acme.nix are there to either open or correct permissions such that the configured group will be able to read the certs

We should best annotate such safe uses with comments to make it clear.

dotlambda commented 3 years ago

I would say that mkdir followed by chmod is usually not problematic because no files will be created in between the two operations. However, it might be better style to use mkdir -m or install -dm.

@mohe2015 Can you elaborate on what your :-1: means?

mohe2015 commented 3 years ago

I would say that mkdir followed by chmod is usually not problematic because no files will be created in between the two operations. However, it might be better style to use mkdir -m or install -dm.

I'm not sure but I could imagine that someone could write to the directory in the rare case that the default permissions allow that. I just thought it would be cleaner to go all the way if somebody puts in the effort. But if this is really impossible it shouldn't matter.

rnhmjoj commented 3 years ago

In searx it's not much of an issue: it only allows users of the searx group to read the secrets, but I will try to fix it anyway.

picnoir commented 3 years ago

FYI:

  1. I'm not NSD's maintainer.
  2. I did not write that code.
  3. The state dir is mode 700.

I'll fix it nevertheless.

alyssais commented 3 years ago

I don't believe the mailman module is vulnerable, because it uses mktemp(1) to create its secret file

Files are created u+rw, and directories u+rwx, minus umask restrictions.

So the chmod should just have the effect of broadening the permissions to make it group readable. (It also does u-w, but that's meaningless since the owner is root.)

But I will do some testing today to verify that I am correct here.

Edit: I have confirmed this by commenting out the chmod/chown, and indeed the file starts being accessible only to root.

Nadrieril commented 3 years ago

The syncserver module is broken and due for removal (https://github.com/NixOS/nixpkgs/pull/74725), so I guess it doesn't matter

picnoir commented 3 years ago

The nsd fix is there: https://github.com/NixOS/nixpkgs/pull/121427

endgame commented 3 years ago

I've done the compute image metadata fetchers in #121449, but GH assigned no reviewers. Can someone here review for me?

talyz commented 3 years ago

nixos/modules/virtualisation/fetch-instance-ssh-keys.bash uses umask 077 early on, so it should be fine.

dotlambda commented 3 years ago

Use umask or install -m instead.

Actually, install -m has the same problem: https://github.com/NixOS/nixpkgs/pull/121427#issuecomment-831141768

endgame commented 3 years ago

You can mark off the following two from me:

ymarkus commented 3 years ago

nixos/modules/services/web-apps/bookstack.nix is fixed too.

Mic92 commented 3 years ago

https://github.com/NixOS/nixpkgs/pull/121667 buildkite

m1cr0man commented 3 years ago

121750 acme :)

nixos-discourse commented 2 years ago

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-should-one-report-small-vulnerabilities-in-individual-modules/17279/1

nh2 commented 2 years ago

@ryantm Would your https://github.com/ryantm/nixpkgs/commit/b0088d4fb42c92bed81444442378d0d9ea531f0f be upstreamable for mattermost?

nixos-discourse commented 2 years ago

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-should-one-report-small-vulnerabilities-in-individual-modules/17279/2

nbraud commented 8 months ago

touch, cp, etc. followed by chmod leaves an opportunity to leak secrets as explained in #121288. install -m does the same, but in one command. Use umask instead.

Thank you so much for going through all the potentially-affected services <3

  • [ ] nixos/modules/config/swap.nix (@nbraud @tokudan @roberth)

Did you mean to tag someone else? I did the PR for wpa_supplicant some time ago, but didn't poke swap.nix

P.S.: Speaking of which, #275312 (backporting the wpa_supplicant fix) is needing a review ;3