Re: [reSIProcate] Hardening the parser (what goes into 1.0?)
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.
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.
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
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.
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