[squid-dev] [PATCH] Reject or sanitize more problematic Content-Length values

[Amos Jeffries]

On 3/09/2016 4:36 a.m., Alex Rousskov wrote:
> On 09/02/2016 09:05 AM, Amos Jeffries wrote:
>> On 2/09/2016 11:21 a.m., Alex Rousskov wrote:
>>> This change handles multiple Content-Length values inside
>>> one header field, negative values, and trailing garbage. Handling the
>>> former required a change in the overall Content-Length interpretation
>>> approach (which is why it was previously left as a TODO).
> 
> 
>> +1. Looks okay to me. With one caveat:
>>
>> Since there are several of these interpreters for special-cased headers
>> I think we need to define a sub-namespace for header
>> interpreters/parsers, etc. 
> 
> I am not sure I understand or agree with your logic for deciding when to
> add a namespace. A namespace is primarily a name clash avoidance
> mechanism. What name clashes are you trying to avoid by adding a "header
> interpreters/parsers" namespace?
> 
> As an illustration, consider the standard library that provides a lot of
> things, including a lot of similar things (e.g., stream classes or math
> functions). How many namespaces does std contain? I knew of only a
> couple before I looked it up. There are actually about a dozen
> "standard" ones, but all the ones I checked are concerned with name
> clashes, not just grouping similar things together.
> 
> 
>> Does keeping the existing "Hdr" term seem like a good idea to you?
> 
> Not sure what you mean. Are you suggesting to rename
> ContentLengthInterpreter to ContentLengthHdrInterpreter? That looks like
> a clash of naming styles to me.
> 

I meant Http::Hdr:: , so the things currently namesd HttpHdrFoo can be
moved there alongside this one as they get updated.


> 
>> * If so please move ContentLengthInterpreter to a Http::Hdr:: namespace
>> and ContentLengthInterpreter.{h,cc} files in a matching directory.
>>
>> * Otherwise, please at least place the class within the Http:: namespace
>> and directory etc.
> 
> I recommend leaving ContentLengthInterpreter class name as is. The class
> interprets Content-Length headers to find their intended meaning during
> parsing. It is not meant to be an Http::ContentLength class that
> stores/manages/writes parsed Content-Length information.
> 
> If that is not going to create linking problems, I can move the
> ContentLengthInterpreter class into http/one/ContentLengthInterpreter.*
> files and place it inside the existing Http::One namespace. Would that
> be sufficient to address your concerns?

It interprets the header in context of RFC7230 (HTTP) rules about what
Content-Length can contain. Other protocols should have their own rules
and handling. ie whether hex representation is permitted, extra ';q='
parameters, and such.

If any other protocol wants to re-use the HTTP rules enforced by this
class they can typedef in their own namespace, or use the Http::Foo
class directly. Which makes the relationship (copying HTTP
interpretation rules) to HTTP explicit in the other protocols code.

Http::One is not appropriate since these rules also apply to Http::Two::
mime header blocks as well. So somewhere under Http:: is right
regardless on whether the Http::Hdr:: idea is used or not.


> 
> 
>> (PS. I plan to bundle next 4.x within the next 2-3 days. I leave it to
>> you to decide whether this gets applied now or after release).
> 
> I would prefer to commit now, but that requires reaching an agreement
> regarding ContentLengthInterpreter name and location as discussed above.
> 

I would realy like it to be under Http:: and in http/ the rest is okay
to skip.

Amos