Skip to content
This repository has been archived by the owner on Apr 24, 2020. It is now read-only.

About sudo detection #852

Closed
iilonmasc opened this issue May 31, 2018 · 24 comments
Closed

About sudo detection #852

iilonmasc opened this issue May 31, 2018 · 24 comments

Comments

@iilonmasc
Copy link
Member

I just encountered an error message while using next

May 31 11:52:26 mymachine sudo[10180]:      tim : a password is required ; TTY=pts/2 ; PWD=/home/tim ; USER=root ; COMMAND=/usr/bin/true

System Information:

OS: Fedora 28
ZSH: 5.5.1
Used segments: os_icon context dir status time

Steps to reproduce:

  1. Start a shell running journalctl -f as root to monitor the error message
  2. Start a new non-root zsh with powerlevel9k on next branch
  3. Just enter a cd or ls (any command will do)
  4. The error message appears on journalctl
@dritter
Copy link
Member

dritter commented May 31, 2018

The only segment where we use sudo is the context segment. That change comes from #729 by @mikewl .
Shouldn't redirecting STDERR to /dev/null already mute the error message?

@iilonmasc
Copy link
Member Author

It is muted, I am not seeing this in the non-root shell, but I see the error messages in the root shell running journalctl

@vmivanov
Copy link

It's worth pointing out that systemd's journal is not the only issue from using sudo to set the context segment's state. Even on systems without systemd, despite "muted", the error is still logged in various authentication logs. Moreover, since running sudo -n true 2>/dev/null results in a failed authentication it also triggers an alert (e.g. email) containing the error message to be sent out to the system administrator. Since this happens every time the prompt is (re)generated, it easily floods the administrator's inbox with hundreds of security alerts.

The latter is especially easy to achieve on systems where sudoers are asked to enter their password every time sudo is used as opposed to temporarily caching their escalated privileges.

@bhilburn
Copy link
Member

Oops, somehow lost track of this one. @sambadevi - thanks for the bug report (a couple of months ago), and thanks for re-raising this, @vmivanov. It is much appreciated.

@sambadevi - I assume you are still seeing this in the latest master and next, yes?

@iilonmasc
Copy link
Member Author

iilonmasc commented Jul 17, 2018

@bhilburn both, yes. Tested it a few minutes ago with master and next, I still get the same message

I should mention, this error was reported for fedora 28. I tested on arch this time

@bhilburn
Copy link
Member

Okay, per your issue description and what @vmivanov is describing, I think it's important we address this ASAP. I wonder if there is actually is a 'safe' way to detect sudo. Calling out to sudo is not ideal, anyway.

I'm genuinely asking, here - I don't know, hah. I wonder if @rjorgenson knows anything, here.

@vmivanov
Copy link

@sambadevi that's ok, as the problem should be independent of your choice of distribution. Whether you see the error in logs is mostly dependent on the logging configuration, and chances are most systems will log and/or report any authentication errors by default. I can certainly observe this behaviour both on Gentoo and on Debian Testing.

@bhilburn no problem, thanks for all your efforts - this theme is priceless! I forgot to mention that at the time of writing my comment, it refers to the master branch.

Wrt the SUDO state itself, I'm not sure if there is a way to detect whether a user has temporarily escalated privileges without running sudo and raising an authentication error as I'm certainly not aware of one. I fail, however, to see the use case for this feature as even if the timeout has expired on systems that cache the escalation of privileges, it will not be reflected on the prompt segment until it is regenerated after a command is executed. If the point is to inform the user whether they have sudo privileges in general, which I presume would be more useful to be wary of, then this can be achieved simply by checking whether the user is part of the sudo and/or wheel system group.

@iilonmasc
Copy link
Member Author

According to this serverfault answer you have special env vars set if you're sudo (copied from there):

$ sudo env |grep SUDO

SUDO_COMMAND=/usr/bin/env
SUDO_USER=tal
SUDO_UID=501
SUDO_GID=20

@vmivanov
Copy link

