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

[Byron Campen]

On Aug 7, 2006, at 12:29 PM, Cullen Jennings wrote:

>
> Reduced people on thread as list server seemed to complain ...
>
> Yah - makes sense what you are saying few bits inline...
>
>
> On Aug 7, 2006, at 10:14 AM, Byron Campen wrote:
>
>> Comments inlined:
>>
>>>
>>>> 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.
>>
>
> There are some people sending 100k MESSAGE messages over TCP with  
> resip but this is sort of evil .... anyways, I agree 640 K should  
> be enough for anyone :-) Seriously 32 bits in totally fine here and  
> everywhere else I can think of too.
>
>>>
>>>> 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.
>
> Yes, when doing Dos protection have to be careful that the code you  
> put in to protect from DoS does not actually make the problem worse.
>
>>
>>>
>>>>
>>>> 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.
>>>
>>
>>
>>
>> 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.
>
> The old vovida stack used has a pedantic mode and it was somewhat  
> useful. In the end we defaulted it off but could flip it on. Mostly  
> people turned it on only when trying to debug a network. One thing  
> this mode could do is make the stack parse every one of the headers.
>
>
>

	This would be trivially easy to do now that we have a  
SipMessage::parseAllHeaders(). The thing is, in many cases, the parse  
code fails in such a way that is it impossible for the app to tell  
that something went wrong (ie; integral overflow results in a value  
like 4. Everything looks okay, but it is not.)

Best regards,
Byron Campen

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20060807/6e3bfbca/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/6e3bfbca/attachment.bin>