Hi Tim,

On Tue, May 01, 2018 at 01:57:06AM +0200, Tim Düsterhus wrote:
> Willy,
> 
> Am 30.04.2018 um 23:06 schrieb Willy Tarreau:
> >> Anything I could do to help investigate this? I can apply patches with
> >> additional logging or I can send you the unredacted configuration in
> >> private if that would help.
> > 
> > OK, it's just that for now I can't propose anything, I'm context-switching
> > far too much between many many different stuff :-(
> 
> I fiddled a bit by myself: I found that the http_header_add_tail2 in
> http_res_get_intercept_rule, case ACT_HTTP_SET_HDR (proto_http.c:2934)
> returns -1 (indicating failure). This return value is being ignored.

OK, thanks for confirming this!

> Adding the following debug output in http_header_add_tail2:
> 
>       printf("Buffer Information: size(%d) i(%d)\n", msg->chn->buf->size,
> msg->chn->buf->i);
>       buffer_dump(stderr, msg->chn->buf, 0, msg->chn->buf->i);
>       printf("Add tail: %d: %s\n", bytes, text);
> 
> Yields:
> 
> Buffer Information: size(16384) i(16367)
> Dumping buffer 0x5654d8047960
>             data=0x5654d8047974 o=0 i=16367 p=0x5654d8047974
>             relative:   p=0x0000
> Dumping contents from byte 0 to byte 16367
> *snip*
> Add tail: 0: X-XSS-Protection: 1; mode=block
> 
> in failure cases. You can see that the buffer is filled to the top.
> 
> Decoding the dumped buffer reveals that the buffer contains
> significantly more of the HTTP *body* in failure cases than in success
> cases.

Yes, that's indeed expected.

> It might make sense to enlarge the rewrite buffer reservation by
> default.

We used to have this a long time ago, the maxrewrite value used to
default to half the buffer size. But it caused too many requests to
be rejected and became absurd when users configure large buffers.

> I can absolutely imagine that people put in a ton of
> information in the times of Content-Security-Policy, Expect-{CT,Staple}
> and whatnot.

Yes but even once you put this, you hardly reach 1 kB. Most requests are
way below 1 kB headers, at least for performance reasons.

> I don't know what issues a too-small buffer for non-rewrites would
> cause.

This limits the maximum size of accepted messages (request or response),
which results in *some* requests to be rejected due to (for example) very
large cookies. Overall, I'd say that the number of problem reports due to
rejected requests or reponses due to their size has dropped to almost zero
since we've limited the value to 1 kB. A long time ago it used to be a very
common report.

> Clearly the body must be able to span multiple buffers already,
> otherwise I would not be able to send bodies greater than 16kB.
> Will it need to allocate more buffers to do the same work, because every
> single one is smaller?

No, the body is simply streamed, not stored. If however you need to analyse
it (eg for a WAF) then you need to configure a larger buffer so that more
of the body can fit.

To give you an idea about how things currently work, when we perform an
recvfrom() call, we decide whether we read up to bufsize or up to
(bufsize-maxrewrite) depending on whether we are forwarding data or
still analysing the message. Thus when we receive a POST, we're not yet
forwarding, so up to 15 kB of headers+body are placed into the buffer,
leaving 1 kB available to add headers.

After the new native internal HTTP representation is implemented, I think
that the need for the maxrewrite will disappear, at the expense of using
one buffer for the headers and another one for the body, but we'll see.

Anyway we need to address the lack of error checking, and I really predict
some breakage here :-/

Willy

Reply via email to