classification
Title: wsgiref.handlers.CGIHandler caches os.environ, leaking info between requests
Type: security Stage:
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: pje Nosy List: barry, gvanrossum, pje, snprbob86
Priority: release blocker Keywords: 26backport, easy, patch

Created on 2009-11-02 07:34 by snprbob86, last changed 2010-03-01 22:25 by barry. This issue is now closed.

Messages (18)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2010-03-01 21:35
r78565 for py26.
msg100275 - (view) Author: PJ Eby (pje) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2010-03-01 22:18
Patch looks correct.
msg100279 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2010-03-01 22:25
Thanks Phillip.
History
Date User Action Args
2010-03-01 22:25:16barrysetmessages: + msg100279
2010-03-01 22:18:48pjesetmessages: + msg100277
2010-03-01 21:55:33barrysetmessages: + msg100276
2010-03-01 21:40:18pjesetmessages: + msg100275
2010-03-01 21:35:15barrysetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg100274
2010-03-01 21:20:07barrysetmessages: + msg100273
2010-02-19 00:30:48pjesetmessages: + msg99549
2010-02-18 20:02:42barrysetnosy: + barry
messages: + msg99524
2010-02-07 00:16:40gvanrossumsetpriority: high -> release blocker
resolution: accepted
messages: + msg98973
2010-02-05 19:16:20gvanrossumsetnosy: + gvanrossum
messages: + msg98895
2009-11-05 15:39:49pjesetmessages: + msg94934
2009-11-05 10:40:30snprbob86setmessages: + msg94924
2009-11-04 16:43:32pjesetkeywords: + patch, easy, 26backport

messages: + msg94893
2009-11-04 07:14:29snprbob86setmessages: + msg94880
2009-11-03 21:13:55pjesetmessages: + msg94871
2009-11-03 20:58:14snprbob86setmessages: + msg94870
2009-11-03 19:05:17pjesetmessages: + msg94867
2009-11-02 23:20:11pitrousetpriority: 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:03snprbob86create