Suggestion for small(?) change in BaseRequest.py. Security effects?
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
On Thu, 2004-09-02 at 12:38, Lennart Regebro wrote:
...
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.
I'd add a transaction rollback before return actually, which is the normal behavior when an exception is raised like Redirect or Unauthorized So if the situation is: user is anonymous or has insufficient rights on a object which is handled after some changes are made to objects, the challenge will take place and rolls back all these changes which should not have taken place for that user. so: if user is None and roles != UNSPECIFIED_ROLES: response.unauthorized() get_transaction().abort() return should do the trick. Regards Tino Wildenhain
Lennart Regebro wrote at 2004-9-2 12:38 +0200:
... 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.
If the traversal made any changes to persistent state, then these changes are committed rather than aborted. Usually, traversal should not change the persistent state -- but... -- Dieter
Dieter Maurer wrote:
Lennart Regebro wrote at 2004-9-2 12:38 +0200:
... 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.
If the traversal made any changes to persistent state, then these changes are committed rather than aborted.
Usually, traversal should not change the persistent state -- but...
Would the transaction.abort() addition suggested by Tino be enough to solve that?
Lennart Regebro wrote:
Dieter Maurer wrote:
Lennart Regebro wrote at 2004-9-2 12:38 +0200:
... 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.
If the traversal made any changes to persistent state, then these changes are committed rather than aborted.
Usually, traversal should not change the persistent state -- but...
Would the transaction.abort() addition suggested by Tino be enough to solve that?
Lennart, I am worried that there may be third-party application code which relies on 'validate' to raise an exception. Returning the login form directly is not really a big win over a redirect; among other things, it messes up cacheability, because the URL no longer corresponds to the "real" content. Tres. -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com
Tres Seaver wrote at 2004-9-3 08:56 -0400:
... I am worried that there may be third-party application code which relies on 'validate' to raise an exception. Returning the login form directly is not really a big win over a redirect; among other things, it messes up cacheability, because the URL no longer corresponds to the "real" content.
This can easily be controlled with cache control headers. Not making a redirect would give the login form more control on what to do after the login. Currently, I would allow to work around a bug in CookieCrumber (it does not include "QUERY_STRING" in its "came_from"). Including additional request information may be also interesting for some "POST" requests (that do not have a meaningfull "QUERY_STRING"). -- Dieter
Lennart Regebro wrote at 2004-9-3 12:05 +0200:
... Dieter Maurer wrote:
If the traversal made any changes to persistent state, then these changes are committed rather than aborted.
Usually, traversal should not change the persistent state -- but...
Would the transaction.abort() addition suggested by Tino be enough to solve that?
Yes. -- Dieter
participants (4)
-
Dieter Maurer -
Lennart Regebro -
Tino Wildenhain -
Tres Seaver