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
