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

Input validation does not raise SaltInvocationError in win_dsc.py #47443

Closed
skylerberg opened this Issue May 2, 2018 · 2 comments

Comments

Projects
None yet
4 participants
@skylerberg
Contributor

skylerberg commented May 2, 2018

Description of Issue/Question

Browsing the tool LGTM, I saw several errors that are initialized but not raised for input validation in win_dsc.py: https://lgtm.com/projects/g/saltstack/salt/snapshot/cea932162130f65040b889fc4e2f0f3bae98c27d/files/salt/modules/win_dsc.py?sort=name&dir=ASC&mode=heatmap&showExcluded=false#L722

Looking at SaltInvocationError and its base class, there doesn't seem to be any magic happening on initialization that would cause the error to be automatically raised. My conclusion is that this is likely a mistake and the input validation routines are most likely not doing anything.

The impact of this is that the powershell command that this code generates could be malformed or have invalid values.

I considered potential security ramifications, but I think there are not any because this would just allow powershell cmd injection on servers where you can already run modules with salt. Please let me know if I have overlooked any security concerns around this.

@dwoz dwoz self-assigned this May 3, 2018

@garethgreenaway garethgreenaway added this to the Approved milestone May 4, 2018

@garethgreenaway garethgreenaway modified the milestones: Approved, Blocked May 4, 2018

@garethgreenaway

This comment has been minimized.

Member

garethgreenaway commented May 4, 2018

@dwoz Since you self-assigned I'll let you determine the issue status, eg. bug or not, and the severity.

@dwoz

This comment has been minimized.

Contributor

dwoz commented May 4, 2018

@garethgreenaway This doesn't look like it has any real security impact. It does look pretty buggy however. I will patch for the bug and make sure we have tests.

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