msg94819 - (view) |
Author: Brandon Bloom (snprbob86) |
Date: 2009-11-02 07:34 |
This issue came up while doing Google App Engine development. Apparently
the default wsgi handler logic is to cache os.environ into os_environ at
import time. This is reasonable behavior for wsgi, but when using cgi,
this is a serious security hole which leaks information between requests.
See this related bug at GAE:
http://code.google.com/p/googleappengine/issues/detail?
id=2040&q=cookies%20dev_appserver.py&colspec=ID%20Type%20Status%20Priority
%20Stars%20Owner%20Summary%20Log%20Component
|
msg94867 - (view) |
Author: PJ Eby (pje) * |
Date: 2009-11-03 19:05 |
This is not an issue with CGI, but an issue with using an inappropriate
WSGI handler for a long-running process environment that only emulates CGI.
That is, in a true CGI environment, there can't be *multiple* requests
made to CGIHandler, and so it can't leak. In "normal" (i.e. pre-GAE)
long-running web environments, os.environ would not contain any request
information, only the process startup environment.
This limitation of CGIHandler should be better documented, but it's a
natural consequence of its design. The os_environ and base_environ
variables are provided so that subclasses with specialized needs can
handle them differently, and GAE is definitely a specialized need.
If someone wants to provide a GAEHandler class, great; otherwise, the
documented way to run a WSGI app under GAE is the
google.appengine.ext.webapp.util.run_wsgi_app function.
|
msg94870 - (view) |
Author: Brandon Bloom (snprbob86) |
Date: 2009-11-03 20:58 |
> That is, in a true CGI environment, there can't be *multiple* requests
> made to CGIHandler, and so it can't leak. In "normal" (i.e. pre-GAE)
> long-running web environments, os.environ would not contain any request
> information, only the process startup environment.
That's fair. In this case the CGIHandler should raise an exception on
subsequent requests to prevent this programming error.
> If someone wants to provide a GAEHandler class, great; otherwise, the
> documented way to run a WSGI app under GAE is the
> google.appengine.ext.webapp.util.run_wsgi_app function.
I'm not sure if run_wsgi_app was available right from the start, as
some early tutorials and samples show using CGIHandler. That's how we
ran into this issue.
|
msg94871 - (view) |
Author: PJ Eby (pje) * |
Date: 2009-11-03 21:13 |
Hm. In retrospect, CGIHandler should probably just set os_environ to an
empty dictionary in its class body (thereby not using the cached
environ), and this would then work correctly for repeated uses.
This would be a clean bugfix and wouldn't affect behavior for any
existing uses of CGIHandler that weren't already worse broken than the
GAE case. ;-)
|
msg94880 - (view) |
Author: Brandon Bloom (snprbob86) |
Date: 2009-11-04 07:14 |
> Hm. In retrospect, CGIHandler should probably just set os_environ to an
> empty dictionary in its class body (thereby not using the cached
> environ), and this would then work correctly for repeated uses.
>
> This would be a clean bugfix and wouldn't affect behavior for any
> existing uses of CGIHandler that weren't already worse broken than the
> GAE case. ;-)
Yup, rock on with that :-)
|
msg94893 - (view) |
Author: PJ Eby (pje) * |
Date: 2009-11-04 16:43 |
I've forwarded the suggested fix to the GAE team; it is to add this line:
os_environ = {} # Handle GAE and other multi-run CGI use cases
to the class body of CGIHandler. This should also be done in the Python
stdlib.
Btw, this fix can also be applied as a monkeypatch, by simply doing
'CGIHandler.os_environ = {}' before using CGIHandler.
|
msg94924 - (view) |
Author: Brandon Bloom (snprbob86) |
Date: 2009-11-05 10:40 |
> I've forwarded the suggested fix to the GAE team; it is to add this line:
>
> os_environ = {} # Handle GAE and other multi-run CGI use cases
>
> to the class body of CGIHandler. This should also be done in the Python
> stdlib.
I don't think that works. This still shares a common os_environ across
requests. You need to clear the dictionary after every request.
|
msg94934 - (view) |
Author: PJ Eby (pje) * |
Date: 2009-11-05 15:39 |
The os_environ attribute is only used in one place; this line:
env = self.environ = self.os_environ.copy()
So, nothing is shared.
|
msg98895 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2010-02-05 19:16 |
Phillip, can you fix this in 2.6, 2.7, 3.1 and trunk? (There are certain rules for merging fixes between branches but I don't recall the details. Contact Benjamin or another active core developer via IRC if you need help.)
|
msg98973 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2010-02-07 00:16 |
Marking this as release blocker to ensure the (one-line) fix makes it into 2.7 and 3.2.
|
msg99524 - (view) |
Author: Barry A. Warsaw (barry) * |
Date: 2010-02-18 20:02 |
I'd like to get a fix for this into 2.6.5. PJE, can you at least generate a patch and a test for Python 2.6 in enough time to review for the first release candidate (slated 2010-03-01)?
|
msg99549 - (view) |
Author: PJ Eby (pje) * |
Date: 2010-02-19 00:30 |
What sort of test did you have in mind? To test the desired outcome, it seems we'd need to poison os.environ, reload wsgiref.handlers, remove the poison, and then make sure it didn't get in. That seems a bit like overkill (as well as hard to get right), while the alternative of simply testing that CGIHandler has an empty os_environ seems a bit like a cheat. Any thoughts?
|
msg100273 - (view) |
Author: Barry A. Warsaw (barry) * |
Date: 2010-03-01 21:20 |
I can't come up with a good way to test this either. :(
I'm just going to apply the suggested change and we'll have to test it in 2.6.5 rc 1. PJE, will you be able to do that?
|
msg100274 - (view) |
Author: Barry A. Warsaw (barry) * |
Date: 2010-03-01 21:35 |
r78565 for py26.
|
msg100275 - (view) |
Author: PJ Eby (pje) * |
Date: 2010-03-01 21:40 |
Will I be able to do what? I have a kludgy test and a patch in my checkout, I was just waiting for word back from you (since Feb 19) on whether that would be ok.
|
msg100276 - (view) |
Author: Barry A. Warsaw (barry) * |
Date: 2010-03-01 21:55 |
Sorry, I already applied the patch you suggested to the release26-maint branch, trunk, and py3k. release31-maint will be applied in a moment.
Please double check that I applied the correct patch. If you have a test you feel moderately confident about, please apply that too. Right now, the change is untested. Ping me on #python-dev if you have questions.
|
msg100277 - (view) |
Author: PJ Eby (pje) * |
Date: 2010-03-01 22:18 |
Patch looks correct.
|
msg100279 - (view) |
Author: Barry A. Warsaw (barry) * |
Date: 2010-03-01 22:25 |
Thanks Phillip.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:54 | admin | set | nosy:
+ benjamin.peterson, georg.brandl github: 51499
|
2010-03-01 22:25:16 | barry | set | messages:
+ msg100279 |
2010-03-01 22:18:48 | pje | set | messages:
+ msg100277 |
2010-03-01 21:55:33 | barry | set | messages:
+ msg100276 |
2010-03-01 21:40:18 | pje | set | messages:
+ msg100275 |
2010-03-01 21:35:15 | barry | set | status: open -> closed resolution: accepted -> fixed messages:
+ msg100274
|
2010-03-01 21:20:07 | barry | set | messages:
+ msg100273 |
2010-02-19 00:30:48 | pje | set | messages:
+ msg99549 |
2010-02-18 20:02:42 | barry | set | nosy:
+ barry messages:
+ msg99524
|
2010-02-07 00:16:40 | gvanrossum | set | priority: high -> release blocker resolution: accepted messages:
+ msg98973
|
2010-02-05 19:16:20 | gvanrossum | set | nosy:
+ gvanrossum messages:
+ msg98895
|
2009-11-05 15:39:49 | pje | set | messages:
+ msg94934 |
2009-11-05 10:40:30 | snprbob86 | set | messages:
+ msg94924 |
2009-11-04 16:43:32 | pje | set | keywords:
+ patch, easy, 26backport
messages:
+ msg94893 |
2009-11-04 07:14:29 | snprbob86 | set | messages:
+ msg94880 |
2009-11-03 21:13:55 | pje | set | messages:
+ msg94871 |
2009-11-03 20:58:14 | snprbob86 | set | messages:
+ msg94870 |
2009-11-03 19:05:17 | pje | set | messages:
+ msg94867 |
2009-11-02 23:20:11 | pitrou | set | priority: high assignee: pje
nosy:
+ pje versions:
+ Python 2.6, Python 3.1, Python 2.7, Python 3.2, - Python 2.5 |
2009-11-02 07:34:03 | snprbob86 | create | |