Repository navigation
platform.architecture does not correctly escape argument to /usr/bin/file #60316
Description
| BPO | 16112 |
|---|---|
| Nosy | @malemburg, @jcea, @pitrou, @vstinner, @tiran |
| Files |
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 = 'https://github.com/jcea'
closed_at = <Date 2016-09-09.07:34:22.855>
created_at = <Date 2012-10-02.19:15:16.864>
labels = ['type-security', 'library']
title = 'platform.architecture does not correctly escape argument to /usr/bin/file'
updated_at = <Date 2016-09-09.07:38:04.571>
user = 'https://bugs.python.org/DavidBenjamin'bugs.python.org fields:
activity = <Date 2016-09-09.07:38:04.571>
actor = 'vstinner'
assignee = 'jcea'
closed = True
closed_date = <Date 2016-09-09.07:34:22.855>
closer = 'vstinner'
components = ['Library (Lib)']
creation = <Date 2012-10-02.19:15:16.864>
creator = 'David.Benjamin'
dependencies = []
files = ['27391', '27427']
hgrepos = []
issue_num = 16112
keywords = ['patch']
message_count = 36.0
messages = ['171825', '171934', '171936', '171937', '171939', '171940', '171942', '171944', '171957', '171982', '171999', '172017', '172021', '172022', '172023', '172024', '172026', '172043', '172044', '172045', '172046', '172047', '172050', '172052', '172055', '172057', '172075', '172085', '172172', '172533', '173265', '275212', '275254', '275278', '275280', '275281']
nosy_count = 8.0
nosy_names = ['lemburg', 'jcea', 'pitrou', 'vstinner', 'christian.heimes', 'Arfrever', 'python-dev', 'David.Benjamin']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'needs patch'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue16112'
versions = ['Python 2.7', 'Python 3.2', 'Python 3.3', 'Python 3.4']
Activity
DavidBenjamin commentedon Oct 2, 2012
The implementation of platform.architecture shells out to the file command. It tries to escape quotes by replacing " with \", but that's not sufficient.
Here's a patch to make it use subprocess instead. I haven't tested it thoroughly building everything from trunk and running tests, but I verified it works by replacing the platform.py in my system Python install.
jcea commentedon Oct 4, 2012
This is security risk, but I can't think about any "platform.architecture()" use that would use a untrusted parameter. Nevertheless "os.popen()" has been deprecated for ages. I take care of this.
python-dev commentedon Oct 4, 2012
New changeset c73b90b6dadd by Jesus Cea in branch '2.7':
Closes bpo-16112: platform.architecture does not correctly escape argument to /usr/bin/file
http://hg.python.org/cpython/rev/c73b90b6dadd
New changeset 6c830b657900 by Jesus Cea in branch '3.2':
Closes bpo-16112: platform.architecture does not correctly escape argument to /usr/bin/file
http://hg.python.org/cpython/rev/6c830b657900
New changeset 3112bf7e0ecb by Jesus Cea in branch '3.3':
MERGE: Closes bpo-16112: platform.architecture does not correctly escape argument to /usr/bin/file
http://hg.python.org/cpython/rev/3112bf7e0ecb
New changeset cd026866b333 by Jesus Cea in branch 'default':
MERGE: Closes bpo-16112: platform.architecture does not correctly escape argument to /usr/bin/file
http://hg.python.org/cpython/rev/cd026866b333
jcea commentedon Oct 4, 2012
I have a bootstrap problem with python 2.7. Backout that patch, for now.
python-dev commentedon Oct 4, 2012
New changeset d6d908dc11f2 by Jesus Cea in branch '2.7':
Closes bpo-16112: platform.architecture does not correctly escape argument to /usr/bin/file. Solve a 2.7 bootstrap issue
http://hg.python.org/cpython/rev/d6d908dc11f2
jcea commentedon Oct 4, 2012
Thanks for bringing this up and for your patch.
vstinner commentedon Oct 4, 2012
I did a similar commit two years ago, but I reverted it because I failed to find a solution for the bootstrap issue.
changeset: 60673:7c90ac194e40
branch: legacy-trunk
parent: 60664:d7d5c76545fb
user: Victor Stinner <[email protected]>
date: Sun Apr 18 09:07:49 2010 +0000
files: Lib/platform.py
description:
platform: use subprocess.Popen() instead of os.popen() in _syscmd_file()
to support filename with spaces
19 remaining items
vstinner commentedon Oct 5, 2012
1.7 - with open(DEV_NULL) as dev_null:
1.8 - proc = subprocess.Popen(['file', '-b', '--', target],
1.9 - stdout=subprocess.PIPE,
stderr=dev_null)
1.9 + proc = subprocess.Popen(['file', target],
1.10 + stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
Errors should be ignored, not written into stderr. subprocess.DEVNULL
was added to Python 3.3, in older version you have to manually call
open the DEV_NULL file.
(Oh by the way, it should be opened in write mode for stderr, not in
read mode!?)
vstinner commentedon Oct 5, 2012
Marc-Andre: I still don't understand why you want to run platform.py
of Python 2.7 on older Python version? How does it happen? I expected
Python 2.3 to use its own version of platform.py.
pitrou commentedon Oct 5, 2012
Agreed with Marc-André. Jesus, please understand that by committing bogus code, you are annoying every other core developer. Be careful and run the test suite before pushing any new code. And waiting for reviews doesn't hurt.
jcea commentedon Oct 5, 2012
Antoine, I agree. I beg your pardon. This patch was suppose to be quite trivial, and "test_platform" passes just fine on my Linux and Solaris computers. And the four buildbots I was monitoring, the testsuite passed.
The suggestion of Marc-Andre of simply adding an "os.access()" to check for file existence first is quite sensible and would be enough and compatible in all platforms and python releases.
I am for backing out my patches so far and push a simple "os.access()" check. What do you think?.
Marc-Andre, I was wondering if you could elaborate a bit why 2.7 platform.py file should be able to run fine in 2.3 :-?
malemburg commentedon Oct 6, 2012
Jesús Cea Avión wrote:
At least for Python 2.7, I think that would be a nice and simple
solution.
That only applies to the version in Python 2.x. Python 2.3 is perhaps
a bit extreme and I'd be fine with bumping it to 2.4.
The main reason for keeping the compatibility is that the module is
also being used outside the stdlib for Python versions starting from
2.4 and later. I don't want to maintain two separate versions.
--
Marc-Andre Lemburg
eGenix.com
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48
D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg
Registered at Amtsgericht Duesseldorf: HRB 46611
http://www.egenix.com/company/contact/
vstinner commentedon Oct 9, 2012
Which projects use their own copy of platform.py? Where does this file come from?
Using subprocess sounds safer than adding a (theorical) TOCTTOU issue:
http://en.wikipedia.org/wiki/Time_of_check_to_time_of_use
If you are concerned about compatibility with Python 2.4, you should maintain platform outside the stdlib. The stdlib is regulary updated to the most recent syntax/modules. For example, "yield from" is now used in the stdlib of Python 3.4.
malemburg commentedon Oct 18, 2012
STINNER Victor wrote:
It's originally from the mxCGIPython project I ran a few years ago
and which now lives on in form of the eGenix PyRun project. Since
the main purpose of platform.py is to identify the underlying Python
runtime and OS platform across multiple Python runtimes and OS platforms,
it's meant to always be used in latest version, even against older Python
versions.
The Python 2.4 compatibility requirement only applies to Python 2.x,
not Python 3.x.
Since Python 2.4 does ship with subprocess, I guess we could use
subprocess even in Python 2.7, but I'm not sure whether it's a good
idea to change the code in such a major way for a patch level release.
--
Marc-Andre Lemburg
eGenix.com
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48
D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg
Registered at Amtsgericht Duesseldorf: HRB 46611
http://www.egenix.com/company/contact/
tiran commentedon Sep 8, 2016
I don't see why backwards compatibility with ancient, unsupported Python versions should be more important than security. Let's use the subprocess module here.
MAL, if you need platform.py for some old projects, please vendor it and use a copy.
vstinner commentedon Sep 9, 2016
Christian: would you mind to review my old patch (and rebase it if needed)?
malemburg commentedon Sep 9, 2016
Shouldn't this ticket be closed ?
platform is using subprocess on both the 2.7 and trunk, so the issue should be fixed - and indeed I cannot reproduce it anymore.
vstinner commentedon Sep 9, 2016
Oh right, I didn't read the history of the issue. _syscmd_file() doesn't use popen() anymore, but subprocess, so the issue can now be closed.
vstinner commentedon Sep 9, 2016
Note: Other platform functions still use os.popen(), you might open new issues.
_syscmd_ver:
Note: there is also platform.popen() which is deprecated since Python 3.3, we might also remove it from Python 3.7 as well (I think that it's now too late for Python 3.6).