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

sudo: set secure_path with root-installed software only #64209

Open
wants to merge 1 commit into
base: master
from

Conversation

@grahamc
Copy link
Member

commented Jul 3, 2019

Given the following hypothetical:

Nix allows any user to install software in to their PATH. A crafty
person with malicious intent could use Nix to alter the installed
software of a given user without editing their PATH, and without any
"writable" user directories in the PATH.

This change explicitly sets sudo's PATH to only include root installed
software. This at least reduces the chances of that hypothetical
coming true.

Tested and seemed to work a treat. Also easy to roll back if it is
unwanted.

Motivation for this change
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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

sudo: set secure_path with root-installed software only
Given the following hypothetical:

> Nix allows any user to install software in to their PATH. A crafty
> person with malicious intent could use Nix to alter the installed
> software of a given user without editing their PATH, and without any
> "writable" user directories in the PATH.

This change explicitly sets sudo's PATH to only include root installed
software. This at least reduces the chances of that hypothetical
coming true.

Tested and seemed to work a treat. Also easy to roll back if it is
unwanted.
@grahamc

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

@samueldr points out this would break sudo on binaries fetched by nix-shell, which seems pretty annoying (nix-shell -p mtr anyone?) so maybe we don't want this by default? Hmm

@grahamc

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

Not a bad workaround, though:

[grahamc@Petunia:~]$ nix-shell -p vim

[nix-shell:~]$ sudo vim
sudo: vim : commande introuvable

[nix-shell:~]$ sudo $(which vim)

(worked fine)

@grahamc grahamc requested a review from Ma27 Jul 3, 2019

@grahamc

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

@andir

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

It sounds like a good idea but I am not sure we have a clear picture of "valid" uses.

For the mtr problem you should be using programs.mtr.enable anyway….

I have the feeling that we might be able to accept this since we are already breaking so many "standard" assumptions when compared to other Distros (Debian, Fedora, …). It might still be an infinite source of user confusion (paradoxically; it should have been the other way) as it used to work on likely works everywhere else. I am also thinking about stuff like ruby, python, … "virtualenvs" where sudo might be used seldomly and we do not know about it.

As with any change: We can try and roll back if things are too broken. I would definitely prefer some kind of A/B-Testing for these but we don't have that.
(nixos.experiements.enable = true; anyone? :-))

@alyssais

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Nix allows any user to install software in to their PATH. A crafty
person with malicious intent could use Nix to alter the installed
software of a given user without editing their PATH, and without any
"writable" user directories in the PATH.

I’m not sure I understand how they would do that without already having root?

@grahamc

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

Yeah, that wasn't worded well, @alyssais. I don't mean an arbitrary user, but the current user. Any program run as a given user could install new programs for that same user.

@grahamc

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

@andir

I have the feeling that we might be able to accept this since we are already breaking so many "standard" assumptions when compared to other Distros (Debian, Fedora, …).

RHEL and Debian lookalikes already set this to something like:

Defaults        secure_path="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin"

:)

@alyssais

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Does sudo nix-shell work out of the box? I imagine it does, right?

@grahamc

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

It does!

$ sudo nix-shell -p hello --run hello
Bonjour, le monde !
@Ekleog

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@grahamc I assume the threat model is:

A user has some restricted sudo access (eg. only sudo foo --bar), and could use $PATH to make foo execute arbitrary programs as root if foo does a system() or similar.

In which case it's AFAIK not nix-specific (anyone could just set $PATH to point to ~/bin), but the generic attack against sudo, which is as you mention mitigated by secure_path.

@alyssais If I'm not mistaken with the above, then I think this is the threat model you were wondering about -- and in this situation sudo nix-shell is not a possible bypass :)

@grahamc

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

@grahamc I assume the threat model is:

A user has some restricted sudo access (eg. only sudo foo --bar), and could use $PATH to make foo execute arbitrary programs as root if foo does a system() or similar.

With Nix in the path, there is no illusion of restricting what programs a user can run with sudo. So, this makes no attempt to help with that. This is more about a problem with an attacker transparently replacing executables on the PATH. For example, replacing vim with a pwnd vim in the user's profile, they might never know because the $PATH looks right and they "never" use nix-env. As it currently stands, next time they run sudo vim they'll give this pwnd vim root access. With this PR, sudo vim would use the root-installed vim.

@@ -42,6 +42,37 @@ in
'';
};

