Issue 16112: platform.architecture does not correctly escape argument to /usr/bin/file (original) (raw)
Created on 2012-10-02 19:15 by David.Benjamin, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Messages (36)
Author: David Benjamin (David.Benjamin)
Date: 2012-10-02 19:15
The implementation of platform.architecture shells out to the file command. It tries to escape quotes by replacing " with ", but that's not sufficient.
$ python3.2 -c 'import platform; platform.architecture("foo\"; echo Hi there > /tmp/Z; echo \"")' && cat /tmp/Z Hi there
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.
Author: Jesús Cea Avión (jcea) * 
Date: 2012-10-04 11:23
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.
Author: Roundup Robot (python-dev) 
Date: 2012-10-04 11:58
New changeset c73b90b6dadd by Jesus Cea in branch '2.7': Closes #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 #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 #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 #16112: platform.architecture does not correctly escape argument to /usr/bin/file http://hg.python.org/cpython/rev/cd026866b333
Author: Jesús Cea Avión (jcea) *
Date: 2012-10-04 12:21
I have a bootstrap problem with python 2.7. Backout that patch, for now.
Author: Roundup Robot (python-dev)
Date: 2012-10-04 13:15
New changeset d6d908dc11f2 by Jesus Cea in branch '2.7': Closes #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
Author: Jesús Cea Avión (jcea) *
Date: 2012-10-04 13:21
Thanks for bringing this up and for your patch.
Author: STINNER Victor (vstinner) *
Date: 2012-10-04 13:30
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 <victor.stinner@haypocalc.com> 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()
- Popen() avoids ugly shell escape: target.replace('"', '\"')
- Use proc.communicate() instead of f.stdout.read()
- Get output from stdout by splitting with ": " instead of splitting by spaces to support filename with spaces
Author: STINNER Victor (vstinner) *
Date: 2012-10-04 13:35
See also issue #9560 and the following email from Marc-Andre Lemburg. http://mail.python.org/pipermail/python-checkins/2010-April/092099.html
"Viktor, before making such changes to platform.py, please coordinate with me, as I am the maintainer of that module.
The subprocess change is not OK, since platform.py has to stay compatible with Python 2.3 which doesn't have subprocess. Please either revert the change or make it fallback to os.popen()."
Author: Jesús Cea Avión (jcea) *
Date: 2012-10-04 14:46
Thanks for the heads-up, Victor.
I have added Marc-Andre Lemburg to the nosy list, so he can know about this issue and can provide feedback (or request a backout for 2.7).
Marc-Andre?.
Author: Antoine Pitrou (pitrou) *
Date: 2012-10-04 18:00
This change broke test_platform for me:
====================================================================== ERROR: test_architecture (test.test_platform.PlatformTest)
Traceback (most recent call last): File "/home/antoine/cpython/32/Lib/test/test_platform.py", line 11, in test_architecture res = platform.architecture() File "/home/antoine/cpython/32/Lib/platform.py", line 1072, in architecture if 'executable' not in fileout: TypeError: Type str doesn't support the buffer API
Author: STINNER Victor (vstinner) *
Date: 2012-10-04 22:11
Reopen: test_platform is failing. Attached patch should fix the issue. It fixes a theoric "deadlock" issue in _syscmd_file: use proc.communicate() instead of proc.stdout.read().
Author: David Benjamin (David.Benjamin)
Date: 2012-10-04 23:00
Well, the theoretical deadlock's just if stdin is also a pipe, right? I think there isn't be a difference between communicate and stdout.read if only stdout is a pipe. Though it might be worth passing DEVNULL to stdin instead of inheriting, just to be tidy.
Author: STINNER Victor (vstinner) *
Date: 2012-10-04 23:19
Well, the theoretical deadlock's just if stdin is also a pipe, right?
If I remember correctly, the parent process (Python) and the child process (file) can be blocked if the child is blocked in a blocking write into a pipe (ex: stderr), whereas the pipe buffer is full (the size of the buffer is usually 4096 bytes) and the parent is reading all content of another pipe (ex: stdout). So it occurs if the process uses more than 1 pipe for standard input/output streams. In the case of platform, it looks like the issue cannot occur.
I always prefer communicate() because I consider it safer :-)
Though it might be worth passing DEVNULL to stdin instead of inheriting, just to be tidy.
Yeah, maybe. As you want.
Author: Antoine Pitrou (pitrou) *
Date: 2012-10-04 23:19
Victor: your patch doesn't apply on 3.2, but it works for me on 3.3.
Author: Antoine Pitrou (pitrou) *
Date: 2012-10-04 23:21
Victor is right, communicate() is usually the right way to do it. No need to use unsafe idioms.
Author: STINNER Victor (vstinner) *
Date: 2012-10-04 23:21
Victor: your patch doesn't apply on 3.2, but it works for me on 3.3.
I wrote it for Python 3.4. I'm too lazy for adapt it to other branches.
Author: David Benjamin (David.Benjamin)
Date: 2012-10-04 23:37
Yes, communicate is needed if you have multiple pipes and need to be careful about both ends doing a blocking reads/writes on different ones. There's only one pipe here. Eh, whatever. If you guys really want to use communicate, I don't really care.
Author: Roundup Robot (python-dev)
Date: 2012-10-05 02:50
New changeset b94a9ff13199 by Jesus Cea in branch '2.7': #16112: platform.architecture does not correctly escape argument to /usr/bin/file. Use 'communicate()' http://hg.python.org/cpython/rev/b94a9ff13199
Author: Roundup Robot (python-dev)
Date: 2012-10-05 03:19
New changeset 04f39958aea9 by Jesus Cea in branch '3.2': #16112: platform.architecture does not correctly escape argument to /usr/bin/file. Use 'communicate()' and decode the bytes http://hg.python.org/cpython/rev/04f39958aea9
New changeset 19d37c8d1882 by Jesus Cea in branch '2.7': #16112: platform.architecture does not correctly escape argument to /usr/bin/file. Fix original patch http://hg.python.org/cpython/rev/19d37c8d1882
Author: Roundup Robot (python-dev)
Date: 2012-10-05 03:37
New changeset 9838ae397a19 by Jesus Cea in branch '3.2': #16112: platform.architecture does not correctly escape argument to /usr/bin/file. Fix original patch http://hg.python.org/cpython/rev/9838ae397a19
New changeset 64a0caf49429 by Jesus Cea in branch '3.3': MERGE: #16112: platform.architecture does not correctly escape argument to /usr/bin/file. Fix original patch http://hg.python.org/cpython/rev/64a0caf49429
New changeset b9ac3c44a4eb by Jesus Cea in branch 'default': MERGE: #16112: platform.architecture does not correctly escape argument to /usr/bin/file. Fix original patch http://hg.python.org/cpython/rev/b9ac3c44a4eb
Author: Jesús Cea Avión (jcea) *
Date: 2012-10-05 03:38
David, after so many patches, can you confirm that we have solved the original issue? :)
Author: Jesús Cea Avión (jcea) *
Date: 2012-10-05 03:39
Why I was not seeing "test_platform.py" failing in the buildbots? :-?
Author: Marc-Andre Lemburg (lemburg) *
Date: 2012-10-05 06:33
Jesús Cea Avión wrote:
Jesús Cea Avión added the comment:
Thanks for the heads-up, Victor.
I have added Marc-Andre Lemburg to the nosy list, so he can know about this issue and can provide feedback (or request a backout for 2.7).
Marc-Andre?.
The comment that Viktor posted still stands for Python 2.7.
You can use subprocess in platform for Python 2.7, but only if it's available. Otherwise the module must fall back to the portable popen() that comes with the platform module.
It may be worth adding that selection process to the popen() function in platform itself.
For Python 3.x, you can use subprocess.
Thanks,
Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Source (#1, Oct 05 2012)
Python Projects, Consulting and Support ... http://www.egenix.com/ mxODBC.Zope/Plone.Database.Adapter ... http://zope.egenix.com/ mxODBC, mxDateTime, mxTextTools ... http://python.egenix.com/
2012-09-27: Released eGenix PyRun 1.1.0 ... http://egenix.com/go35 2012-09-26: Released mxODBC.Connect 2.0.1 ... http://egenix.com/go34 2012-09-25: Released mxODBC 3.2.1 ... http://egenix.com/go33 2012-10-23: Python Meeting Duesseldorf ... 18 days to go
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/
Author: Marc-Andre Lemburg (lemburg) *
Date: 2012-10-05 06:50
The implementation of platform.architecture shells out to the file command. It tries to escape quotes by replacing " with ", but that's not sufficient.
$ python3.2 -c 'import platform; platform.architecture("foo\"; echo Hi there > /tmp/Z; echo \"")' && cat /tmp/Z Hi there
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.
I think a much better patch would be to test for existence of the file in question. File names rarely use any of the mentioned quoting and most certainly not for an executable, so if the check fails, that's a good indication that something is not right.
Perhaps such a check could be added in addition to the other things in the patch ?
BTW: It's probably better to discuss such patches on the tracker first, before applying them to the code base. It becomes difficult discussing patches that have already been partially applied to the code.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Source (#1, Oct 05 2012)
Python Projects, Consulting and Support ... http://www.egenix.com/ mxODBC.Zope/Plone.Database.Adapter ... http://zope.egenix.com/ mxODBC, mxDateTime, mxTextTools ... http://python.egenix.com/
2012-09-27: Released eGenix PyRun 1.1.0 ... http://egenix.com/go35 2012-09-26: Released mxODBC.Connect 2.0.1 ... http://egenix.com/go34 2012-09-25: Released mxODBC 3.2.1 ... http://egenix.com/go33 2012-10-23: Python Meeting Duesseldorf ... 18 days to go
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/
Author: STINNER Victor (vstinner) *
Date: 2012-10-05 07:14
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!?)
Author: STINNER Victor (vstinner) *
Date: 2012-10-05 07:18
You can use subprocess in platform for Python 2.7, but only if it's available. Otherwise the module must fall back to the portable popen() that comes with the platform module.
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.
Author: Antoine Pitrou (pitrou) *
Date: 2012-10-05 12:05
BTW: It's probably better to discuss such patches on the tracker first, before applying them to the code base. It becomes difficult discussing patches that have already been partially applied to the code.
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.
Author: Jesús Cea Avión (jcea) *
Date: 2012-10-05 14:11
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 :-?
Author: Marc-Andre Lemburg (lemburg) *
Date: 2012-10-06 07:24
Jesús Cea Avión wrote:
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?.
At least for Python 2.7, I think that would be a nice and simple solution.
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 :-?
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
Professional Python Services directly from the Source (#1, Oct 06 2012)
Python Projects, Consulting and Support ... http://www.egenix.com/ mxODBC.Zope/Plone.Database.Adapter ... http://zope.egenix.com/ mxODBC, mxDateTime, mxTextTools ... http://python.egenix.com/
2012-09-27: Released eGenix PyRun 1.1.0 ... http://egenix.com/go35 2012-09-26: Released mxODBC.Connect 2.0.1 ... http://egenix.com/go34 2012-09-25: Released mxODBC 3.2.1 ... http://egenix.com/go33 2012-10-23: Python Meeting Duesseldorf ... 17 days to go
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/
Author: STINNER Victor (vstinner) *
Date: 2012-10-09 21:34
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.
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.
Author: Marc-Andre Lemburg (lemburg) *
Date: 2012-10-18 12:52
STINNER Victor wrote:
STINNER Victor added the comment:
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.
Which projects use their own copy of platform.py? Where does this file come from?
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.
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.
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
Professional Python Services directly from the Source (#1, Oct 18 2012)
Python Projects, Consulting and Support ... http://www.egenix.com/ mxODBC.Zope/Plone.Database.Adapter ... http://zope.egenix.com/ mxODBC, mxDateTime, mxTextTools ... http://python.egenix.com/
2012-09-27: Released eGenix PyRun 1.1.0 ... http://egenix.com/go35 2012-09-26: Released mxODBC.Connect 2.0.1 ... http://egenix.com/go34 2012-09-25: Released mxODBC 3.2.1 ... http://egenix.com/go33 2012-10-23: Python Meeting Duesseldorf ... 5 days to go
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/
Author: Christian Heimes (christian.heimes) *
Date: 2016-09-08 23:41
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.
Author: STINNER Victor (vstinner) *
Date: 2016-09-09 04:51
Christian: would you mind to review my old patch (and rebase it if needed)?
Author: Marc-Andre Lemburg (lemburg) *
Date: 2016-09-09 07:26
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.
Author: STINNER Victor (vstinner) *
Date: 2016-09-09 07:34
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.
Author: STINNER Victor (vstinner) *
Date: 2016-09-09 07:38
Note: Other platform functions still use os.popen(), you might open new issues.
_syscmd_ver:
for cmd in ('ver', 'command /c ver', 'cmd /c ver'):
try:
pipe = os.popen(cmd)
...def _syscmd_uname(option, default=''): ... f = os.popen('uname %s 2> %s' % (option, DEV_NULL))
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).
History
Date
User
Action
Args
2022-04-11 14:57:36
admin
set
github: 60316
2016-09-09 07:38:04
vstinner
set
messages: +
2016-09-09 07:34:22
vstinner
set
status: open -> closed
resolution: fixed
messages: +
2016-09-09 07:26:04
lemburg
set
messages: +
2016-09-09 04:51:52
vstinner
set
messages: +
2016-09-08 23:41:26
christian.heimes
set
nosy: + christian.heimes
messages: +
stage: resolved -> needs patch
2012-10-18 12:52:02
lemburg
set
messages: +
2012-10-09 21:34:52
vstinner
set
messages: +
2012-10-07 02:53:57
Arfrever
set
nosy: + Arfrever
2012-10-06 07:24:54
lemburg
set
messages: +
2012-10-05 14:11:47
jcea
set
messages: +
2012-10-05 12:05:28
pitrou
set
messages: +
2012-10-05 07🔞33
vstinner
set
status: closed -> open
resolution: fixed -> (no value)
2012-10-05 07🔞03
vstinner
set
messages: +
2012-10-05 07:14:51
vstinner
set
messages: +
2012-10-05 06:50:30
lemburg
set
messages: +
2012-10-05 06:33:53
lemburg
set
messages: +
2012-10-05 03:39:28
jcea
set
messages: +
2012-10-05 03:38:18
jcea
set
status: open -> closed
resolution: fixed
messages: +
2012-10-05 03:37:18
python-dev
set
messages: +
2012-10-05 03:19:04
python-dev
set
messages: +
2012-10-05 02:50:43
python-dev
set
messages: +
2012-10-04 23:37:29
David.Benjamin
set
messages: +
2012-10-04 23:21:16
vstinner
set
messages: +
2012-10-04 23:21:01
pitrou
set
messages: +
2012-10-04 23:19:47
pitrou
set
messages: +
2012-10-04 23:19:43
vstinner
set
messages: +
2012-10-04 23:00:33
David.Benjamin
set
messages: +
2012-10-04 22:11:28
vstinner
set
files: + platform.patch
resolution: fixed -> (no value)
messages: +
2012-10-04 18:00:19
pitrou
set
status: closed -> open
nosy: + pitrou
messages: +
2012-10-04 14:46:39
jcea
set
nosy: + lemburg
messages: +
2012-10-04 13:35:03
vstinner
set
messages: +
2012-10-04 13:30:58
vstinner
set
nosy: + vstinner
messages: +
2012-10-04 13:21:49
jcea
set
status: open -> closed
resolution: fixed
2012-10-04 13:21:26
jcea
set
status: closed -> open
resolution: fixed -> (no value)
messages: +
2012-10-04 13:15:09
python-dev
set
status: open -> closed
resolution: fixed
messages: +
2012-10-04 12:21:19
jcea
set
status: closed -> open
resolution: fixed -> (no value)
messages: +
2012-10-04 11:58:41
python-dev
set
status: open -> closed
nosy: + python-dev
messages: +
resolution: fixed
stage: resolved
2012-10-04 11:23:31
jcea
set
versions: - Python 2.6, Python 3.1, Python 3.5
nosy: + jcea
messages: +
assignee: jcea
2012-10-02 19:15:16
David.Benjamin
create