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 <[email protected]> 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
