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.

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.

>> But that does not mean that the current
>> documented behaviour is a good behaviour.
> 
> Absolutely. What I'm much more worried about now is that a number of
> deployments may already accidently be relying on this problem and may
> discoverr 400s or 502s the hard way after we fix it :-(  So we'll have
> to be very careful about this and enumerate what works and what fails
> to figure if there's a real risk that people rely on this or not. If so
> we may need to add an extra option to relax addition in case of failure
> to preserve the current behaviour, which I'd personally hate.
> 

I've outlined above what exactly fails where in my case (i.e. missing
return value checking).

It might make sense to enlarge the rewrite buffer reservation by
default. I can absolutely imagine that people put in a ton of
information in the times of Content-Security-Policy, Expect-{CT,Staple}
and whatnot.
I don't know what issues a too-small buffer for non-rewrites would
cause. 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?

Best regards
Tim Düsterhus

Reply via email to