security.sudo.enableSecurePath = mkOption {
type = types.bool;
default = true;

This comment has been minimized.

Copy link
@edolstra

edolstra Jul 3, 2019

Member

Options considered harmful. If we do this, it should not be an option IMHO. Random variability between NixOS systems should be avoided.

This comment has been minimized.

Copy link
@alyssais

alyssais Jul 3, 2019

Member

+1, since it can be overridden with extraRules anyway.

@Ekleog

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@grahamc IMO your example is not a realistic threat model: if the attacking program can replace vim, it can replace sudo with myevilsudo just as well (ie. something that would be a wrapper around sudo but also installs a backdoor or similar).

OTOH, for a user with restricted sudo access (including, presumedly, no nix, otherwise it wouldn't be restricted), the patch does totally make sense to me: the user with something like [...] = (ALL) /run/current-system/sw/bin/something --foobar, assuming something does system("foobar"), could execute whatever they want as root by sending in a $PATH that includes their own evil foobar without this patch.

@grahamc

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

This patch has nothing to do with explicitly setting the PATH for a restricted sudo user, it is just a Defaults clause. More specific restrictions should be set with extraRules. This specific option should not be used for general access restriction.

@grahamc grahamc requested a review from adisbladis Jul 3, 2019

@grahamc

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

I asked in #debian why they set the secure_path the way they do, and they said indeed it is to make carefully restricted users' PATHs safe. I'm not sure we can claim that value, though, since nix-shell will still be in PATH and that is a lot of power.

@grahamc

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

Note: I'm requesting reviews and ccing people who tell me they combine sudo and nix-shell.

@adisbladis

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Not a bad workaround, though:

My feeling is that sudo nix-shell -p ... would be preferable over nix-shell -p ... --run '...' is preferable anyway as in the former case the entire environment is being set up as the privileged user and the latter is possibly being influenced by ~/.config/nixpkgs and other user-specific configuration.

@Ekleog

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@grahamc I think debian's threat model, as well as the one I'm envisioning, is basically something like:

#!/bin/sh
# (in /whatever/foo.sh)
exec bar

And then the user U having access to sudo /whatever/foo.sh.

In this model, with $PATH user-controlled, U would be able to execute anything as root by naming it bar and prepending its dir to $PATH. While, with $PATH root-controlled (ie. with this patch), U can execute only the root-expected bar.

Now, if /whatever/foo.sh were calling nix-shell, then it would potentially open up some more ways to bypass things by U, but the point is so long as /whatever/foo.sh isn't explicitly calling nix-shell, it won't allow the user to run whatever-they-want :)

@timokau

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

So if a malicious program could just take over sudo anyways, what are the upsides? You can't really install suftware as non-root anyway on debian, so there are not much downsides there. But there are for nixos.

@grahamc

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

what are the upsides?

Yes, that is the question.

@Ekleog

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@timokau The upsides are to defend root against the malicious (or compromised) restricted-sudo user U described in #64209 (comment) :)

@timokau

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

That does seem a bit contrieved to me. Its basically the setuid scenario, and the same security concerns apply. This change sounds like it could be a significant inconvenience (I manage most of my packages with home-manager, sometimes use nix-shell to query hardware info or something that needs a one-off sudo package). The "restricted sudo" case doesn't seem worth it to me.

@Ekleog

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

It is exactly the setuid scenario indeed. OTOH, sudo is actually designed to work around the security shortcomings of setuid.

BTW, this “cleaning $PATH” behavior is very similar to the “cleaning $HOME” behavior we already have, and has similar inconvenience level: user-level config (vs. user-level packages) don't get in, because they would be dangerous as they'd allow unexpected software to run as root (eg. .vimrc or similar).

Arguably managing all one's packages through home-manager is thus a broken workflow (it installs packages for root as a user), and it makes sense to make it no longer work. OTOH, it is convenient.

I'm personally in favor of this change, mostly because people expect sudo to clean $PATH, while they don't expect setuid to, as it's the way all other distros do it.

@timokau

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Well "cleaning $HOME" is actually a convenience in my view, since it keeps all my home from being cluttered by root-owned files.

Arguably managing all one's packages through home-manager is thus a broken workflow (it installs packages for root as a user), and it makes sense to make it no longer work. OTOH, it is convenient.

I don't understand that point. Why would that be a broken workflow?

mostly because people expect sudo to clean $PATH

Do they though? Because I don't. Debian may do it, but I don't think most people will have noticed because most debian binaries will live in one of the whiltelisted paths anyway.

@Ekleog

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

This will be my last message on this topic, as we're clearly running around in circles. (for the “broken workflow”, it's in the parenthesis that just follow these two words, in your quote)

I'm sorry in advance if my tone gets heated along the message (which is another reason for this to be my last message), I've tried to tone it down by re-reading but may have missed places.

Here is a scenario:

  1. I'm a sysadmin.
  2. I allow my apache2 user to run sudo my_database_cleanup.sh, which will be called by some kind of webcron or whatever.
  3. my_database_cleanup.sh is some kind of shell script, written like any usual shell script, that just performs a database cleanup.
  4. (Note: this is exactly how restricted-sudo is used just about everywhere)
  5. Poof, apache2 now has rights equivalent to those of root, by using the attack listed at #64209 (comment). Welcome privilege escalation.

NixOS currently encourages behavior that unfortunately doesn't work with the fix to this vulnerability, by having user-only-installed packages propagate to root. However, this behavior is not right, just like a user being able to install packages in root's environment would be wrong (though it's not the same threat model).

The workaround @grahamc suggested at #64209 (comment) works when you already are with user-installed packages (nix-shell or home-manager) and really want to do that, and other than that just enter a sudo and then enter a nix-shell.

But having the user able to make it look to other programs like it's installed software as root is exactly what causes the vulnerability I described above.

Now, your opposition to this is based on usability. To which my answer will be: why are you even installing sysadmin tools via home-manager, and not via /etc/nixos or via root's home-manager? There is no semantic reason for them to be there, and they could shadow things installed as root.

Your user being an all-privileged sudo user should be considered the exception, not the rule. Sudo is not just a way to have a keyword that you need to type to brick your system, it's a tool that 1. switches to another user, 2. runs a command, and 3. switches back to the original user. When you see it in these 3 steps, it becomes obvious that what you have installed as your local user should not affect execution of the command in step 2 -- if it did (and for the time being it does), then it'd be a security vulnerability.

@grahamc

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

One way to easily preserve the UX and improve automated script security is to make wheel an exempt group.

@timokau

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

This will be my last message on this topic, as we're clearly running around in circles. (for the “broken workflow”, it's in the parenthesis that just follow these two words, in your quote)

I'm sorry in advance if my tone gets heated along the message (which is another reason for this to be my last message), I've tried to tone it down by re-reading but may have missed places.

I certainly don't want to step on anyones toes, sorry if it seems that way :)

Here is a scenario:

1. I'm a sysadmin.

2. I allow my `apache2` user to run `sudo my_database_cleanup.sh`, which will be called by some kind of webcron or whatever.

3. `my_database_cleanup.sh` is some kind of shell script, written like any usual shell script, that just performs a database cleanup.

4. (Note: this is _exactly_ how restricted-sudo is used just about everywhere)

5. Poof, `apache2` now has rights equivalent to those of root, by using the attack listed at [#64209 (comment)](https://github.com/NixOS/nixpkgs/pull/64209#issuecomment-508123221). Welcome privilege escalation.

Okay now the scenario doesn't seem contrieved anymore. For me sudo is just a convenient tool to elevate my privileges temporarily, for that usecase I would use setuid. I also think in that case authors should use absolute paths anyway. But I know that "I think people should be doing" is not what people actually will be doing. And its always good to remove footguns.

NixOS currently encourages behavior that unfortunately doesn't work with the fix to this vulnerability, by having user-only-installed packages propagate to root. However, this behavior is not right, just like a user being able to install packages in root's environment would be wrong (though it's not the same threat model).

I disagree here. sudo (for me) is about executing something with the privileges of another user, while keeping the environment. Just because a package is in my PATH while I run sudo, I wouln't consider it to "propagate to root" -- its just part of my current environment.

The workaround @grahamc suggested at #64209 (comment) works when you already are with user-installed packages (nix-shell or home-manager) and really want to do that, and other than that just enter a sudo and then enter a nix-shell.

In my opinion that would break user expectations too much. I would certainly have been surprised, and it wouldn't be obvious why its not working as I expect.

Now, your opposition to this is based on usability. To which my answer will be: why are you even installing sysadmin tools via home-manager, and not via /etc/nixos or via root's home-manager? There is no semantic reason for them to be there, and they could shadow things installed as root.

Actually looking into my shell history, I have to admit I don't actually do that very often. Still, not every tool run with sudo has to strictly be a "sysadmin tool", it could be a tool that is only executed with elevated permissions as an exception. The best example I can come up with off the top of my head is an editor. That's not ideal of course (sudoedit), but you get the idea.

I like @grahamc's idea. If the main thread model here is the restricted-sudo case, why not just apply to that one. Only downside is that it could be even more confusing because of the inconsistency.

@Ma27

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

(disclaimer: I only got pinged because I participated in the twitter poll).

For me personally the sudo $(which foobar) workaround is fine (if I understood everything correctly 😅 ), I don't have any strong opinions about adding further options for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.