[reSIProcate] Hardening the parser (what goes into 1.0?)

[Byron Campen]

Comments inlined:

>
> I have to admit I have not been typing much code into resip lately  
> so weight my opinion accordingly .....
>
> First, I would love to see the stack become rock solid, never  
> crash, pass all the evil test suite stuff etc ...
>
> Second, I like the stack is receives lots of things - much of it  
> being broken but sends good stuff if the applications wants
>
> Third, we took a very strong position that checking if things were  
> correct in the stack just slowed things down and was undesirable.  
> Now the stack should never crash, but if you send it a bad message,  
> I'm not sure it should catch that.
>

	I agree mostly, except for the cases where we parse some garbage,  
and end up throwing entirely different (and possibly worse) garbage  
onto the wire. For example, suppose we get an overlarge CSeq as a  
proxy, parse it incorrectly (and we do parse CSeq to get the method  
from responses), throw an entirely different garbage CSeq onto the  
wire, but then subsequent traffic doesn't pass through us so the CSeq  
"changes", causing all kinds of hard-to-diagnose-but-probably-blamed- 
on-us fun.

> For "protocol repair engines" the stack needs to be able to receive  
> many broken things and pass them up to the application for repair.
>
>
> On Aug 7, 2006, at 8:55 AM, Byron Campen wrote:
>
>> 	A summary of the issues that I have found so far in the parse  
>> code while working with the torture tests:
>>
>> 1. ParserCategory will accept empty and repeated parameters.
> I seem to recall there was some stuff out in the wild that did this  
> but can't remember if it was just repeated or what it is was.  
> Perhaps someone else knows. I don't care too much one way or the other
>
>> 2. In general, overflow checking is not performed on header-field- 
>> values/parameters that take integral values. The parse code is  
>> yielding garbage when these are not within the range of an int (in  
>> most cases, this is not enough to handle what the RFCs say are  
>> valid).
> It irritates me off that the spec refuses to say what the upper  
> bound of say content length is. My personal opinion is that the  
> performance of 64 bit ints on the machines we use is about the same  
> and we should just use either 32 or 64 bit ints for this and call  
> it done.
>
	Given that the maximum size of a SipMessage is 64K, I think it is  
safe to assume that a UInt32 will be enough.

>
>> 3. Content-Length is not processed according to RFC 3261 Sec 18.3  
>> (ie, it is ignored in the UDP case)
> As a practical matter, proxies end up doing better when they send  
> the body they got not the truncated version but I don't care.
>
>> 4. Multiple values are allowed (and re-emitted) in single-value  
>> headers.
>>
>> On point 1:
>> 	We haven't taken any action yet. David queried whether it had  
>> been decided that tracking uniqueness of parameters was too  
>> expensive. I made the point that this could be leveraged in a DoS  
>> attack by putting very large numbers of empty parameters (ie. semi- 
>> colons) in a header-field-value, causing the data structures used  
>> to hold these parameters to grow very large.
> The DOS attack could equally well be done by having a bunch of  
> parameters a1, a2, a3 etc so if we are going to deal with the DOS  
> issues, I would prefer some approach that just limited the memory a  
> single message could use in total and not do this

	Not equally; it would be about 1/3 as effective (or if they're doing  
a,b,c etc, 1/2 as effective). I feel conflicted about setting upper  
limits on the number of parameters we will allow; on the one hand, it  
kills this issue dead, but on the other, it limits functionality.  
Checking for repeated params entails using a more complicated data  
structure that will really get us into trouble if someone DoS's us in  
this manner. I figure we should at least ignore empty parameters  
though, since it is very light work.

>
>>
>> On point 2:
>> 	I have some code in a working-copy that has (mostly) replaced  
>> IntegerCategory and IntegerParameter with the more descriptive  
>> (and overflow safe) UInt32Category, UInt8Category,  
>> UInt32Parameter, and UInt8Parameter. All the necessary changes to  
>> Headers.?xx, HeaderTypes.hxx, SipMessage.?xx, ParameterTypes.?xx,  
>> ParameterTypeEnums.hxx, and ParserCategory.?xx have been made. Do  
>> people want to see this in a branch before we work it into the  
>> main trunk? Also, is this something we want to push into the release?
> Is there any advantage to UInt8 over always using UInt32? Again, I  
> don't really care.
>
	There are a couple of integral values whose upper bound is 255  
(namely, Max-Forwards and the ttl parameter). This simplifies the  
overflow/bounds checking somewhat. We could just as easily handle  
these with UInt32, and allow applications to fix invalid values if  
they wish.

>>
>> On point 3:
>> 	Robert has suggested that the stack's treatment of Content-Length  
>> should be tunable; by default it would function according to sec  
>> 18.3, but it could be configured to behave as it does now. I have  
>> not implemented the configurable behavior yet, but I do have code  
>> that implements 18.3, again in a working copy. Does this need to  
>> make it into a branch first? Is this something we want to put in  
>> the release? (Note that 2 and 3 are closely-tied here; if we are  
>> going to be taking Content-Length seriously, we NEED overflow-safe  
>> integral value parse code, or we will end up dumping core when  
>> someone sends a bad Content-Length value at us.)
> Configurable addresses the concern I had...
>
>>
>> On point 4:
>> 	I have code in a working copy that checks for this problem within  
>> SipMessage::addHeader(). Once again, does anyone want to review  
>> this stuff in a branch, and do we think this should make it in 1.0?
>>
>> Best regards,
>> Byron Campen


Maybe we could have a "pedantic" mode that the stack could run in,  
that performs thorough checking for garbage. This could be incredibly  
useful for interop testing/protocol debugging.

Best regards,
Byron Campen

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20060807/a60af55b/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2369 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20060807/a60af55b/attachment.bin>