X Tutup
The Wayback Machine - https://web.archive.org/web/20220413190130/https://github.com/python/cpython/issues/68966
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

mailcap.findmatch: document shell command Injection danger in filename parameter #68966

Open
TheRegRunner mannequin opened this issue Aug 2, 2015 · 18 comments
Open

mailcap.findmatch: document shell command Injection danger in filename parameter #68966

TheRegRunner mannequin opened this issue Aug 2, 2015 · 18 comments
Labels
3.11 docs stdlib type-security

Comments

@TheRegRunner
Copy link
Mannequin

@TheRegRunner TheRegRunner mannequin commented Aug 2, 2015

BPO 24778
Nosy @vstinner, @bitdancer
Files
  • screenshot.png
  • The Quote Problem.py
  • mailcap patch.zip: mailcap.py patches and diffs for python2.7 and python 3.5
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2015-08-02.08:25:07.171>
    labels = ['type-security', '3.11', 'library', 'docs']
    title = 'mailcap.findmatch: document shell command Injection danger in filename parameter'
    updated_at = <Date 2022-04-06.15:30:37.106>
    user = 'https://bugs.python.org/TheRegRunner'

    bugs.python.org fields:

    activity = <Date 2022-04-06.15:30:37.106>
    actor = 'vstinner'
    assignee = 'docs@python'
    closed = False
    closed_date = None
    closer = None
    components = ['Documentation', 'Library (Lib)']
    creation = <Date 2015-08-02.08:25:07.171>
    creator = 'TheRegRunner'
    dependencies = []
    files = ['40099', '40116', '40897']
    hgrepos = []
    issue_num = 24778
    keywords = []
    message_count = 14.0
    messages = ['247857', '247861', '247944', '247946', '247951', '247979', '247992', '248058', '248061', '248062', '248070', '248074', '253689', '416878']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'r.david.murray', 'docs@python', 'TheRegRunner']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = None
    status = 'open'
    superseder = None
    type = 'security'
    url = 'https://bugs.python.org/issue24778'
    versions = ['Python 3.11']

    @TheRegRunner
    Copy link
    Mannequin Author

    @TheRegRunner TheRegRunner mannequin commented Aug 2, 2015

    if the filename contains Shell Commands they will be executed if they
    are passed to os.system() as discribed in the docs.
    Filename should be quoted with quote(filename) to fix the bug.

    https://docs.python.org/2/library/mailcap.html

    "mailcap.findmatch(/caps/, /MIMEtype/[, /key/[, /filename/[, /plist/]]])

    Return a 2-tuple; the first element is a string containing the
    command line to be executed
    (which can be passed to\*os.system() \*),
    

    ......"

    Exploid Demo wich runs xterm but should not :
    =============================

    import mailcap
    d=mailcap.getcaps()
    commandline,MIMEtype=mailcap.findmatch(d, "text/*", filename="'$(xterm);#.txt")
    ## commandline = "less ''$(xterm);#.txt'"
    import os
    os.system(commandline)
    ## xterm starts

    =============================

    By the way ... please do not use os.system() in your code, makes it unsafe.

    Best regards
    Bernd Dietzel
    Germany

    @TheRegRunner TheRegRunner mannequin added stdlib type-security labels Aug 2, 2015
    @TheRegRunner
    Copy link
    Mannequin Author

    @TheRegRunner TheRegRunner mannequin commented Aug 2, 2015

    Maybe it would be a good idea to do so as run-mailcap does :

    theregrunner@mint17 : ~ € run-mailcap --debug "';xterm;#'.txt"

    • parsing parameter "';xterm;#'.txt"
    • Reading mime.types file "/etc/mime.types"...
    • extension "txt" maps to mime-type "text/plain"
    • Reading mailcap file "/etc/mailcap"...
      Processing file "';xterm;#'.txt" of type "text/plain" (encoding=none)...
    • checking mailcap entry "text/plain; less '%s'; needsterminal"
    • program to execute: less '%s'
    • filename contains shell meta-characters; aliased to '/tmp/fileV7f2MZ'
    • executing: less '/tmp/fileV7f2MZ'
      theregrunner@mint17 : ~ €

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Aug 3, 2015

    In this case os.system is an appropriate API, because it mirrors the API of mailcap itself (that is, mailcap entries are shell commands).

    I'm not convinced there is a security bug here. It seems to me that there are two cases: either the filename is determined by the program, in which case there is no security issue, or the filename comes from an external source, and the program will have had to *write it to the file system* before the mailcap command will do anything. So the security hole, if any, will have happened earlier in the process.

    Now, one can argue that the quoting should be done in order to preserve the meaning of an arbitrary filename. Which would allay your concern even if I disagree that it is a real security bug :)

    (I don't understand why run-mailcap uses an alias rather than correctly quoting the meta-characters.)

    @TheRegRunner
    Copy link
    Mannequin Author

    @TheRegRunner TheRegRunner mannequin commented Aug 3, 2015

    @david
    Thanks for the comment :-)

    I think if you read the Documentation
    https://docs.python.org/2/library/mailcap.html
    this may lead new programmers, wich may never heard of Shell Injections before, step by step directly to write insecure webbbrowsers and/or mail readers. At least there should be a warning in the docs !

    You ask why run-mailcap do not use quotig, i believe because quoting is not an easy thing to do, i attached a demo ;-)

    Thank you.

    @TheRegRunner
    Copy link
    Mannequin Author

    @TheRegRunner TheRegRunner mannequin commented Aug 3, 2015

    Exploid Demo wich works with quote() :

    >>> commandline,MIMETYPE=mailcap.findmatch(d, 'text/*', filename=quote(';xterm;#.txt'))
    >>> commandline
    "less '';xterm;#.txt''"
    >>> os.system(commandline)
    ### xterm starts

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Aug 4, 2015

    Hmm. I see. The problem is that our desire to quote conflicts with mailcap's attempts to quote.

    I now agree with you that run-mailcap's approach is correct, but creating a temporary alias is out of scope for findmatch. That would need to be done by findmatch's caller.

    I think we should add a documentation note about the problem and the solution. I don't see any reliable way to detect the problem and raise an error for the same reason that quoting doesn't work. (The aliasing can tolerate false positives; but, for backward compatibility reasons, an error detection function here cannot.)

    It would be possible to add a helper for the aliasing to 3.6, but if someone wants to propose that they should open an new issue for the enhancement.

    I'm

    @bitdancer bitdancer added the docs label Aug 4, 2015
    @TheRegRunner
    Copy link
    Mannequin Author

    @TheRegRunner TheRegRunner mannequin commented Aug 4, 2015

    Yes changing the docs is a good idea.

    I was thinking about a patch :

    import os
    ####### patch
    import random
    try:
      from shlex import quote
    except ImportError:
      from pipes import quote
    #######

    ....... and so on ....

    # Part 3: using the database.

    def findmatch(caps, MIMEtype, key='view', filename="/dev/null", plist=[]):
        """Find a match for a mailcap entry.
    Return a tuple containing the command line, and the mailcap entry
    used; (None, None) if no match is found.  This may invoke the
    'test' command of several matching entries before deciding which
    entry to use.
    
    """
    
        entries = lookup(caps, MIMEtype, key)
        # XXX This code should somehow check for the needsterminal flag.
        for e in entries:
            if 'test' in e:
                test = subst(e['test'], filename, plist)
                if test and os.system(test) != 0:
                    continue
    ####### patch
            ps=''.join(random.choice('python') for i in range(100))
            x=e[key]
            while '%s' in x:
                x=x.replace('%s',ps)  
            command=subst(x, MIMEtype, filename, plist)
            while "'"+ps+"'" in command:
                command=command.replace("'"+ps+"'",quote(filename))
            while ps in command:
                command=command.replace(ps,quote(filename))          
    ######  command = subst(e[key], MIMEtype, filename, plist)
            return command, e
        return None, None

    @TheRegRunner
    Copy link
    Mannequin Author

    @TheRegRunner TheRegRunner mannequin commented Aug 5, 2015

    # for the docs ... quoting of the filename when you call mailcap.findmatch()

    f=";xterm;#.txt" # Shell Command Demo ... xterm will run if quote() fails

    import mailcap
    import random
    try:
      from shlex import quote
    except ImportError:
      from pipes import quote
    d=mailcap.getcaps()
    PY=''.join(random.choice('PYTHON') for i in range(100))
    cmd,MIMEtype=mailcap.findmatch(d, 'text/plain', filename=PY)
    while "'"+PY+"'" in cmd:
       cmd=cmd.replace("'"+PY+"'",quote(f))
    while PY in cmd:
       cmd=cmd.replace(PY,quote(f))
    print(cmd)  
    # less ';xterm;#.txt'

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Aug 5, 2015

    I have no idea what your code samples are trying to accomplish, I'm afraid, but that's not the kind of documentation I'm advocating anyway.

    @bitdancer bitdancer changed the title mailcap.findmatch() ........ Shell Command Injection in filename mailcap.findmatch: document shell command Injection danger in filename parameter Aug 5, 2015
    @TheRegRunner
    Copy link
    Mannequin Author

    @TheRegRunner TheRegRunner mannequin commented Aug 5, 2015

    What i do is the last doc is like this :

    1. Replace the filename with a random name
    2. Run mailcap.findmatch() with the random name
    3. If exists, replace the quote characters ' before and behind the random name with nothing.
    4. Now the random name has no quoting from mailcap itself
    5. So now we can use our own quote() savely

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Aug 5, 2015

    Ah, that's a clever idea.

    @TheRegRunner
    Copy link
    Mannequin Author

    @TheRegRunner TheRegRunner mannequin commented Aug 5, 2015

    Thanks :-)

    As you may noticed i now choosed to use a random name made of the chars of "PYTHON" in BIG letters instead of small letters i used before.

    Thats because i do not want to get in trouble with the little "t" in %t wich is replaced by the subst function too.

    @TheRegRunner
    Copy link
    Mannequin Author

    @TheRegRunner TheRegRunner mannequin commented Oct 29, 2015

    My patch for mailcap.py. Please check and apply my patch please.

    1. I have removed the os.system() calls for security reasons.

    2. New "findmtach_list()" function witch returns the commandline as a [list] witch can be passed to subprocess instead of passing it to os.system().

    3. New run() function to execute the cmd_list with subprocess.

    4. The test() function now uses findmatch_list() and run() instead of the old findmatch() and os.system() calls.

    5. The subst() function is now shorter an does a quote(filename) when its replacing %s with a filename.

    6. The "old" findmatch() function is still there if the user still likes to have the commandline as a "string".
      Attention ! With this old findmatch() function it's still possible that a shell command in the filename like '$(ls).txt' will be executed when the users passes the string to os.system() outside the mailcap script. Use findmatch() only for backwards compatibility.

    7. Use the new findmatch_list() an run() for future projects.

    8. Add 1)-7) to the docs

    Thank you.

    @tiran tiran added the 3.7 label Sep 24, 2016
    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Apr 6, 2022

    In 2022, Python 3.11 still has the issue:
    ----------------
    vstinner@apu$ python3.11 -m mailcap
    Mailcap files:
    /home/vstinner/.mailcap
    /etc/mailcap
    (...)
    Mailcap entries:
    (...)
    text/html
    copiousoutput
    lineno 5
    view /usr/bin/xdg-open %s

    $ python3 -m mailcap text/html 'filename; pwd'
    Executing: /usr/bin/xdg-open filename; pwd
    (...)
    /home/vstinner/python/main

    Maybe subst() can be modified to work on a list (as Bernd Dietzel proposed) and then use subprocess to avoid shell and so avoid having to pass a single string, but pass a *list*
    of arguments (strings).

    The problem is that it would change the public mailcap.findmatch() API:
    "Return a 2-tuple; the first element is a string containing the command line to be executed (which can be passed to os.system()), (...)"
    https://docs.python.org/dev/library/mailcap.html#mailcap.findmatch

    Adding a new findmatch_list() function avoids the backward compatibility issue, but the existing findmatch() function would remain vulnerable.

    The other problem is that the mailcap.findmatch() function supports "test" command which
    executes os.system() on string created by mailcap.subst().

    Is the mailcap format (RFC 1524) still used in 2022? Does the mailcap module still belong to the Python stdlib in 2022?

    I propose to:

    • (1) Document the shell injection vulnerability: the caller is responsible to validate the filename
    • (2) Deprecate the mailcap module

    A code search in the top 5000 PyPI projects (at 2022-01-26) did not find any Python source code using the "mailcap" module. I only found the word "mailcap" used to refer to other things:

    • https://docs.djangoproject.com/en/4.0/ref/contrib/staticfiles/ mentions a "mailcap" RHEL package:

      "This can be achieved, for example, by installing or updating the mailcap package on a Red Hat distribution, mime-support on a Debian distribution, or by editing the keys under HKEY_CLASSES_ROOT in the Windows registry."

    • wxPython refers to "KDE< mailcap and mime.types"

    https://docs.djangoproject.com/en/4.0/ref/contrib/staticfiles/

    @vstinner vstinner added 3.11 and removed 3.7 labels Apr 6, 2022
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @zooba
    Copy link
    Member

    @zooba zooba commented Apr 13, 2022

    It's too bad this didn't make it into PEP 594, but I think we can deprecate it anyway unless someone steps up to be a maintainer.

    The original patch doesn't add quotes in a way that solves the problem without potentially breaking legitimate users. The best mitigation is to validate user-provided values before using them, in this case, probably by checking that the file exists (nope, checked and it's easy to create a file with an embedded shell command, so users should verify that they trust the incoming path).

    @zooba
    Copy link
    Member

    @zooba zooba commented Apr 13, 2022

    FYI, this issue now has the CVE ID CVE-2015-20107 to assist with discussions.

    Notification at https://mail.python.org/archives/list/security-announce@python.org/thread/QDSXNCW77UGULFG2JMDFZQ7H4DIR32LA/

    @brettcannon
    Copy link
    Member

    @brettcannon brettcannon commented Apr 13, 2022

    I vote to deprecate. I guess we just need to ask on python-dev/committers to see if anyone will maintain it and what the community impact may be?

    @CySirX
    Copy link

    @CySirX CySirX commented Apr 13, 2022

    Hi all, i am the one raised the issue that the security issue was not fixed 7 years ago - you are welcomed to address me regarding the mitigation.
    I've attached some more details..image
    image
    image
    image

    Thanks, Dan

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 docs stdlib type-security
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants
    X Tutup