[Zope-dev] Suggestion for small(?) change in BaseRequest.py. Security effects?

[Lennart Regebro]

First Explanation:
==================

We have been discussing how to implement the challenge support in 
PluggableAuthService, and I have been trying a couple of different ways.

(Note: challenge-support in this context means, how to ask the client 
for authenticifcation credentials, that is username and password, or 
tickets, or certificates, or something.)

The preferred solution has been narrowed down to using a way similar to 
CookieCrumbler, that is, replacing the current response-objects 
unauthorized() method with a method that does the challenge, like so:

         if not req.get('disable_cookie_login__', 0):
             if attempt == ATTEMPT_LOGIN or attempt == ATTEMPT_NONE \
                    or attempt == ATTEMPT_RESUME:
                 # Modify the "unauthorized" response.
                 req._hold(ResponseCleanup(resp))
                 resp.unauthorized = self.unauthorized
                 resp._unauthorized = self._unauthorized

Now, there is one tiny problem with that, and that problem is in 
BaseRequest.Traverse(). (BaseRequest.py, line 438):

             if user is None and roles != UNSPECIFIED_ROLES:
                 response.unauthorized()

         if user is not None:
             ...

As you see, this code assumes that response.unauthorized will raise an 
error. If it does not, the code will continue *as if the user is 
validated*. An error will occur later, since the user isn't validated, 
so it's nota security hole (is think), but it causes a problem:
The only acceptable end of an unauthortized() is to raise an exception. 
This means in practice, either the Unauthorized exception that prpvoces 
the standasd 401 login box challenge, or a Redirect to a login page.

This seems a bit limiting, and there has been requests for doing other 
things, such as returning an HTML form directly, and whatnot.

Then, suggestion:
=================
My tests seems to show that inserting a return after the unauthorized 
call above:
             if user is None and roles != UNSPECIFIED_ROLES:
                 response.unauthorized()
		return
will solve this issue. It is now possible to NOT raise an exception in 
unauthorized and still not get problems. Instead, you can now to a 
RESPONSE.redirect(), or you can replace the body with setBody for a 
login form, or something like that.

I haven't been able to find any other code that continues after an 
unathorized call, so this should be the only place. Also, during normal 
operation, it is obvuiosly a safe bet. The change in itself has no nasty 
side effects.


Last, questions:
================
Are there any other problems with NOT raising an exception in 
unathorized(). Becuase if there is, we probably limit the possible 
challenge responses to a redirect, and then this change makes no difference.

I would suggest that the change goes in ANYWAY, to stop people almost 
accessing stuff by fiddling with response.unauthorized (although I admit 
  I don't know how to do that).

Lennart Regebro