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

wireguard module: generatePrivateKeyFile: Fix chmod security race #121294

Merged
merged 2 commits into from
May 3, 2021

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented Apr 30, 2021

Fixes #121288.

Until now, the touch + chmod 600 + write approach made it possible for
an unprivileged local user read the private key file, by opening
the file after the touch, before the read permissions are restricted.

This was only the case if generatePrivateKeyFile = true and the parent
directory of privateKeyFile already existed and was readable.

This commit fixes it by using umask, which ensures kernel-side that
the touch creates the file with the correct permissions atomically.

This commit also:

  • Removes mkdir --mode 0644 -p "${dirOf values.privateKeyFile}"
    because setting permissions drw-r--r-- ("nobody can enter that dir")
    is awkward. drwx------ would perhaps make sense, like for .ssh.
    However, setting the permissions on the private key file is enough,
    and likely better, because privateKeyFile is about that file
    specifically and no docs suggest that there's something special
    about its parent dir.
  • Removes the chmod 0400 "${values.privateKeyFile}"
    because there isn't really a point in removing write access from
    the owner of the private key.
Motivation for this change
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.

@nh2 nh2 added the 1.severity: security Issues which raise a security issue, or PRs that fix one label Apr 30, 2021
@nh2 nh2 requested a review from grahamc April 30, 2021 16:39
@nh2 nh2 self-assigned this Apr 30, 2021
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Apr 30, 2021
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Apr 30, 2021
 NixOS#121288

Until now, the `touch + chmod 600 + write` approach made it possible for
an unprivileged local user read the private key file, by opening
the file after the touch, before the read permissions are restricted.

This was only the case if `generatePrivateKeyFile = true` and the parent
directory of `privateKeyFile` already existed and was readable.

This commit fixes it by using `umask`, which ensures kernel-side that
the `touch` creates the file with the correct permissions atomically.

This commit also:

* Removes `mkdir --mode 0644 -p "${dirOf values.privateKeyFile}"`
  because setting permissions `drw-r--r--` ("nobody can enter that dir")
  is awkward. `drwx------` would perhaps make sense, like for `.ssh`.
  However, setting the permissions on the private key file is enough,
  and likely better, because `privateKeyFile` is about that file
  specifically and no docs suggest that there's something special
  about its parent dir.
* Removes the `chmod 0400 "${values.privateKeyFile}"`
  because there isn't really a point in removing write access from
  the owner of the private key.
@nh2 nh2 force-pushed the issue-121288-wireguard-fix-chmod-race branch from 8a545d0 to 0dc08b4 Compare April 30, 2021 16:55
@@ -246,12 +246,15 @@ let
};

script = ''
mkdir --mode 0644 -p "${dirOf values.privateKeyFile}"
set -e
Copy link
Member

Choose a reason for hiding this comment

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

This is automatic:

#!${pkgs.runtimeShell} -e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but who knows if it will stay that way forever; thus especially for the more sensitive stuff, it may be good to be explicit.

(Also evidently a ton of shell users don't know about set -e, that it's off by default, and on-by-default in some NixOS scripts, so they may get surprises when copying scripts from one place to the other.)

@github-actions github-actions bot added 8.has: changelog 8.has: documentation This PR adds or changes documentation labels Apr 30, 2021
@nh2
Copy link
Contributor Author

nh2 commented Apr 30, 2021

I've added a release notes entry.

@ofborg ofborg bot added 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Apr 30, 2021
@infinisil infinisil merged commit 3e930b7 into NixOS:master May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.severity: security Issues which raise a security issue, or PRs that fix one 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

security: Wireguard generatePrivateKeyFile NixOS option allows unprivileged private key read
3 participants