< Previous by Date Date Index Next by Date >
< Previous in Thread Thread Index Next in Thread >

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



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

Attachment: smime.p7s
Description: S/MIME cryptographic signature