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

[BUG] Snapper module _is_text_file function broken #60046

Closed
stealthcopter opened this issue Apr 17, 2021 · 4 comments
Closed

[BUG] Snapper module _is_text_file function broken #60046

stealthcopter opened this issue Apr 17, 2021 · 4 comments
Assignees
Labels
Bug broken, incorrect, or confusing behavior Regression The issue is a bug that breaks functionality known to work in previous releases. severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around Silicon v3004.0 Release code name
Milestone

Comments

@stealthcopter
Copy link

stealthcopter commented Apr 17, 2021

Description

I believe the snapper module's _is_text_file has been broken as part of the security vulnerabilities that were fixed in this pull request: #59648

Steps to Reproduce the behavior
Attempt to run the snapper module's diff function. Or you can run the following code to test (copied from snapper module):

import subprocess

def _is_text_file(filename):
    """
    Checks if a file is a text file
    """
    type_of_file = subprocess.run(
        ["file", "-bi", filename],
        check=False,
        stdout=subprocess.STDOUT,
        universal_newlines=True,
    ).stdout
    return type_of_file.startswith("text")

_is_text_file('textfile.txt')

If you test this out just in a python file you notice it errors with the following:

OSError: [Errno 9] Bad file descriptor

Expected behavior
For _is_text_file to function correctly

Additional context

** Fix **

This is fixed by changing STDOUT to PIPE. Example:

def _is_text_file(filename):
    """
    Checks if a file is a text file
    """
    type_of_file = subprocess.run(
        ["file", "-bi", filename],
        check=False,
        stdout=subprocess.PIPE,
        universal_newlines=True,
    ).stdout
    return type_of_file.startswith("text")

I would be happy to submit a pull request for this if this fix is acceptable.

Note that I attempted to notify you of this via the security email address but was ignored. I have detailed the security vulnerability that was fixed by the previous pull request here:
https://sec.stealthcopter.com/saltstack-snapper-minion-privledge-escaltion/

@stealthcopter stealthcopter added Bug broken, incorrect, or confusing behavior needs-triage labels Apr 17, 2021
@welcome
Copy link

welcome bot commented Apr 17, 2021

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@OrangeDog OrangeDog added Regression The issue is a bug that breaks functionality known to work in previous releases. severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around and removed needs-triage labels Apr 19, 2021
@OrangeDog OrangeDog added this to the Approved milestone Apr 19, 2021
@sagetherage sagetherage added the Silicon v3004.0 Release code name label Apr 19, 2021
@sagetherage sagetherage modified the milestones: Approved, Silicon Apr 19, 2021
@sagetherage sagetherage assigned Ch3LL and dwoz and unassigned Ch3LL Apr 19, 2021
@sagetherage
Copy link
Contributor

@stealthcopter

Note that I attempted to notify you of this via the security email address but was ignored. I have detailed the security vulnerability that was fixed by the previous pull request here:
https://sec.stealthcopter.com/saltstack-snapper-minion-privledge-escaltion/

I will look into this, we did receive other emails, but it is true I didn't see one around this issue described. I will get back to you on any findings.

@stealthcopter
Copy link
Author

@sagetherage Ah sorry, I thought I had included it in the original report but couldn't check due to encrypting it 🤦

@sagetherage
Copy link
Contributor

@stealthcopter we did migrate the email address recently, but have received emails before and after the migration date. Can you give me some more details on the email address and date sent? You can send those to me here: sage@saltstack.com

thank you!

bdrung added a commit to bdrung/salt that referenced this issue Apr 27, 2021
In SaltStack Salt 2016.9 through 3002.6, a command injection
vulnerability exists in the snapper module that allows for local
privilege escalation on a minion. The attack requires that a file is
created with a pathname that is backed up by snapper, and that the
master calls the snapper.diff function (which executes popen unsafely).

Cherry-pick the fix from pull request saltstack#59648 [1], but also fix the
regression introduced by that commit [2].

[1] saltstack#59648
[2] saltstack#60046
Closes: #987496
Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
bdrung added a commit to bdrung/salt that referenced this issue May 12, 2021
In SaltStack Salt 2016.9 through 3002.6, a command injection
vulnerability exists in the snapper module that allows for local
privilege escalation on a minion. The attack requires that a file is
created with a pathname that is backed up by snapper, and that the
master calls the snapper.diff function (which executes popen unsafely).

Cherry-pick the fix from pull request saltstack#59648 [1], but also fix the
regression introduced by that commit [2].

[1] saltstack#59648
[2] saltstack#60046
Closes: #987496
Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
@sagetherage sagetherage assigned s0undt3ch and unassigned dwoz Jun 25, 2021
@sagetherage sagetherage modified the milestones: Silicon, Approved Aug 11, 2021
twangboy pushed a commit to twangboy/salt that referenced this issue Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Regression The issue is a bug that breaks functionality known to work in previous releases. severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around Silicon v3004.0 Release code name
Projects
None yet
Development

No branches or pull requests

6 participants