@sambadevi The problem with this approach is that these environment variables are not persisted outside of the sudo call. For example, imagine you have recently executed a command via sudo, entered your password, and are currently in the grace period of not having to enter a password again. A subsequent call to $ env | grep SUDO (i.e. without prefixing it with sudo) will not show the above variables. Hence, the sudo call is still needed to make use of this feature.

@Ratatosk42
Copy link

You can try this command : sudo -S true < /dev/null 2> /dev/null

With -S flag, you provide a password from /dev/null ! I don’t see auth error on journalctl anymore.

@onaforeignshore
Copy link
Contributor

onaforeignshore commented Jul 18, 2018

Did you try sudo -nv?

From man sudo:
-n, --non-interactive
Avoid prompting the user for input of any kind. If a password is required for the command to run, sudo will display an error message and exit.
-v, --validate
Update the user's cached credentials, authenticating the user if necessary. For the sudoers plugin, this extends the sudo timeout for another 5 minutes by default, but does not run a command. Not all security policies support cached credentials.

@vmivanov
Copy link

vmivanov commented Jul 18, 2018

@onaforeignshore yes, this is exactly what the script in powerlevel9k.zsh-theme already does at various points, as I mentioned in my previous comments which is what is causing the problem. For example, take a look at the following snippet from line 623 of the current master branch:

if [[ $(print -P "%#") == '#' ]]; then
  current_state="ROOT"
elif [[ -n "$SSH_CLIENT" || -n "$SSH_TTY" ]]; then
  if sudo -n true 2>/dev/null; then
    current_state="REMOTE_SUDO"
  else
    current_state="REMOTE"
  fi
elif sudo -n true 2>/dev/null; then
  current_state="SUDO"
fi

So no, what you are referring to in the man pages will not work. Moreover, -v should absolutely be avoided as you do not want to unnecessarily extend the user's escalated privileges beyond the grace period as this could be a security concern potentially leading to a case of having escalated privileges indefinitely. Such extension should only be granted if the user is explicitly running sudo during the grace period, which is one of the main points of sudo.

@gzugmeyer Interesting! But bear in mind the systemd journal is not the only concern here, in fact, I would say it's a lower priority issue. That said, having done a quick test of your snippet it does seem to partially resolve the issue. It correctly returns a value of 0 or 1 depending on the outcome of sudo and doesn't seem to trigger an authentication alert to the system administrators. It will, however, pollute the system authentication logs (e.g. /var/log/auth.log by default) with the following messages:

Jul 18 10:40:37 myhostname sudo: pam_unix(sudo:auth): conversation failed
Jul 18 10:40:37 myhostname sudo: pam_unix(sudo:auth): auth could not identify password for [myusername]

I would argue that using sudo to test is, in general, best avoided as @bhilburn mentioned.

@bhilburn
Copy link
Member

Thanks for weighing in, @gzugmeyer! It looks like that would be a solid fix if it weren't for the log flooding.

So, I think @vmivanov has a great point, here:

I fail, however, to see the use case for this feature as even if the timeout has expired on systems that cache the escalation of privileges, it will not be reflected on the prompt segment until it is regenerated after a command is executed. If the point is to inform the user whether they have sudo privileges in general, which I presume would be more useful to be wary of, then this can be achieved simply by checking whether the user is part of the sudo and/or wheel system group.

The prompt indicating sudo really isn't of much use, because the prompt could have been drawn long before the sudo window went away. I think we need to figure out what our goal for this feature is before sorting out how we want to move forward. In my mind, there are three possible use-cases:

  1. The current scenario, where sudo indicates whether or not the user had elevated privileges when the prompt was drawn. There are clearly some folks who want this functionality (e.g., @onaforeignshore), but I think it will be misleading for most.
  2. Indicating whether or not the previous command was executed with elevated privileges. In this scenario, it's really more of a new feature for the status segment, not user, and I suspect it will just confuse people.
  3. A permanent indicator of whether or not the user is in sudoers/wheel.

