Re: API inconsistency in HttpHeader.cc?

Found the issue. I had inadvertently flipped a test in
HttpStateData::httpBuildRequestHeader, which caused Cc to be defined
but not to be filled-in.
So now that code-path is not taken anymore, and I will then merge the
playground branch soon.
These four methods IMVHO have however a weakness (not
security-related), which will be hopefully solved by StringNg.

Thanks for the eyeballs,
  Kinkie

On Tue, Oct 4, 2011 at 3:18 AM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> On Mon, 03 Oct 2011 17:31:30 -0600, Alex Rousskov wrote:
>>
>> 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.
>>
>
> Ah, Right.
>
>>
>>> 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).
>
> For this case I think we want to have a header-blob construct. Essentially
> what I'm expecting the HeaderParser to produce after SBuf conversion:
>  - a 'parent' SBuf pointing to the start of the header line and running to
> terminal '\n'.
>  - a 'label' SBuf pointing to the start of the header name and running to
> terminal ':'.
>  - a 'header-value' SBuf pointing to the start of the non-whitespace header
> values and running to terminal '\n'.
>
> When preserving originals we decide whether to output the second two as a
> pair, like we are supposed to for compliant syntax cleaning, resulting in
> trimmed whitespace garbage. Or for fully transparent, dump the first back
> onto the wire.
>
>>
>>  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.
>
> I think we may have this as a unreported problem. There was a comment in
> HTTPbis about proxies emitting empty headers questioning what to do about
> storage in that case. So likely there is a portion of the user base causing
> or experiencing this brokenness.
>
>>
>>  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.
>
> Okay. I stand corrected.
>
>
> Amos
>
>

-- 
    /kinkie