Le dimanche 29 novembre 2009 23:43:15, Willy Tarreau a écrit :
> The suggested way to to that is to enclose the values between the comment
> and the mask like here (though any other proposal might fit) :
>
> /* appsession */
> #define PR_O2_AS_REQL 0x00004000 /* learn the session id from the
> request */
> #define PR_O2_AS_PFX 0x00008000 /* match on the cookie prefix */
>
> /* Encoding of appsession cookie matching modes : 2 possible values => 1 bit
> */
> #define PR_O2_AS_M_PP 0x00000000 /* path-parameters mode (the default
> mode) */
> #define PR_O2_AS_M_QS 0x00010000 /* query-string mode */
> #define PR_O2_AS_M_ANY 0x00010000 /* mask covering all
> PR_O2_AS_M_* values */
I'll remember that if I have more patches to send ;)
> > void get_srv_from_appsession(struct session *t, const char *begin, int len)
> > {
> > - char *request_line;
> > + char *end_params, *first_param, *cur_param, *next_param;
> > + char separator;
> > + int value_len;
> > +
> > + int mode = t->be->options2 & PR_O2_AS_M_ANY;
> >
> > if (t->be->appsession_name == NULL ||
> > - (t->txn.meth != HTTP_METH_GET && t->txn.meth != HTTP_METH_POST) ||
> > - (request_line = memchr(begin, ';', len)) == NULL ||
> > - ((1 + t->be->appsession_name_len + 1 + t->be->appsession_len) >
> > (begin + len - request_line)))
> > + (t->txn.meth != HTTP_METH_GET && t->txn.meth != HTTP_METH_POST)) {
> > return;
> > + }
>
> I may be wrong but I think we lost a length check above and that
> we'll always onsider we have <len> bytes available in the buffer
> so that memchr(begin, ';', len) has no chance of going beyond the
> buffer's limit. Oh well, maybe it was already like that :-/
>
> Could you please check on your side and confirm/infirm my doubts ?
> Basically I want to ensure we never dereference the buffer past its
> end, so begin+len bust always be below the buffer size. If you think
> a control is missing, we can merge it as a separate patch because
> it's already missing in current code then.
Previously, the function was called with :
get_srv_from_appsession(s, &req->data[msg->som], msg->sl.rq.l);
I assume that msg->sl.rq variables are already correctly calculated and that we
can replace this call with :
get_srv_from_appsession(s, &req->data[msg->som + msg->sl.rq.u],
msg->sl.rq.u_l);
This allows to parse only the URL, skipping the http method and the protocol.
I believe this should always be contained in the buffer size.
The parser will then restrict to this area when it will extract the session
value (It was missing in the previous code).
Did I understand what you wanted to know ?
> I'm just waiting for your response on this possible issue and I'm
> OK to merge it. Please tell me if you'd prefer to resend a different
> patch with the cosmetic changes or if I can do them myself. While
> you're at it, I noticed a mis-indented "if" statement alone in the
> remaining part. Also something I can fix if needed.
It would be great if you can do this changes (to prevent several patch versions
in case I still leave some mis-indented code or misplaced comments in the
options constants).
Thanks a lot.
--
Cyril Bonté
