On Thu, Nov 05, 2015 at 08:54:39AM +0100, Lukas Tribus wrote:
> >> @@ -402,7 +402,7 @@ smp_fetch_req_ssl_ver(const struct arg *args, struct
> >> sample *smp, const char *kw
> >> if (bleft < 5)
> >> goto too_short;
> >>
> >> - version = (data[1] << 16) + data[2]; /* version: major, minor */
> >> + version = (data[9] << 16) + data[10]; /* client hello version: major,
> >> minor */
> >> msg_len = (data[3] << 8) + data[4]; /* record length */
> >
> > See above ? we check for 5 bytes minimum because we didn't parse further
> > than data[4], and now we're reading data[10], so the test on bleft above
> > must be changed to bleft < 11.
> >
> > Can someone please check if the other patch referenced above has the same
> > bug?
>
> Ouch, here's the original patch:
> http://marc.info/?l=haproxy&m=144431273015849&w=2
>
> It does indeed bump this check to 11 bytes. Also, there is a third change in
> that path, where it touches bleft and data values (bleft -= 11; data += 11;)
> some lines below.
>
> The original patch does not work, at this point I'm not quite sure why.
It's because of bleft -= 11 and data += 11. At the end of the code there's
a comment and a check :
/* We could recursively check that the buffer ends exactly on an SSL
* fragment boundary and that a possible next segment is still SSL,
* but that's a bit pointless. However, we could still check that
* all the part of the request which fits in a buffer is already
* there.
*/
if (msg_len > channel_recv_limit(req) + req->buf->data - req->buf->p)
msg_len = channel_recv_limit(req) + req->buf->data -
req->buf->p;
msg_len is the length of what starts at byte 5. So by skipping 6 extra bytes
the msg_len becomes incorrect and this check doesn't match anymore.
... which leads to another point. Since we're checking inside a record, are
we sure that we don't need to check the record type ? There are probably
bytes between the record length and the record's version.
I'd suggest at minima that the comment on "version" is adjusted and that the
version is read *after* msg_len so that we parse the message from left to
right only to avoid any confusion :
- version = (data[1] << 16) + data[2]; /* version: major, minor */
msg_len = (data[3] << 8) + data[4]; /* record length */
+ version = (data[9] << 16) + data[10]; /* record version: major,
minor */
And in fact I'm still not satisfied with this because the check for the
message length is performed a few lines later so we may parse crap from
a subsequent record. Thus the proper thing to do is this (sorry for hand-
written patch, I think you'll get the idea) :
/* SSLv3 header format */
- if (bleft < 5)
+ if (bleft < 11)
goto too_short;
version = (data[1] << 16) + data[2]; /* version: major, minor */
msg_len = (data[3] << 8) + data[4]; /* record length */
/* format introduced with SSLv3 */
if (version < 0x00030000)
goto not_ssl;
+ /* message length between 6 and 2^14 + 2048 */
+ if (msg_len < 6 || msg_len > ((1<<14) + 2048))
+ goto not_ssl;
bleft -= 5; data += 5;
/* we want to return the record version, not the envelope's
version */
+ version = (data[4] << 16) + data[5]; /* record version: major,
minor */
> Thanks and sorry for the near-miss,
No worries, that's exactly why I want to see patches posted to the list and
why I'm strongly against pull requests. I want to see eyes looking at the
code. There are 2000 eyeballs on this list, anyone at any moment can chime
in and ask a question or report a doubt.
Thanks,
Willy
