classification
Title: Consistently handle path separator in Py_GetPath on Windows
Type: security Stage: resolved
Components: Interpreter Core, Windows Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: steve.dower Nosy List: Nam.Nguyen, barry, benjamin.peterson, christian.heimes, eric.araujo, flox, gvanrossum, loewis, mhammond, steve.dower, terry.reedy, tim.golden, vstinner, zach.ware
Priority: normal Keywords: needs review, patch

Created on 2011-09-16 00:08 by Nam.Nguyen, last changed 2019-07-26 16:44 by steve.dower. This issue is now closed.

Files
File name Uploaded Description Edit
getpath.consistent.delim.patch Nam.Nguyen, 2011-09-16 00:08 make path delimiter handling in getpathp.c consistent
Messages (11)
msg144111 - (view) Author: Nam Nguyen (Nam.Nguyen) * Date: 2011-09-16 00:08
The module search path is constructed from PYTHONPATH env-var, then zip path, then HKCU PythonPath, then HKLM PythonPath, then PYTHONPATH define (in pyconfig.h), and finally argv[0]. If PYTHONHOME is available, the PYTHONPATH define is expanded. These paths are separated by semicolon.

Without PYTHONHOME, PYTHONPATH define is appended to module_search_path as-is, and a semicolon comes **after** that. With PYTHONHOME, PYTHONPATH define is expanded, and there is no semicolon after it. Then, finally, when argv[0] is added to module_search_path, a semicolon is **prepended** before it.

This inconsistency in handling path delimiter leads to a case where two semicolons are next to each other (;;), which is translated to the current directory. It happens when PYTHONHOME is not found. The current directory is put in front of the application directory (argv[0]) causing a security issue whereby external modules might be imported inadvertently.

This patch makes semicolon handling consistent. A semicolon is appended at the end of every path component, except argv[0].
msg144161 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-09-16 21:00
Barry, Benjamin, do you agree that this is a security issue as far as future 2.6 and 3.1 security fix releases are concerned?
msg144304 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2011-09-19 17:45
I'm not Barry or Benjamin, but having followed the thread on psrt@python.org, this certainly looks like a security issue to me. As a second pair of eyes, I recommend MvL, who builds our Windows installers.
msg144305 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2011-09-19 17:50
Approved for 3.1 as far as I'm concerned.
msg144311 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-09-19 21:36
Brian, you marked this 'patch review', bypassing 'test needed'. Should this have any visible effect at the Python level that can be tested?
msg145989 - (view) Author: Mark Hammond (mhammond) * (Python committer) Date: 2011-10-19 23:01
The first chunk of that patch is for when pythonhome==NULL.  There is also a similar block just under it when MS_WINDOWS is not defined.  While I don't know in which cases this will be built without that define, it looks as though the *buf++ = DELIM; should also be added to that block?

Also, there is an existing conditional:
     if (argv0_path) {

which can never be false (as argv0_path is a char array).  It could be changed to if (argv0_path[0]) but ISTM that it could also be removed completely - ie, the 2 lines left in that block should have no effect in the case where the buffer is empty.

I haven't actually tested it though, but apart from the first comment above, it *looks* like it does the right thing :)
msg220205 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-10 22:31
If this is a security issue shouldn't it have been actioned?  If not does the type move to behaviour or what?
msg220242 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-06-11 07:10
Hum, it would be nice to have a unit test for this change.
msg236995 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2015-03-01 23:17
The patch is three parts that adds one line, moves one line and deletes one line.  I've checked 2.6, 2.7, 3.2, 3.3, 3.4 and default.  In all cases the second part has already been implemented, the first and third have not.  Assuming that these changes must still be done, do we also need additional unit tests?
msg277327 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2016-09-24 19:08
Steve, is this bug still relevant and a security problem?
msg348507 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-07-26 16:44
This code has been significantly rewritten since this bug, and I believe it's no longer an issue.
History
Date User Action Args
2019-07-26 16:44:42steve.dowersetstatus: open -> closed
resolution: out of date
messages: + msg348507

stage: patch review -> resolved
2016-09-24 19:25:50BreamoreBoysetnosy: - BreamoreBoy
2016-09-24 19:08:55christian.heimessetversions: + Python 3.6, Python 3.7, - Python 3.2, Python 3.3, Python 3.4
nosy: + christian.heimes

messages: + msg277327

assignee: steve.dower
2015-03-01 23:17:21BreamoreBoysetnosy: + tim.golden, zach.ware, steve.dower
messages: + msg236995
2014-06-11 07:10:20vstinnersetmessages: + msg220242
2014-06-10 23:51:50terry.reedysetversions: + Python 3.4, Python 3.5, - Python 2.6, Python 3.1
2014-06-10 22:31:17BreamoreBoysetnosy: + BreamoreBoy
messages: + msg220205
2012-01-07 15:40:49eric.araujosetnosy: + eric.araujo
2011-10-19 23:01:13mhammondsetnosy: + mhammond
messages: + msg145989
2011-10-19 17:37:03floxsetnosy: + flox
2011-10-19 16:23:49vstinnersetnosy: + vstinner
2011-09-19 21:36:54terry.reedysetmessages: + msg144311
2011-09-19 17:50:36benjamin.petersonsetmessages: + msg144305
2011-09-19 17:45:53gvanrossumsetnosy: + gvanrossum, loewis
messages: + msg144304
2011-09-16 21:00:01terry.reedysetnosy: + barry, terry.reedy, benjamin.peterson
messages: + msg144161
2011-09-16 02:38:03brian.curtinsetkeywords: + needs review
stage: patch review
type: security
versions: - Python 3.4
2011-09-16 00:08:15Nam.Nguyencreate