CVE-2008-5983 python: untrusted python modules search path

@loewis

BPO	5753
Nosy	@loewis, @warsaw, @akuchling, @gpshead, @jcea, @pitrou, @benjaminp, @glyph, @bitdancer, @davidmalcolm
Files	python-CVE-2009-5983.patch: Patch from Ray Strode against python 2.6. py_umspath_test.tar.gz: PoC setargvex.patch setargvex2.patch

^{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 = None
closed_at = <Date 2010-05-21.17:40:59.705>
created_at = <Date 2009-04-14.11:39:38.241>
labels = ['type-security', 'interpreter-core']
title = 'CVE-2008-5983 python: untrusted python modules search path'
updated_at = <Date 2010-09-28.03:25:15.350>
user = 'https://bugs.python.org/iankko'

bugs.python.org fields:

activity = <Date 2010-09-28.03:25:15.350>
actor = 'jcea'
assignee = 'none'
closed = True
closed_date = <Date 2010-05-21.17:40:59.705>
closer = 'pitrou'
components = ['Interpreter Core']
creation = <Date 2009-04-14.11:39:38.241>
creator = 'iankko'
dependencies = []
files = ['13685', '13686', '13860', '17418']
hgrepos = []
issue_num = 5753
keywords = ['patch']
message_count = 49.0
messages = ['85965', '85966', '85967', '85968', '86904', '86906', '86927', '86943', '87061', '87063', '87083', '87084', '87212', '87239', '87281', '87285', '87299', '87300', '87309', '87343', '87347', '87348', '87350', '87399', '87556', '89688', '90329', '90330', '90336', '90448', '90473', '90480', '90481', '90543', '90556', '98027', '104927', '104939', '104950', '105945', '105976', '105980', '106184', '106214', '106221', '106256', '107508', '107515', '117504']
nosy_count = 14.0
nosy_names = ['loewis', 'barry', 'akuchling', 'gregory.p.smith', 'jcea', 'pitrou', 'benjamin.peterson', 'glyph', 'psss', 'r.david.murray', 'iankko', 'akr', 'thoger', 'dmalcolm']
pr_nums = []
priority = 'critical'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue5753'
versions = ['Python 2.6', 'Python 3.1', 'Python 2.7', 'Python 3.2']

Common Vulnerabilities and Exposures assigned an identifier
CVE-2008-5983 (and related CVE ids) to the following vulnerability:

Untrusted search path vulnerability in the PySys_SetArgv API function in
Python 2.6 and earlier, and possibly later versions, prepends an empty
string to sys.path when the argv[0] argument does not contain a path
separator, which might allow local users to execute arbitrary code via a
Trojan horse Python file in the current working directory.

To sum up the behavior, the following table displays whether
modules are read from the current working directory for various
ways how the python scripts can be launched (unfixed/fixed version):

unfixed   fixed   run as
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
yes       no      python test.py
yes       no      python ./test.py
yes       no      python /tmp/396/test.py
yes       no      /bin/env python test.py

yes       yes     test.py
yes       yes     ./test.py
yes       yes     /tmp/396/test.py
yes       yes     /usr/bin/python test.py
yes       yes     /usr/bin/python ./test.py
yes       yes     /usr/bin/python /tmp/396/test.py

no        no      test-in-different-dir.py
no        no      ./bin/test-in-different-dir.py
no        no      python ./bin/test-in-different-dir.py

As no longer work of "python ./foo.py" after patch utilization may
cause, the update won't be acceptable, could you guys review the
above patch and potentially provide an another one?

Just drop into /tmp and run (you will need the zenity package installed):

python3.1 ./test.py

or

gedit # unfixed gedit

in that directory.

What is the problem exactly?
An user can run arbitrary Python code from a file in his own account --
well, sure, that's a feature. Unless I'm misunderstanding something.

I wanted to read the patch at
https://bugzilla.redhat.com/attachment.cgi?id=334888 but apparently its
access is restricted...

Antoine,

The problem is that apparently every program that embeds Python calls
PySys_SetArgv and does not understand the consequences of doing so. For
example, a user running 'gedit' to edit some files in a potentially
insecure directory may not expect that starting the program there will
cause it to load python files from that directory.

The 'python' executable itself is not really "vulnerable" in quite the
same way, because if you (i.e. a developer) start 'python' in some
directory, you *do* typically expect that it will load code from that
directory. For applications written *in* python, that have scripts in,
let's say, /usr/bin, the directory added to the path is /usr/bin, not
the application's working directory.

I'm not sure we can change the behaviour of PySys_SetArgv() like that.
At least not in a bugfix release.
In 2.7/3.1, we could either change PySys_SetArgv(), or introduce a new
PySys_SetArgvEx() with an additional argument indicating whether
sys.path should be modified or not. I suggest asking on python-dev first.

both the behavior change and PySys_SetArgvEx() with an additional
boolean parameter sounds good to me.

Some people may disagree about changing the default behavior. So long
as its documented in the whatsnew I personally think it is fine. But
would doing that require incrementing the API version number?

+1 on adding a PySys_SetArgvEx() in time for 3.1 (the clock is ticking
fast).

+0 on the existing API default change.

By the way, the advantage of a new function over a behaviour change is
that the new function could safely be backported to 2.6.3, since it is
also a "security fix".

Here is a patch for trunk.

Jan, would the new API be ok to you?

Can anyone move this to Stage: patch review (for the fix approach proposed in msg90336)? Or does anyone have better idea on how to move this closer to final fix or wontfix / reject? Thank you!

Can anyone move this to Stage: patch review (for the fix approach
proposed in msg90336)? Or does anyone have better idea on how to move
this closer to final fix or wontfix / reject? Thank you!

I stand by my opinion that adding another hack in the initialization
path will not do us a lot of good, while a separate API would solve the
problem neatly. Perhaps Dave Malcolm can chime in?

FWIW I agree with Antoine.

Attempting to summarize IRC discussion about this.

PySys_SetArgv is used to set up sys.argv There is plenty of code which assumes that this is a list containing at least a zeroth string element; for example warnings.warn (see msg89688).

It seems reasonable for an program that embeds Python to have no arguments, and for this case, it makes sense for sys.argv to be [""] (i.e. a list containing a single empty string).

However, in this case, it doesn't necessarily make sense to prepend the empty string to the front of sys.path

Looking through Python/sysmodule.c: if argc is 0 in the call to PySys_SetArgv, it looks like makeargvobject makes sys.argv be [""] (which is good), but it looks like it uses argc[0] (as "argv") to prepend sys.path.

My reading of PySys_SetArgv is that if argv is NULL, then "char *argv0 = argv[0];" will read through NULL and thus will segfault on a typical platform.

So one possible way to handle this might be to support PySys_SetArgv(0, NULL) as signifying that sys.argv should be set to [""] with no modification of sys.path

This Google code search for "pysys_setargv(0" shows 25 hits:
http://www.google.com/codesearch?hl=en&lr=&q=pysys_setargv\\(0&sbtn=Search

Hoever, the function is complicated, and adding more special-cases seems error-prone.

I favor Antoine's approach in http://bugs.python.org/file13860/setargvex.patch of adding a new API entry point, whilst maximizing compatibilty for all of the code our there using the existing entry point.

I think that both the old and the new entry point need to have better documentation, in particular, spelling out the meaning of the args, what the effect of argc==0 is, and that argv must be non-NULL in the old entry point, but may be NULL for argc==0 in the new entry point (assuming that I'm reading that correctly).

Ok, I will try to write better documentation.

My reading of PySys_SetArgv is that if argv is NULL, then
"char *argv0 = argv[0];" will read through NULL and thus will
segfault on a typical platform.

Right.

I favor Antoine's approach in
http://bugs.python.org/file13860/setargvex.patch of adding a new API
entry point, whilst maximizing compatibilty for all of the code our
there using the existing entry point.

Sadly, this won't help existing applications affected by this problem, without all of them needing to be changed.

My change proposed in msg90336 won't help either, at least not in all cases. Apps that call PySys_SetArgv with 1, { "myappname", NULL } can still be tricked to add full CWD path at the beginning of sys.path on platforms with realpath().

Here is a new patch giving more details in the doc, and explicitly mentioning the CVE entry.

+ - If the name of an existing script is passed in ``argv[0]``, its absolute
+ path is prepended to :data:`sys.path`

Absolute path to the directory where script is located. And I believe there's no absolute path guarantee for platforms without realpath / GetFullPathName.

Should the documentation also give some guidance to those that embed python and don't want to start using SetArgvEx right away and break compatibility with older python versions? Something like:

If you're embedding python in your application, using SetArgv and don't want modified sys.path, call PyRun_SimpleString("sys.path.pop(0)\n"); after SysArgv to unconditionally drop the first sys.path argument added by SetArgv.

Absolute path to the directory where script is located. And I believe
there's no absolute path guarantee for platforms without realpath /
GetFullPathName.

Yes, this is more precise indeed. As for realpath(), I would expect it
to be present on modern Unices (man page says "4.4BSD, POSIX.1-2001").

If you're embedding python in your application, using SetArgv and
don't want modified sys.path, call
PyRun_SimpleString("sys.path.pop(0)\n"); after SysArgv to
unconditionally drop the first sys.path argument added by SetArgv.

I suppose
PyRun_SimpleString("import sys; sys.path.pop(0)\n");
would be better.
Thanks for the comments, I'll update the patch.

Committed in r81398 (trunk), r81399 (2.6), r81400 (py3k), r81401 (3.1). Thank you!

Demo/embed/demo.c calls PySys_SetArgv(), which may be where
some people are copying their code from. I've updated it to
use PySys_SetArgvEx() and added an explanatory comment in rev. 81881.

Since the function was also added to 2.6, the 2.6 What's New should mention it; added in rev81887.

This issue is equivalent to MS Windows DLL hijacking (the MS situation is worse, because the DDL can be in network shares or, even , in remote webdav servers):

http://blog.metasploit.com/2010/08/exploiting-dll-hijacking-flaws.html
http://news.cnet.com/8301-27080_3-20014625-245.html

When I learned about this attack, my first thought was "what if sys.path.index('')>=0?". Arg!.

Navigation Menu

Repository navigation

CVE-2008-5983 python: untrusted python modules search path #50003

25 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

CVE-2008-5983 python: untrusted python modules search path #50003

Description

Activity

iankko commented on Apr 14, 2009

iankko commented on Apr 14, 2009

iankko commented on Apr 14, 2009

iankko commented on Apr 14, 2009

pitrou commented on May 1, 2009

pitrou commented on May 1, 2009

glyph commented on May 2, 2009

pitrou commented on May 2, 2009

gpshead commented on May 3, 2009

pitrou commented on May 3, 2009

pitrou commented on May 3, 2009

pitrou commented on May 3, 2009

25 remaining items

thoger commented on May 4, 2010

pitrou commented on May 4, 2010

bitdancer commented on May 4, 2010

davidmalcolm commented on May 17, 2010

pitrou commented on May 18, 2010

thoger commented on May 18, 2010

pitrou commented on May 20, 2010

thoger commented on May 21, 2010

pitrou commented on May 21, 2010

pitrou commented on May 21, 2010

akuchling commented on Jun 11, 2010

akuchling commented on Jun 11, 2010

jcea commented on Sep 28, 2010

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions