Invalid characters in mod_python session ID.

[Graham Dumpleton <gr...@dscpl.com.au>]

In mod_python, the session ID consists of 32 characters coming from the ranges
0-9 and a-f. At the moment the code will if it detects invalid characters in
the SID or it is the wrong size, raise a HTTP_INTERNAL_SERVER_ERROR exception.

        if self._sid:
            # Validate the sid *before* locking the session
            # _check_sid will raise an apache.SERVER_RETURN exception 
            if not _check_sid(self._sid):
                self._req.log_error("mod_python.Session warning: The session id contains invalid characters, valid characters are only 0-9 and a-f",
                        apache.APLOG_WARNING)
                raise apache.SERVER_RETURN, apache.HTTP_INTERNAL_SERVER_ERROR

This occurs regardless of whether the SID was supplied explicitly by user code,
or it was sent by the browser.

A problem with raising an Apache error like this is that if a invalid SID has managed
to get into the browsers cache, a web application isn't able to automatically just
discard the existing value and replace it with a brand new SID. Instead, it is pushed
onto the user to go into the cookie cache of their browser and expunge the SID
which is causing the problem. Only after the user taking this action will they be
able to use your web application.

To me this seems to be the wrong way of going about it. It would make much more
sense to me if the web application, ie., mod_python session code, just treated it as
if the browser hadn't sent any session ID in the first place and create a new session
with the session ID in the browser being replaced.

That said, the other way the SID can be supplied is if the application code supplied
it explicitly to the session object when it is created. In that case any mistake is the
fault of the application code and I believe it should still raise an exception, however
it should be a general ValueError exception to make it easier for the application
to catch it, rather than a HTTP exception.

Thus propose the above code be replaced with:

        if self._sid:
            if not _check_sid(self._sid):
                if sid:
                    # Supplied explicitly by user of the class,
                    # raise an exception and make the user code
                    # deal with it.
                    raise ValueError("Invalid Session ID: sid=%s" % sid)
                else:
                    # Derived from the cookie sent by browser,
                    # wipe it out so it gets replaced with a
                    # correct value.
                    self._sid = None

Jim, you wrote the original code which raised the HTTP exception for all cases,
are you happy with this change? Anyone else see any issues with this?

I'll take the usual period of silence to mean I should just make the change and
you do still trust me to the do the correct thing. :-)

Graham

Re: Invalid characters in mod_python session ID.

[Jim Gallacher <jp...@jgassociates.ca>]

Graham Dumpleton wrote:
> In mod_python, the session ID consists of 32 characters coming from the ranges
> 0-9 and a-f. At the moment the code will if it detects invalid characters in
> the SID or it is the wrong size, raise a HTTP_INTERNAL_SERVER_ERROR exception.
> 
>         if self._sid:
>             # Validate the sid *before* locking the session
>             # _check_sid will raise an apache.SERVER_RETURN exception 
>             if not _check_sid(self._sid):
>                 self._req.log_error("mod_python.Session warning: The session id contains invalid characters, valid characters are only 0-9 and a-f",
>                         apache.APLOG_WARNING)
>                 raise apache.SERVER_RETURN, apache.HTTP_INTERNAL_SERVER_ERROR
> 
> This occurs regardless of whether the SID was supplied explicitly by user code,
> or it was sent by the browser.
> 
> A problem with raising an Apache error like this is that if a invalid SID has managed
> to get into the browsers cache, a web application isn't able to automatically just
> discard the existing value and replace it with a brand new SID. Instead, it is pushed
> onto the user to go into the cookie cache of their browser and expunge the SID
> which is causing the problem. Only after the user taking this action will they be
> able to use your web application.
> 
> To me this seems to be the wrong way of going about it. It would make much more
> sense to me if the web application, ie., mod_python session code, just treated it as
> if the browser hadn't sent any session ID in the first place and create a new session
> with the session ID in the browser being replaced.
> 
> That said, the other way the SID can be supplied is if the application code supplied
> it explicitly to the session object when it is created. In that case any mistake is the
> fault of the application code and I believe it should still raise an exception, however
> it should be a general ValueError exception to make it easier for the application
> to catch it, rather than a HTTP exception.
> 
> Thus propose the above code be replaced with:
> 
>         if self._sid:
>             if not _check_sid(self._sid):
>                 if sid:
>                     # Supplied explicitly by user of the class,
>                     # raise an exception and make the user code
>                     # deal with it.
>                     raise ValueError("Invalid Session ID: sid=%s" % sid)
>                 else:
>                     # Derived from the cookie sent by browser,
>                     # wipe it out so it gets replaced with a
>                     # correct value.
>                     self._sid = None
> 
> Jim, you wrote the original code which raised the HTTP exception for all cases,
> are you happy with this change? Anyone else see any issues with this?

The change seems reasonable. The motivation for writing a message to the 
error log was to give the server admin a chance to detect a possible 
attack, but perhaps that is overkill?

On another topic, I think any comments in the code such as "# For 
backwards compatability only" should include some mention of the version 
so we don't lose track of when these changes were made. The simplest 
thing to do would be to include a deprecation note. eg.

# For backwards compatability only (deprecated in 3.3)

And in some future versions log a warning (3.4) before finally removing 
the code (3.5).
	
Jim