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

Fix cmd.script runas for Windows #36520

Merged
merged 4 commits into from
Sep 28, 2016

Conversation

twangboy
Copy link
Contributor

@twangboy twangboy commented Sep 22, 2016

What does this PR do?

Fixes problem with ``cmd.script` when the runas parameter is set in Windows.

What issues does this PR fix or reference?

#33761

Previous Behavior

The default Temp directory for Windows us something like C:\Users\Joe\AppData\Local\Temp. The cmd.script copies the script down to the temporary folder using a randomized file name. Then it executes the script. The problem is that the runas user doesn't have permissions to the TEMP directory of the process that cached the file... so it can't run it.

New Behavior

For Windows, with runas, andcwdnot set, the file will be copied to the file_cache. The folder permissions will be set to allow that user to read and execute within that directory. The`runas`` user can now access the script.

Tests written?

No

cwd = os.path.join(__opts__['cachedir'], 'wintmp')
if not os.path.isdir(cwd):
__salt__['file.mkdir'](root)
ret = __salt__['win_dacl.add_ace'](
Copy link
Contributor

Choose a reason for hiding this comment

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

This ret appears to be unused after assignment.

@@ -2004,6 +2004,14 @@ def _cleanup_tempfile(path):
# Backwards compatibility
saltenv = __env__

if salt.utils.is_windows() and runas and cwd is None:
cwd = os.path.join(__opts__['cachedir'], 'wintmp')
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's truly necessary to apply directory permissions to an entire directory just for the sake of being able to one execute one file, I think that we should take the extra step of giving each script its own directory. What do you think about randomizing a directory name per-script and then caching the script there?

@twangboy
Copy link
Contributor Author

@cachedout Did you get a chance to look at the change?

@cachedout cachedout merged commit e7def53 into saltstack:2016.3 Sep 28, 2016
@damon-atkins
Copy link
Contributor

Keep in mind if you have access to edit script.bat, you can change it while the script is running. So a long running script gives time to inject commands.

I am surprised the runas user can get into cache directory.

e.g. might be an admin on the Windows Server, I can run salt highstates, which would have the password, wait for the file to be created, and gain access to user id I would not normally have access too.

@twangboy twangboy deleted the fix_cmd.script_runas branch August 21, 2017 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants