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.

Sign up for GitHub

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

Jump to bottom

NixOS modules: chmod leaves opportunity to leak secrets #121293

Open
31 of 42 tasks
dotlambda opened this issue Apr 30, 2021 · 25 comments
Open
31 of 42 tasks

NixOS modules: chmod leaves opportunity to leak secrets #121293

dotlambda opened this issue Apr 30, 2021 · 25 comments
Labels
1.severity: security 3.skill: sprintable 5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems 6.topic: nixos
Milestone
23.11

Comments

@dotlambda
Copy link
Member

dotlambda commented Apr 30, 2021 •
edited by Stunkymonkey
Loading

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.
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
Copy link
Member Author

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.

@Ma27 Ma27 mentioned this issue Apr 30, 2021
Merged
10 tasks
@bjornfor
Copy link
Contributor

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

dotlambda commented Apr 30, 2021 •
edited
Loading

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.

Ma27 added a commit to Ma27/nixpkgs that referenced this issue Apr 30, 2021
@Ma27
nixos/gitea: set umask for secret creation
02c3bd2 
This ensures that newly created secrets will have the permissions
`0640`. With this change it's ensured that no sensitive information will
be word-readable at any time.

Related to NixOS#121293.

Strictly speaking this is a breaking change since each new directory
(including data-files) aren't world-readable anymore, but actually these
shouldn't be, unless there's a good reason for it.
@cole-h
Copy link
Member

cole-h commented Apr 30, 2021

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

@dotlambda
Copy link
Member Author

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

@m1cr0man
Copy link
Contributor

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

nh2 commented Apr 30, 2021

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.

markuskowa added a commit to markuskowa/nixpkgs that referenced this issue Apr 30, 2021
@markuskowa
nixos/slurm: fix creation of slurmdbd config file
d07185f 
replace cp/chmod by install to avoid security issues.
See NixOS#121293
@markuskowa markuskowa mentioned this issue Apr 30, 2021
Merged
10 tasks
@dotlambda
Copy link
Member Author

dotlambda commented May 1, 2021 •
edited
Loading

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 👎 means?

@mohe2015
Copy link
Contributor

mohe2015 commented May 1, 2021

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

rnhmjoj commented May 1, 2021

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

picnoir commented May 1, 2021

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

alyssais commented May 1, 2021 •
edited
Loading

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.

bjornfor added a commit to bjornfor/nixpkgs that referenced this issue May 1, 2021
@bjornfor
nixos/atd: prefer 'install' over 'mkdir/chmod/chown'
225d915 
I don't think there was a security issue here, but using 'install' is
preferred.

Ref NixOS#121293.
@bjornfor bjornfor mentioned this issue May 1, 2021
Merged
10 tasks
bjornfor added a commit to bjornfor/nixpkgs that referenced this issue May 1, 2021
@bjornfor
nixos/wpa_supplicant: prefer 'install' over 'touch/chmod/mkdir/chgrp'
5d47dc7 
Ref NixOS#121293.
@bjornfor bjornfor mentioned this issue May 1, 2021
Merged
10 tasks
@Nadrieril
Copy link
Member

Nadrieril commented May 1, 2021 •
edited
Loading

The syncserver module is broken and due for removal (#74725), so I guess it doesn't matter

@picnoir picnoir mentioned this issue May 1, 2021
Closed
10 tasks
@picnoir
Copy link
Member

picnoir commented May 1, 2021

The nsd fix is there: #121427

@endgame endgame mentioned this issue May 1, 2021
Merged
@endgame
Copy link
Contributor

endgame commented May 1, 2021 •
edited
Loading

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

github-actions bot pushed a commit that referenced this issue Jul 6, 2021
@m1cr0man @github-actions
nixos/acme: Ensure certs are always protected
c192fd5 
As per #121293, I ensured the UMask is set correctly
and removed any unnecessary chmod/chown/chgrp commands.
The test suite already partially covered permissions
checking but I added an extra check for the selfsigned
cert permissions.

(cherry picked from commit 083aba4)
@nixos-discourse
Copy link

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

nh2 commented Jan 23, 2022

@ryantm Would your ryantm@b0088d4 be upstreamable for mattermost?

@nixos-discourse
Copy link

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

@Artturin Artturin modified the milestones: 21.05, 23.05 Dec 31, 2022
@RaitoBezarius RaitoBezarius modified the milestones: 23.05, 23.11 May 31, 2023
@nbraud
Copy link
Contributor

nbraud commented Dec 24, 2023 •
edited
Loading

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

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

This was referenced Jan 26, 2024
Merged
Merged
This was referenced Feb 3, 2024
Merged
Merged
Merged
@Stunkymonkey Stunkymonkey mentioned this issue Feb 19, 2024
Closed
13 tasks
This was referenced Mar 27, 2024
Merged
Merged
This was referenced Apr 7, 2024
Open
Merged
This was referenced Apr 15, 2024
Merged
Merged
@samueldr samueldr added the 5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems label Apr 23, 2024
This was referenced Apr 28, 2024
Draft
Open
Open
Merged
@Stunkymonkey Stunkymonkey mentioned this issue May 17, 2024
Merged
13 tasks
This was referenced Jul 7, 2024
Merged
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
1.severity: security 3.skill: sprintable 5. scope: tracking Long-lived issue tracking long-term fixes or multiple sub-problems 6.topic: nixos
Projects
None yet
Milestone
23.11
Development

No branches or pull requests