Page MenuHomePhabricator

Investigate potential issues with the sudoeres env_keep values
Closed, ResolvedPublic


We currently have the following configuration in the sudoeres file

env_keep += "HOME TMUX STY"

Home was originally added in 2014 with the message

sudo >= 1.7.4p4-2 is resetting $HOME and $MAIL to the target user when
  sudoing. Undo this for $HOME by adding it to env_keep as we're too
  used to it.

However i have herd anecdotal accounts of this causing issues so we should investigate if this is required and it it is required can we confine it to only the commands/usere who require it

"TMUX STY" were added recently*. The initial reason to add theses was to overcome an issue in wmflib.interactive.ensure_shell_is_durable() method which has since had a fix (release pending). however it was thought theses variables could still be generally useful to pass through. A quick search didn't result in any issues and this doesn't appear to create any additional security issues however we should still investigate if this is actually needed and perhaps also confine this to commands which actually need them.

* Original PS adding TMUX/STY with more justification.

Event Timeline

An example of where passing $HOME caused issues for me:
Effectively, anything that assumes $HOME will be set to the homedir of the current user breaks when sudo does not set $HOME appropriately.
mysql defaults to looking for ~/.my.cnf, which is why i've kept running into this.

Re: passing through $TMUX, i'm not concerned about security issues, i'm just slightly concerned that it's going to cause.. unexpected outcomes. Like $HOME, it's non-standard to expose it, and it seems like a hack to work around an issue that has a much simpler user-based workaround until wmflib is fixed and released (i.e. changing the user's $TERM in tmux to contain the substring screen).

in relation to home I think it makes sense to remove it and for users who want that behaviour by default could simply add alias sudo='sudo -H' (would obvioulsy need to be communicated) but would also like to here from @faidon and or @MoritzMuehlenhoff in case I', missing something.

In relation to TMUX/STY my gut feeling is that is much less likely to cause an issue but it is based on nothing. @klausman are you able to put forward a case for TMUX?

jbond triaged this task as Medium priority.Feb 26 2021, 1:28 PM

TMUX being visible is, as mentioned, not a security issue when sudo'ing to non-root. The var contains just a path, with its own permissions, and racing attacks with symlinks are unlikely to work since the enclosing dir is typically owned by the user, or it has tempdir semantics. It is theoretically possible that a bad user config makes this attackable, but it still seems very remote compare to other threats.

As for the "necessity" of exposing STY and/or TMUX in order to detect presence of screen or tmux, I think just checking TERM for screen* and tmux* is enough. Since screen itself is unlikely to ever have a TERM setting that doesn't start with a screen prefix, and the tmux manpage explicitly mentions that it likely will break if its TERM is not screen* or tmux*:

default-terminal terminal
        Set the default terminal for new windows created in this session -
        the default value of the TERM environment variable.  For tmux to
        work correctly, this must be set to ‘screen’, ‘tmux’ or a deriva‐
        tive of them.

tl;dr: if we keep the TERM logic to check for both prefixes, I think we can ditch the TMUX and STY exceptions. Given what has been said about HOME, I think we can remove that as well and then we'd be basically using upstream's/Debian's settings in this regard.

jbond moved this task from Unsorted 💣 to Active 🚁 on the User-jbond board.

@MoritzMuehlenhoff @faidon are you able to comment in relation to the history around why we have HOME in sudoes env_keep and more importunately can we remove it

If you're talking about my 2014 commit… if I recall correctly¹ this was in order to minimize changes between different distribution and enforce a unified policy (this was part of a larger patch series to put some structure around sudoers). I opted into env_keep because that's what folks were most used to and "secure enough". I don't have an opinion these days or whether it should be removed or not :)

1: not a given, that was a long time ago :)

Change 697723 had a related patch set uploaded (by Jbond; author: John Bond):

[operations/puppet@production] sudo: drop keep_env option

Mentioned in SAL (#wikimedia-operations) [2021-06-09T11:27:55Z] <jbond> drop keep_env from sudo config - #T275852

Change 697723 merged by Jbond:

[operations/puppet@production] sudo: drop keep_env option

jbond claimed this task.

config has now been updated to remove keep_env