-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CVE-2008-5983 python: untrusted python modules search path #50003
Comments
To sum up the behavior, the following table displays whether
|
As no longer work of "python ./foo.py" after patch utilization may |
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? |
I wanted to read the patch at |
Antoine, The problem is that apparently every program that embeds Python calls The 'python' executable itself is not really "vulnerable" in quite the |
I'm not sure we can change the behaviour of PySys_SetArgv() like that. |
both the behavior change and PySys_SetArgvEx() with an additional Some people may disagree about changing the default behavior. So long +1 on adding a PySys_SetArgvEx() in time for 3.1 (the clock is ticking +0 on the existing API default change. |
By the way, the advantage of a new function over a behaviour change is |
Here is a patch for trunk. |
Jan, would the new API be ok to you? |
I disagree that this issue is release critical. I'm still skeptical that |
Ok, downgrading to critical. |
Antoine, (re: #msg87083, #msg87084) -- while the API change is acceptable and 1, doesn't avoid the need to fix the issue (by calling 2, doesn't dismiss the risk of future appearance of application, Wouldn't be possible to fix it 'only in Python' and prevent such To Martin (re: #msg87212): What's the question of 'security nature' of the issue, Glyph in I recommend the final fix should be applied to all active Python Regards, Jan. |
The question is whether these are theoretical or real problems. |
Hello Jan,
As you said yourself, we don't want to break backwards compatibility for Besides, the patch you proposed is fragile as it relies on a hard coded
Well, you can always shoot yourself in the foot in C, even without using
AFAICT, not without risking breaking compatibility for perfectly |
You have to use a Python-written gedit plugin for that to happen. For 17569:open("gconf.so", O_RDONLY) = -1 ENOENT (No such file |
I wonder why all these applications call PySys_SetArgv at all if they |
It suggests to me that somewhere there's some documentation, or an If the right thing to do is to just not call the function at all, we |
That may be an explanation. However, it would be immensely useful IOW, I *really* want to understand what's happening before fixing |
Agreed. Does anyone currently subscribed to this ticket know the author |
gedit does it here: http://git.gnome.org/cgit/gedit/tree/plugin-loaders/python/gedit-plugin- I've emailed the file's author (Jesse) out of the blue to see if he knows |
re: gedit """I'm by no means an expert (I did not design the original python module |
It seems other projects are already fighting with the path-changing |
src/if_python.c in vim-7.2 has a comment: I think the crash is follows. % python
Python 2.5.2 (r252:60911, Jan 4 2009, 17:40:26)
[GCC 4.3.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import warnings
>>> warnings.warn("foo")
__main__:1: UserWarning: foo
>>> import sys
>>> sys.argv
['']
>>> sys.argv = []
>>> sys.argv
[]
>>> warnings.warn("foo")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python2.5/warnings.py", line 54, in warn
filename = sys.argv[0]
IndexError: list index out of range
>>> |
Hello guys, what's the current state of this issue? The proposed patch hasn't Thanks && Regards, Jan.Jan iankko Lieskovsky |
Hello,
I was hoping for more feedback before committing it. While it has been
If it were so this bug would have been closed. |
Have you considered something like this? (patch against 3.1) --- Python/sysmodule.c.orig
+++ Python/sysmodule.c
@@ -1643,6 +1643,7 @@ PySys_SetArgv(int argc, wchar_t **argv)
#endif /* Unix */
}
#endif /* All others */
+ if (n > 0 || argv0 == NULL || wcscmp(argv0, L"-c") == 0) {
a = PyUnicode_FromWideChar(argv0, n);
if (a == NULL)
Py_FatalError("no mem for sys.path insertion");
@@ -1650,6 +1651,7 @@ PySys_SetArgv(int argc, wchar_t **argv)
Py_FatalError("sys.path.insert(0) failed");
Py_DECREF(a);
+ }
}
Py_DECREF(av);
} I presume main problem here is that '' may end up as first item in That is desired in some cases, namely:
It does not happen and is not desired in other cases:
Here foo.py can be just filename or filename with relative or absolute Problematic case is embedded use when bogus argv0 can cause '' to be Patch above attempts to skip modification of sys.path when realpath
This should fix the problem for apps embedding python and providing Ideas? Anyone is aware of the valid usecase that can break with this? Advantage to Ex approach is that it does not require change on the |
Tomas, your patch is breaking an existing API, which may break existing Besides, parsing of command line flags is already done in |
Additional API has one disadvantage - it requires a modification of all Therefore, it may still be worth reviewing current behaviour (that As for command line flags, I presume you're referring to the http://svn.python.org/view?view=rev&revision=39544 Again, it's an attempt to make sure this only changes behaviour in |
Indeed, it would certainly be useful to review current behaviour and |
Besides, the new API makes the behaviour more explicit and puts the In the face of ambiguity, refuse the temptation to guess. |
Link to older Python tracker issue discussing the same problem and
Strange enough, but implied from reading above issue, just an http://docs.python.org/modindex.html and in that case: Probably off-topic, but is there in Python some mechanism how to |
This is not really the same thing as bpo-946373. That one seems to be |
Has anyone else had an opportunity to have a look at the change proposed in #msg90336? |
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 |
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: 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. |
Right.
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 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. |
Yes, this is more precise indeed. As for realpath(), I would expect it
I suppose |
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 |
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 When I learned about this attack, my first thought was "what if sys.path.index('')>=0?". Arg!. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: