classification
Title: distutils : file "bdist_rpm.py" does not quote filenames when executing the rpm command
Type: behavior Stage: resolved
Components: Distutils Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: SilentGhost, TheRegRunner, dpward, dstufft, eric.araujo, martin.panter, r.david.murray, steve.dower
Priority: normal Keywords: patch

Created on 2015-11-14 21:13 by TheRegRunner, last changed 2021-02-03 18:26 by steve.dower. This issue is now closed.

Files
File name Uploaded Description Edit
setup.py TheRegRunner, 2015-11-14 21:13 Exploit demo setup.py script with a Shell command in "name"
issue25627.diff SilentGhost, 2015-11-14 22:04 review
issue25627_3.diff SilentGhost, 2015-11-29 12:43 review
issue25627_4.diff SilentGhost, 2015-11-30 06:56 review
issue25627_5.diff SilentGhost, 2015-11-30 11:14 review
issue25627_6.diff dpward, 2016-04-12 00:45 review
Messages (13)
msg254670 - (view) Author: Bernd Dietzel (TheRegRunner) Date: 2015-11-14 21:13
https://bugs.launchpad.net/ubuntu/+source/python2.7/+bug/1514183

File :
/usr/lib/python2.7/distutils/command/bdist_rpm.py

Line 358 :
This line in the code uses the depreached os.popen command, should be replaced with subprocess.Popen() :

out = os.popen(q_cmd)

Exploit demo :
============
1) Download the setup.py script witch i attached
2) Create a test folder an put the setup.py script in this folder
3) cd to the test folder
4) python setup.py bdist_rpm
5) A xmessage window pops up as a proof of concept
msg254671 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2015-11-14 22:04
This also seem to affect python 3, there os.popen implemented using subprocess.Popen, but that one is called with shell=True. So basically the string that's passed to os.popen should be quoted. The attached patch seem to be sufficient when applied on the default branch.
msg254691 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-11-15 19:36
Since setup.py can run arbitrary python code, it is pointless to worry about this from a security perspective.  The change is otherwise not a bad idea, though, since it avoids filename quoting problems.  Is there any chance this would break existing setup.py files that do their own quoting of the filenames to get around the quoting problem?  I'm guessing not since the filename gets used in multiple contexts, and the other contexts probably require an unquoted filename. Which would make this a simple bug fix against bdist_rpm.

However, why not convert to using Popen?
msg255568 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2015-11-29 11:09
I tried re-writing that bit using subprocess.Popen but since the 2.7 support is needed I genuinely don't see any benefit that would add on top of the submitted patch.
msg255570 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-29 11:39
I don’t understand what 2.7 has got to do with using subprocess or not. Jumping through a shell quoting hoop only to pass a command to the shell to be unquoted seems like double handling. This is how I might rewrite it (untested):

q_cmd = ("rpm", "-q", "--qf", r"%s %s\n" % (...), "--specfile", spec_path)
with subprocess.Popen(q_cmd, stdout=PIPE, universal_newlines=?) as proc:
    for line in proc.stdout:
        ...
if proc.returncode:
    raise DistutilsExecError(...)
msg255572 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2015-11-29 12:43
Yeah, it would be great, Martin, if only that code worked in python2. Anyway, here is an alternative patch.
msg255601 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-29 23:05
Okay so now I see 2.7 doesn’t support the context manager. I was mainly curious why you didn’t want to use subprocess. However a bigger problem with your first patch is shlex.quote() does not appear to be in Python 2 either.

If you want to use proc.communicate() rather than reading line by line, perhaps subprocess.check_output() would be simpler. I’m not familiar with the RPM command but if there is only a modest amount of output it should be good enough.
msg255612 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2015-11-30 06:56
OK, here is seemingly python2-proof patch. At least it shouldn't have glaring syntax errors on python2 (as the previous one did). But I'd appreciate if some RPM people could actually run the test.
msg255616 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-30 10:36
I left some comments on the code review. I think this definitely needs someone able to test it; I don’t think I am up to it.
msg255618 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2015-11-30 11:14
Updated version of the patch including Martin's suggestions.
msg258478 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2016-01-17 19:34
Eric, Donald, is any one of you able to test this?
msg263213 - (view) Author: David Ward (dpward) * Date: 2016-04-12 00:45
This revised patch has a small change so that the subprocess output is decoded from a byte sequence to a string, which is necessary when running this under Python 3.

With this change, this worked for me on Fedora 23 with Python 3.4.3.

It also worked on Fedora 23 with Python 2.7.11, but I had to apply the patch by hand, because of small differences in the original file as a result of SVN revisions 54854 and 57699 for Python 3.0. I also had to add "import subprocess" to the top of this file.
msg386375 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-02-03 18:26
Distutils is now deprecated (see PEP 632) and all tagged issues are being closed. From now until removal, only release blocking issues will be considered for distutils.

If this issue does not relate to distutils, please remove the component and reopen it. If you believe it still requires a fix, most likely the issue should be re-reported at https://github.com/pypa/setuptools
History
Date User Action Args
2021-02-03 18:26:17steve.dowersetstatus: open -> closed

nosy: + steve.dower
messages: + msg386375

resolution: out of date
stage: patch review -> resolved
2016-04-12 00:45:17dpwardsetfiles: + issue25627_6.diff
nosy: + dpward
messages: + msg263213

2016-01-17 19:34:40SilentGhostsetmessages: + msg258478
2015-11-30 11:14:14SilentGhostsetfiles: + issue25627_5.diff

messages: + msg255618
2015-11-30 10:36:29martin.pantersetmessages: + msg255616
2015-11-30 06:56:54SilentGhostsetfiles: + issue25627_4.diff

messages: + msg255612
2015-11-29 23:05:32martin.pantersetmessages: + msg255601
2015-11-29 12:43:38SilentGhostsetfiles: + issue25627_3.diff

messages: + msg255572
2015-11-29 11:39:52martin.pantersetnosy: + martin.panter

messages: + msg255570
stage: patch review
2015-11-29 11:09:36SilentGhostsetmessages: + msg255568
2015-11-15 19:37:31r.david.murraysettitle: distutils : file "bdist_rpm.py" allows Shell injection in "name" -> distutils : file "bdist_rpm.py" does not quote filenames when executing the rpm command
2015-11-15 19:36:55r.david.murraysetversions: + Python 3.5, Python 3.6
nosy: + r.david.murray

messages: + msg254691

type: security -> behavior
2015-11-14 22:04:18SilentGhostsetfiles: + issue25627.diff

nosy: + SilentGhost
messages: + msg254671

keywords: + patch
2015-11-14 21:16:14TheRegRunnersettitle: distutils : file "bdist_rpm.py" allows Shell injection in "name -> distutils : file "bdist_rpm.py" allows Shell injection in "name"
2015-11-14 21:13:32TheRegRunnercreate