tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Craig R. McClanahan" <Craig.McClana...@eng.sun.com>
Subject Re: [PATCH] Re: Ready for 3.2b7?
Date Sat, 11 Nov 2000 01:14:26 GMT
Paul and others,

See intermixed comments below.

Paul Frieden wrote:

> I've attached 1 patch, 2 modified interceptors, and one new
> interceptor.  These all work with 3.2b6.  Please consider these for
> 3.2b7.  Here is what they do:
>
> CookieTools.patch:
>         This patch fixes cookie deletion.  The problem was that to delete a
> cookie, you use
>         setMaxAge(0).  The implementation of CookieTools adds the max age onto
> the current time of
>         the server.  Thats fine, but the RFCs refer to setting the time into
> the past for
>         deletion.  This patch sets the expire time on the cookie way back into
> the past.  It at
>         least fixes it for me.
>

Makes perfect sense.  I patched this for both 3.2 and 4.0 (since I had copied CookieUtils
from the 3.2 tree :-).

>
> SessionInterceptor1.java & StandardSessionInterceptor1.java:
>         This patch modifies the behavior of session cookies.  The way the
> original worked was
>         SessionInterceptor would find the first cookie named JSESSIONID, and
> store it as the
>         requested session ID.  StandardSessionInterceptor would then check the
> requested session
>         ID to see if it was valid in the selected context.  This version
> removes the cookie
>         checking from SessionInterceptor, and moves all the cookie handling to
>         StandardSessionInterceptor.  This gives us access to the context so
> that we can check all
>         the cookies named JSESSIONID to find a valid one for that context.
> That allows a / and a
>         non-/ cookie to both be present and it will still pick the right one.
> These two new
>         interceptors should be used together, and replace the original
> SessionInterceptor and
>         StandardSessionInterceptor.  These also set the jvmRoute on the /
> context.  We can do that
>         now because it will always get the right session ID for the context.
>

I like the thinking here, and I'm currently experimenting with these two new interceptors
to
make sure nothing else breaks.  I do have one suggested change, though.

Right now, your patch tries to find the "correct" JSESSIONID cookie by looking for a valid
one in the current context.  Not only will this fail in the (admittedly rare) case where the
same session id is assigned by two different webapps, it also hides a useful piece of
information.

Consider that I have a cookie-maintained session, and I go to lunch (so my session expires)
before submitting the next request.  My request will still include the cookie for this web
app, so it should be reported via request.getRequestedSessionId() even though it's not valid
-- which means request.isRequestedSessionIdValid() should return false.

What I suggest we do instead (for both 3.2 and 4.0, by the way) is to treat the *first*
JSESSIONID occurrence that is submitted as the one that is relevant for this web
application.  This is based on a requirement in the cookie spec (RFC 2109) that the browsers
actually seem to be obeying:

    "If multiple cookies satisfy the criteria above, they are
    ordered in the Cookie header such that those with more
    specific Path attributes precede those with less specific."

This conveniently matches the rules that servlet containers use to map requests to context
paths -- the longest matching context path wins.  Therefore, if I have active sessions in
three contexts with the following context paths:

    /

    /foo

    /foo/bar

then if I submit a request for "/foo/bar/index.jsp" I will get be sent to the "/foo/bar"
context, and will receive all three cookies -- the one for the /foo/bar context being first.

What do you think?


>
> SessionCookieSanitizer.java:
>         This is a new interceptor that will hide all cookies named JSESSIONID
> that are not valid
>         sessions from the webapp.  In the case of a good app and a malicious
> app, if the one
>         user logged in on the good app, and then accessed the malicious app,
> the malicious app
>         could send the session ID to someone who could then masquerade as the
> user in the good
>         app.
>

I haven't gotten to this one yet, but it sounds reasonable.  If implemented, this should
*definitely* be in a separate interceptor so it can be commented out -- otherwise, you could
not write a web app that acts like a "proxy" and therefore needs access to all of the
incoming headers.

>
> Both SessionInterceptor1 and StandardSessionInterceptor1 are mostly the
> same as the originals, but some code has been moved.  Sending these as
> patches might make it unclear where the changes had occured and why, so
> I'm sending them in their entirity.
>
> Paul Frieden
>

Thanks for the patches!!!

Craig



Mime
View raw message