"in Python before 2.6" ... I've heard that this isn't fixed in py2.6.

 Also when you first pinged me about this, there were two proposals to fix this in python instead of each app. (I assume that's what this BZ is for):

So our proposal is to fix this vulnerability on Python package basis, i.e:
1, either append the arguments provided to the python script on the 
   command line as the LAST argument of the sys.path -- the same
   behavior as Perl does (patch attached),
2, or perform some embedded Python sys.path sanitization based
   on Debian patches (see above links) and include it into the 
   code of PySy_SetArgv function, so all packages using this
   function would be no more vulnerable to this flaw.

...the first one, if applied just to embeded apps., seemed fine from a working POV ... but could still be exploited (although less so). The second seemed like it might break things as it just did:

 PyRun_SimpleString("import sys; sys.path = filter(None, sys.path)");

...also having a quick look at PySys_SetArgv his morning ... why can't we just filter out the cases where ARGV[0] == NULL or whatever it is, so they don't add "" to the path in the first place?

Fixing this in python would definitely be better than fixing it in all the apps.  Most any app that embeds a python interpreter will be hit by this, since the documentation for PySys_SetArgv says to use a bogus argv[0] in that case.

I pushed an update out for gedit already (see bug 481556) but gedit upstream is hesitant to take the fix because this really seems like a python issue.

See http://bugzilla.gnome.org/show_bug.cgi?id=569214 for the upstream gedit bug.

and http://bugzilla.gnome.org/show_bug.cgi?id=569273 for the blocker bug that shows some of the other gnome componenets affected (it's not a short list).

Created attachment 330270 [details]
Python CVE-2008-5983 patch -- moving the list of args provided to the python script on the command line as the last argument of sys.path.

So there were two things about moving the "." to the end of the path:

1. It didn't fix all problems.

2. Not sure how it interacts with "common" names, like "import utils" sub-modules which are in more than one module.

...I don't really care about #1, if it fixes something it's better than nothing. But I don't want to break anything due to #2 unless upstream signs off on it ... not that I know it'll break, but I'd just rather not.

 Better, I think, would be to distinguish between gedit running from /tmp which happens to load the interpreter ... and someone doing "./yummain.py" or whatever in a directory with yum in it. Ie. have something trigger between what python passes to SetArgv and what gedit/etc. pass?

I seem to be a recent victim of this bug as I wondered for several weeks now,
why my totem and my rhythmbox players crashed at startup. I even filed bugs for
both projects and tried to get help from the developers. After some debuggin it
was sure, that python plugins were the culprit as both apps crashed while
initializing the embedded interpreter. The reason was actually found now:

I'm a hobby python programmer and downloaded some recipes from ASPN and saved
them in my $HOME - one of them was a custom optparse.py! Now most python libs
will ask for optparse sooner or later and as $HOME seems to be the CWD for the
whole Xorg session all my GUI apps crashed with a SIGSEV when opened from
nautilus.

Please try to find a fix ASAP....

Created attachment 334888 [details]
Don't ever add "" and don't add cwd when python is embedded

Hey Jan, this untested patch might work.  I'd appreciate if you could look at it and try it out.

What it does is check if Py_SetProgramName() has been called and if it *hasn't* been called then assume that PySys_SetArgv is getting called from an embedded interpreter.  I'm not sure this is a perfect litmus test, but I looked at a few of the affected applications and they didn't call SetProgramName().  Also, I tried 

print sys.executable 

from idle, ipython, and python and it shows that it is getting set for all those cases, so I think the test may be good enough. I also added print sys.executable at the top of some python programs, and it's set in those cases, too.

I haven't tried compiling the python package to see if the patch actually fixes the problem.

When we get the patch all finalized would be good to see what upstream's reaction to it is.

Link to Python tracker:

http://bugs.python.org/issue5753

(In reply to comment #22)
> Created an attachment (id=334888) [details]
> Don't ever add "" and don't add cwd when python is embedded

Upstream does not seem to like the idea of is_embedded.  Quite independently of your patch, I've create something very similar, which attempts skip path modification if realpath failed.  It adds special checks to do modification if python is run in interactive mode or with -c argument.  My proposed change is in upstream bug:

http://bugs.python.org/issue5753#msg90336

Comments are welcome.

I've not checked thoroughly it there may be some catch when the fix is combined with HAVE_CANONICALIZE_FILE_NAME patch, which we have and it never went upstream.

> ipython

Btw, ipython seems to put '' in sys.path intentionally and it's done out of PySys_SetArgv.

Tomas, can you describe in which of the following cases modules
would be read from the current directory if your patch is used?

orig  patch run as
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
yes   ???   python test.py
yes   ???   python ./test.py
yes   ???   python /tmp/396/test.py
yes   ???   /bin/env python test.py

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

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

I'm asking because with the original patch which changed this
behaviour (see comment #32) I've encountered quite a few
regressions during the integration testing.

In upstream bug you mention that prepending '' is not desired in
the following cases:
- ./foo.py
- python foo.py
- env python foo.py

I'm afraid that especially the second case caused some of the
regressions mentioned above.

(In reply to comment #44)
> Tomas, can you describe in which of the following cases modules
> would be read from the current directory if your patch is used?

Just to clarify...  I'm *not* trying to prevent loading of module from current directory when using something like 'python test.py', I'm trying to prevent '' being added to sys.path.  It's not the same.

If you run 'python whatever.py', full path to a directory where whatever.py is located is added to sys.path.  This seems intended and desired, and not doing so is what leads to regressions you've probably seen before.  It's also not a big security risk, as if you're running python inside the directory where other users can write their files, the main script you're running may be maliciously modified as well.  Not perfect, as you can easily shoot yourself in a foot (e.g. by saving some script to /tmp and then running it from there), but seems to be expected and relied upon behaviour for python / perl / ... .

So instead of checking only whether modules are loaded from current directory, you should also look at sys.path.

> yes   ???   python test.py
> yes   ???   python ./test.py

Yes, as `pwd` will be in sys.path.

> yes   ???   python /tmp/396/test.py

Are you sure about this?  This should not try to load modules from cwd even before patch, should rather use /tmp/396.

> yes   ???   /bin/env python test.py

Yes, `pwd`.

> yes   ???   test.py
> yes   ???   ./test.py

`pwd`

> yes   ???   /tmp/396/test.py

Same as above, should not load modules from cwd.

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

Same as above for python without full path.

> no    ???   test-in-different-dir.py

Do you have . in $PATH for this test?  Otherwise, it should not even run, right?  If you have . in $PATH, or actually run it with ./, you should have `pwd` in sys.path.

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

`pwd`/bin should be in sys.path.

> In upstream bug you mention that prepending '' is not desired in
> the following cases:
> - ./foo.py
> - python foo.py
> - env python foo.py

That's right, and it does not happen in unpatched version, unless I'm missing something...

(In reply to comment #48)
> > yes   ???   python /tmp/396/test.py
> 
> Are you sure about this?  This should not try to load modules
> from cwd even before patch, should rather use /tmp/396.

Sorry, I was not accurate with the definition of the "cwd". The
first 10 tests check whether modules are loaded from the script's
directory, the last three checked that modules sitting in cwd are
not loaded, when script resides elsewhere.

Thanks for clarifying the issue with sys.path. I don't know what
exactly was broken with the original patch. Just wanted to make
sure we don't run into those regressions again.

python-2.6.2-8.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/python-2.6.2-8.fc12

python-2.6.4-27.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/python-2.6.4-27.fc13

python-2.6-14.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/python-2.6-14.fc11

python3-3.1.2-6.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/python3-3.1.2-6.fc13

python26-2.6.5-5.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/python26-2.6.5-5.el5

python-2.6.4-27.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

python-2.6.2-8.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

This issue has been addressed in following products:

  Red Hat Enterprise Linux 5

Via RHSA-2011:0027 https://rhn.redhat.com/errata/RHSA-2011-0027.html

Statement:

(none)