Hi Cyril,
On Sun, Nov 01, 2009 at 12:19:05AM +0100, Cyril Bonté wrote:
> Hello Willy and Aleksandar,
> If you agree, I would like to apply this new patch to add some more integrity
> checking on appsession.
>
> * the session value (provided by the URL or by the request/response cookie)
> is now well delimited :
> currently, setting "len 52" on a 32 chars value has a bad effect on load
> balancing (for example, a part of the query string is considered as the
> session id ; same thing happens with the cookie).
> => with the patch, "len 52" is now a maximum length.
OK.
> * it adds a verification on the '=' char :
> currently (with appsession JSESSIONID for example), an URL like
> http://<haproxy>/path;jsessionidfake=0123... matches the session id
> "ake=0123..."
> => with the patch, jsessionidfake won't be recognized.
I think we must not do this, because one of the reasons for not
checking the equal sign was because of people who have to use IIS
with its ASPSESSION cookies. The end of the cookie name is part
of the value. So it's important that we can check cookie prefixes.
> Tell me if you're OK for the loop added in get_srv_from_appsession(). This
> can be a little slower but I think this will help to eliminate some troubles
> in production.
At first glance I'm not too much scared.
However, please avoid variable declaration in the middle of the
code such as below, this is not compatible with compilers before
gcc version 3.0.
> @@ -4080,7 +4080,11 @@
> /* first, let's see if the cookie is
> our appcookie*/
>
> /* Cool... it's the right one */
> - manage_client_side_appsession(t, p3);
> + int value_len = p4 - p3;
> + if (value_len > t->be->appsession_len) {
> + value_len =
> t->be->appsession_len;
> + }
> + manage_client_side_appsession(t, p3,
> value_len);
Cheers,
Willy
