classification
Title: mailcap.findmatch: document shell command Injection danger in filename parameter
Type: security Stage:
Components: Documentation, Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: docs@python Nosy List: TheRegRunner, docs@python, r.david.murray
Priority: normal Keywords:

Created on 2015-08-02 08:25 by TheRegRunner, last changed 2016-09-24 19:22 by christian.heimes.

Files
File name Uploaded Description Edit
screenshot.png TheRegRunner, 2015-08-02 08:25
The Quote Problem.py TheRegRunner, 2015-08-03 19:27
mailcap patch.zip TheRegRunner, 2015-10-29 19:26 mailcap.py patches and diffs for python2.7 and python 3.5
Messages (13)
msg247857 - (view) Author: Bernd Dietzel (TheRegRunner) Date: 2015-08-02 08:25
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
msg247861 - (view) Author: Bernd Dietzel (TheRegRunner) Date: 2015-08-02 10:16
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 : ~ €
msg247944 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-03 18:44
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.)
msg247946 - (view) Author: Bernd Dietzel (TheRegRunner) Date: 2015-08-03 19:27
@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.
msg247951 - (view) Author: Bernd Dietzel (TheRegRunner) Date: 2015-08-03 20:31
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
msg247979 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-04 03:08
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
msg247992 - (view) Author: Bernd Dietzel (TheRegRunner) Date: 2015-08-04 18:42
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
msg248058 - (view) Author: Bernd Dietzel (TheRegRunner) Date: 2015-08-05 18:58
# 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'
msg248061 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-05 19:18
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.
msg248062 - (view) Author: Bernd Dietzel (TheRegRunner) Date: 2015-08-05 19:32
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
msg248070 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-05 20:06
Ah, that's a clever idea.
msg248074 - (view) Author: Bernd Dietzel (TheRegRunner) Date: 2015-08-05 20:26
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.
msg253689 - (view) Author: Bernd Dietzel (TheRegRunner) Date: 2015-10-29 19:26
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.
History
Date User Action Args
2016-09-24 19:22:58christian.heimessetversions: + Python 3.7, - Python 3.4
2015-10-29 19:26:37TheRegRunnersetfiles: + mailcap patch.zip

messages: + msg253689
2015-08-05 20:26:43TheRegRunnersetmessages: + msg248074
2015-08-05 20:06:33r.david.murraysetmessages: + msg248070
2015-08-05 19:32:42TheRegRunnersetmessages: + msg248062
2015-08-05 19:18:59r.david.murraysetmessages: + msg248061
title: mailcap.findmatch() ........ Shell Command Injection in filename -> mailcap.findmatch: document shell command Injection danger in filename parameter
2015-08-05 18:58:52TheRegRunnersetmessages: + msg248058
2015-08-04 18:42:11TheRegRunnersetmessages: + msg247992
2015-08-04 03:08:56r.david.murraysetnosy: + docs@python
messages: + msg247979

assignee: docs@python
components: + Documentation
2015-08-03 20:31:07TheRegRunnersetmessages: + msg247951
2015-08-03 19:27:35TheRegRunnersetfiles: + The Quote Problem.py

messages: + msg247946
2015-08-03 18:44:10r.david.murraysetnosy: + r.david.murray

messages: + msg247944
versions: + Python 3.4, Python 3.5, Python 3.6
2015-08-02 10:16:11TheRegRunnersetmessages: + msg247861
2015-08-02 08:25:07TheRegRunnercreate