GeezerVison is showing github.com — CVE-2008-5983 python: untrusted python modules search path · Issue #50003 · python/cpython
CVE-2008-5983 python: untrusted python modules search path #50003Description
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'] Activityiankko commented on Apr 14, 2009 MannequinAuthor More actions iankko commented on Apr 14, 2009 MannequinAuthor More actions 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
iankko commented on Apr 14, 2009 MannequinAuthor More actions 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? iankko commented on Apr 14, 2009 MannequinAuthor More actions Just drop into /tmp and run (you will need the zenity package installed):
python3.1 ./test.py
or
gedit # unfixed gedit
in that directory. pitrou commented on May 1, 2009 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. pitrou commented on May 1, 2009 glyph commented on May 2, 2009 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. pitrou commented on May 2, 2009 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. gpshead commented on May 3, 2009 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. pitrou commented on May 3, 2009 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". pitrou commented on May 3, 2009 Here is a patch for trunk. pitrou commented on May 3, 2009 Jan, would the new API be ok to you? thoger commented on May 4, 2010 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! pitrou commented on May 4, 2010
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? bitdancer commented on May 4, 2010 FWIW I agree with Antoine. davidmalcolm commented on May 17, 2010 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). pitrou commented on May 18, 2010 Ok, I will try to write better documentation. thoger commented on May 18, 2010
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(). pitrou commented on May 20, 2010 Here is a new patch giving more details in the doc, and explicitly mentioning the CVE entry. thoger commented on May 21, 2010 + - 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. pitrou commented on May 21, 2010
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. pitrou commented on May 21, 2010 Committed in r81398 (trunk), r81399 (2.6), r81400 (py3k), r81401 (3.1). Thank you! akuchling commented on Jun 11, 2010 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. akuchling commented on Jun 11, 2010 Since the function was also added to 2.6, the 2.6 What's New should mention it; added in rev81887. jcea commented on Sep 28, 2010
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|