To be honest, I personally don't think any of the three of these is useful, but am open to hearing other opinions. If people really like the current functionality, #1, then I would be okay leaving it in and (a) defaulting it to disabled (b) conditionalizing that code path out of execution otherwise.

@bhilburn bhilburn changed the title Call to sudo /usr/bin/true About sudo detection Jul 18, 2018
@onaforeignshore
Copy link
Contributor

@bhilburn Since my prompt is always elevated (my user is in sudoers with the NOPASSWD flag), I don't have any use for this. I was just adding the state to solve the sudoers problem.

@vmivanov
Copy link

@bhilburn seconded. Not really sure what the best approach would be for a SUDO state. Option 1 has the downsides I mentioned. Option 2 would make sense in my view for such a feature. However, given that the command will have already been prefixed with "sudo" it would be kind of redundant, unless having some extra colouring, etc. to bring the attention that this was a privileged command. Option 3 makes the most sense in my view, but it does raise the question of whether it is actually necessary. Chances are sudoers already know they have sudo rights.

Either way, it would be good to have it configurable so that it can be disabled if relying on a call to sudo. Especially in the meantime so that people relying on the master branch won't have to deal with the log/journal problem.

@rjorgenson
Copy link
Collaborator

@bhilburn @dritter
I did some looking into this as this bit me last week when I updated p9k on all my work systems and started getting sudo alerts emailed to me and the rest of the admin team =P

From everything I could tell, determining wether there is an active sudo session is not possible without calling the sudo command. I found this superuser article from like 8 years ago, but its been active pretty recently - https://superuser.com/questions/195781/sudo-is-there-a-command-to-check-if-i-have-sudo-and-or-how-much-time-is-left - There are many suggestions on how to do this, but they all require running sudo or already having root access.

I'm not really sure of the use case for this personally, if I have to run something as sudo knowing wether or not I have a session isn't going to change the command I run or my behavior. But if this is needed functionality for something the best solution I was able to come up with was creating a wrapper script for sudo and tracking that within p9k. We'd create a sudo function that just calls the actual sudo with the arguments passed by the user and then tracks the time and return status of the sudo command. This probably wouldn't be trivial and might raise some eyebrows if we start hijacking user commands to do our stuff. Another far less effective solution would be checking the last 10(or some definable timeout value) minutes of command history for a sudo command, obviously this is not ideal and can easily result in false positives. I don't recommend it but that's the only other thing I could come up with.

The final option is to leave this in as a configurable option(off by default) for the users who do have the use case for it and have mitigations in place for the results of doing this(a sudo call on every prompt draw).

If anyone is looking for a quick fix for this you can add the below code to your .zshrc file and change your POWERLEVEL9K_LEFT_PROMPT_ELEMENTS entry for context to pcontext, same for the right prompt elements if you use context there. This was my emergency fix and is non-invasive to the powerlevel9k codebase.

prompt_pcontext() {
  local current_state="DEFAULT"
  typeset -AH context_states
  context_states=(
    "ROOT"        "yellow"
    "SUDO"        "yellow"
    "DEFAULT"     "yellow"
    "REMOTE"      "yellow"
    "REMOTE_SUDO" "yellow"
  )

  local content=""

  if [[ "$POWERLEVEL9K_ALWAYS_SHOW_CONTEXT" == true ]] || [[ "$(whoami)" != "$DEFAULT_USER" ]] || [[ -n "$SSH_CLIENT" || -n "$SSH_TTY" ]]; then
      content="${POWERLEVEL9K_CONTEXT_TEMPLATE}"
  elif [[ "$POWERLEVEL9K_ALWAYS_SHOW_USER" == true ]]; then
      content="$(whoami)"
  else
      return
  fi

  if [[ $(print -P "%#") == '#' ]]; then
    current_state="ROOT"
  # elif [[ -n "$SSH_CLIENT" || -n "$SSH_TTY" ]]; then
  #   if sudo -n true 2>/dev/null; then
  #     current_state="REMOTE_SUDO"
  #   else
  #     current_state="REMOTE"
  #   fi
  # elif sudo -n true 2>/dev/null; then
  #   current_state="SUDO"
  fi

  "$1_prompt_segment" "${0}_${current_state}" "$2" "$DEFAULT_COLOR" "${context_states[$current_state]}" "${content}"
}

@TJuberg
Copy link

TJuberg commented Jul 30, 2018

Came here to see if anyone else had encountered this as well.

In any form of shared environment this "feature" is likely to end up getting unwanted attention from security, or in the worst case having your service terminated.

Having the security team spammed by email or the IDS system go haywire because a blind sudo is issued every time you hit enter is not very pleasant for either party.

I implemented the same fix as @rjorgenson and it would be nice if this was turned off by default to prevent unpleasantness for future users.

@docwhat
Copy link
Contributor

docwhat commented Jul 31, 2018

@bhilburn This took me a while to track down. I consider this to be a pretty big problem since it set off security alerts and made me very concerned.

The sudo -S true < /dev/null solution is just as bad since it generates log messages like pam_unix(sudo:auth): auth could not identify password for [docwhat]. Depending on your security settings, that'll trip alerts too.

I think the correct approach is to use the SUDO_* variables as mentioned by @sambadevi.

I don't think that @vmivanov's objections are a problem. On my system (Ubuntu) they are exported:

$ sudo -i -u docwhat
$ env | grep SUDO_
SUDO_COMMAND=/bin/zsh
SUDO_USER=docwhat
SUDO_UID=9009
SUDO_GID=9009

But even if they aren't exported, then I would argue that you have left the confines of sudo when launching sub-commands anyway. Having it in the parent shell is good enough and better than nothing.

Either way, calling sudo over and over again is a "Really Bad Idea™".

@docwhat
Copy link
Contributor

docwhat commented Jul 31, 2018

I forgot to point out that sudo -n true and sudo -S true both do not detect whether you're using sudo or not.

They only show that you can run a command as root at the moment.

It doesn't work for:

  • You're running as root; it'll always return true.
  • You're sudoed into a user that is not root and you don't have access to root for whatever reason (sudo timestamp expired or you never had access).
  • Systems where /bin/true is allowed to be run with NOPASSWD.

Checking for SUDO_COMMAND is the only thing that will actually capture the meaning of the that state.

@dritter
Copy link
Member

dritter commented Aug 2, 2018

So, from my point of view we have an Issue with the sudo state, and no clear idea about the intention of the state. I think the best we can do is to fix the actual issue of generating security notices with the SUDO env variables that might not work in every scenario, but probably well enough. I added #937 to the v0.6.6 project.

@docwhat
Copy link
Contributor

docwhat commented Aug 3, 2018

Detecting if you can sudo to root with no password at moment was never the right check.

I think the variable is very close to the intention.

Sent with GitHawk

@dritter dritter mentioned this issue Aug 8, 2018
@vmivanov
Copy link

@docwhat very elegant solution, I had not thought of the SUDO_COMMAND variable!

I still fail to see the usefulness of this feature but the solution does indeed appear to preserve original intent without the extra downsides.

Wrt the SUDO_ environment variables, if not else it serves to indicate that on different systems the behaviour might be different (at least as per the default configuration), further reinforcing the fact that using sudo itself to perform the check was fundamentally quite bad.

@mcinquin
Copy link
Contributor

The @docwhat solution solves the error of the /bin/true command executed as sudo !

@bhilburn
Copy link
Member

Merged into master as part of #944! Will be in the v0.6.6 release.

MenkeTechnologies pushed a commit to MenkeTechnologies/powerlevel9k that referenced this issue Sep 3, 2020
`sudo -n true` only checks that we _could_ use `sudo`, not if we are in
a sudo session.

closes Powerlevel9k#852
MenkeTechnologies pushed a commit to MenkeTechnologies/powerlevel9k that referenced this issue Sep 3, 2020
`sudo -n true` only checks that we _could_ use `sudo`, not if we are in
a sudo session.

closes Powerlevel9k#852
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants