Re: API inconsistency in HttpHeader.cc?

On 10/03/2011 04:42 PM, Amos Jeffries wrote:
> On Mon, 03 Oct 2011 10:09:42 -0600, Alex Rousskov wrote:
>> On 10/03/2011 08:37 AM, Kinkie wrote:
>>> Hi all,
>>> while working on playground (HttpHdrCc c++-ification, mostly), I
>>> stumbled upon something which worries me a bit, and I wonder why it is
>>> not causing issues.
>>>
>>> HttpHeader.cc defines a few functions which add headers, I'm zoning in
>>> onto HttpHeader::putCc as is the one I've been looking at the most.
>>> Here it is:
>>>
>>> void
>>> HttpHeader::putCc(const HttpHdrCc * cc)
>>> {
>>> MemBuf mb;
>>> Packer p;
>>> assert(cc);
>>> /* remove old directives if any */
>>> delById(HDR_CACHE_CONTROL);
>>> /* pack into mb */
>>> mb.init();
>>> packerToMemInit(&p, &mb);
>>> cc->packInto(&p);
>>> /* put */
>>> addEntry(new HttpHeaderEntry(HDR_CACHE_CONTROL, NULL, mb.buf));
>>> /* cleanup */
>>> packerClean(&p);
>>> mb.clean();
>>> }
>>>
>>> The problem is that addEntry is initialized from the raw storage of a
>>> MemBuf (filled-in via packer), expecting a NULL-terminated c-string
>>> value.
>>> Which may very well not be the case. For instance, if noone as written
>>> anything to the MemBuf.
>>
>> I suspect it was impossible for the old code to call putCc with an empty
>> Cache-Control header under normal conditions. Abnormal conditions were
>> rare, [nearly] never fatal because the buffer is [usually] zeroed on
>> allocation, and so the bug was never noticed.

I should have made my comments less specific to the empty CC header
case. What Kinkie is asking about is null-termination which is not
specific to empty CC. Kinkie just used an empty CC as an obvious example
of a not explicitly terminated mb.buf.

> Empty CC is invalid HTTP. If you are creating an empty CC header
> something is wrong elsewhere in the code.

While it is true that empty CC on the wire is syntactically invalid, I
can think of three cases where an object representing it may appear
inside Squid:

  1. Squid trying to preserve original headers instead of trying to
"fix" them. I do not think we do that to CC now, but many proxy admins
want their proxies to be minimally invasive. Yes, this creates
smuggling-like exploitation opportunities but so is fixing headers (it
is essentially two sides of the same coin).

  2. Squid obeying configuration options or some kind of internal logic
that removes an existing CC directive. I am not sure there are cases
like that now, but that is not important for this thread. There is no
code that checks that removing a directive does not lead to an empty
cache control header. Moreover, I do not think we need such code as it
would make callers life difficult.

  3. Debugging. We may print an "under construction" CC header that is
currently empty. I doubt we do that now, but I doubt an audit would
catch this if we start doing it later. And debugging code is not the
place to check for header validity anyway.

Until we start doing #1, we can simply check for an empty CC header when
packing it to solve #2 and #3 (but this is not what Kinkie is asking
about, I think).

> Same for range and SC headers.
>
> In the case of SC replacing a CC with nothing there will still be a '
> field="" ' string value to set.
>
>>
>>> A possible solution could be to mb.terminate() just before addEntry,
>>> but that has its own problems: if the MemBuf doesn't have the space to
>>> grow for appending the trailing \0, it will assert (see XXX on
>>> MemBuff::terminate() ).
>>>
>>> putContRange, putRange, putSc all share the same blueprint and,
>>> potentially, issue.
>>>
>>> I wonder why this is not biting us, and how to best address this. Any
>>> ideas or suggestions?
>
> I think the code which needs to just clear the header is being compliant
> and using delById() API instead of the put*() API with no value.
>
>>
>> I suggest the following:
>>
>> 1) Add a safe convertion from MemBuf to String (the new explicit
>> String constructor or a global function will have access to MemBuf
>> length so it will not need to terminate the MemBuf).
>>
>> 2) Add a HttpHeaderEntry constructor that accepts const MemBuf
>> reference as header value.
>>
>> This approach will not increase the number of copies we make, will not
>> change overall header handling logic, and will avoid unterminated buffer
>> use.
>>
>
> This may be needed anyway, and/or a good idea. But I think its a fix for
> the wrong problem here.

If I interpreted Kinkie's concern correctly, the situation is reversed:
Empty CC is the "wrong problem" as he is concerned about not explicitly
terminated buffer which may happen even if CC is not empty as packing
API does not guarantee termination, IIRC. My suggestions are meant to
address that concern only.

Cheers,

Alex.
Received on Mon Oct 03 2011 - 23:31:40 MDT