cmd: force use of a known working shell. #4141

Merged
merged 1 commit into from Mar 18, 2013

Conversation

Projects
None yet
4 participants
Contributor

wichert commented Mar 18, 2013

Users may have an odd shell: for example /bin/false is often used for system accounts. Currently the su call used by cmd to determine a users environment fails on those accounts. This pull request fixes that by explicitly asking su to use a known working shell.

@wichert wichert Force use of a known working shell
This solves problems with running commands as users with an invalid or
odd shell such as /bin/false
7a50783
Contributor

wichert commented Mar 18, 2013

FWIW: Travis failed, but as far as I can see only because tests on develop have been broken for a while, not due to any changes from this pull request.

@thatch45 thatch45 added a commit that referenced this pull request Mar 18, 2013

@thatch45 thatch45 Merge pull request #4141 from wichert/cmd-enforce-working-shell
cmd: force use of a known working shell.
95a1d87

@thatch45 thatch45 merged commit 95a1d87 into saltstack:develop Mar 18, 2013

Owner

thatch45 commented Mar 18, 2013

this is a great addition! Thanks!

wichert deleted the wichert:cmd-enforce-working-shell branch Mar 18, 2013

Contributor

seanchannel commented Mar 20, 2013

I must disagree; If a user is defined with a non-shell in /etc/passwd it is specifically to prevent any commands being run as that user and this forcibly overrides that configuration & could be a security vulnerability. That, however, is a matter of opinion. I would suggest managing files directly or other alternatives that would be the case in the absence of Salt, or configuring those users to have valid shells (using salt) if you must.

Separately, the use of su in this commit appears to be providing the username as the argument to the -s option, which should instead be the name of a shell to use (/bin/sh according to this pull req). This will most likely fail silently and thus fall-through & is probably the root cause of issue #4168 and issue #4171

I would strongly prefer to have new changes like this first discussed in a new issue before a merge is done as I do not know of any bugs this fixes, YMMV.

Thank you @terminalmage for pointing out the try/except

Owner

thatch45 commented Mar 20, 2013

@seanchannel, lol
the string formats correctly, take a closer look.
Also, the reason for non-login shells is to prevent logins, not process restrictions, which makes this valid. This patch also should fix both of the issues that reference it

Contributor

seanchannel commented Mar 20, 2013

assigned both issues to myself. at least one appears "fixed", but I'm not convinced. I also don't understand why this would have been cherry-picked for 0.13.3, but nevermind that. I maintain it's a secufity issue & the user should be aware (e.g. specify flags) when a non-shell is being overridden, but that's just an opinion and I wouldn't suggest considering it further at this point as this is going to be the thing users want anyway.

don't you lol me, mr. super-nice-wonderful-all-the-time-to-everybody-not-letting-me-be-cranky! :)

Owner

thatch45 commented Mar 20, 2013

Yes, I don't think it is a security issue, and I have seen many valid cases for it. The problem was that before the command would execute still, but with the wrong environment and shell.

@seanchannel ROFL!

Contributor

yml commented Mar 21, 2013

I am bit confused because my issues #4171 #4168 started with the ubuntu package 0.13.3-1 and a message earlier mentioned that this was cherry picked to that version.

Contributor

seanchannel commented Mar 21, 2013

I was confused before, don't let that confuse you too. :) Fixes for those should be in now. I assigned myself just as a reminder (since they weren't assigned).

Owner

thatch45 commented Mar 21, 2013

Yes, these fixes were after 0.13.3 was released, I don't believe they got cherry-picked

Contributor

yml commented Mar 22, 2013

I have just tested with this changeset 6740bbd and it seems that the issue is still there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment