Re: [reSIProcate-users] how big is a valid SIP body (SDP)? (Re: Issues building WebRTC branch)
On 25/03/13 04:05, Byron Campen wrote:
> Ok, the code that handles buffer allocation for the body runs from
> ConnectionBase.cxx:303-386. Here's the gist of it:
>
> - Parse the Content-Length header, or drop.
http://tools.ietf.org/html/rfc3261#section-20.14
Content-Length is actually optional for UDP and for WebSockets, because
they both have their own way of specifying the size of a message unit:
http://tools.ietf.org/html/draft-ietf-sipcore-sip-websocket-08#section-5.1
This has two consequences:
a) We should not assume it is always there
b) We should add a Content-Length header when necessary (e.g. proxying
from WebSocket to TLS or TCP)
If this appears undesirable, now is the time to comment on the draft.
> - Perform sanity-check on value of Content-Length, or drop.
I see 10485760 hardcoded there now (10MB), I would propose:
- making it configurable
- WebSocket code will use the same value when screening the size in a
payload size header
> - If Content-Length indicates that we have already read the entire body in
> a prior read (ie; while reading headers), do a little buffer management so
> that the next message is preserved (we've already read at least part of
> it), call SipMessage::setBody(), and move the message along.
> - Otherwise, alloc a buffer of either ChunkSize or half again the size of
> the body read so far, whichever is _bigger_, up to a maximum of the
> Content-Length.
>
> As more bytes of the body come in, whenever we run out of space, we (sort
> of) repeat the logic in the last step above (see ConnectionBase.cxx:503).
>
> In no case is ChunkSize acting as an upper bound on the size of a buffer
> allocation for a body. Initially it is used as a lower bound, though.
For traditional streams, I agree, it is not so bad. It is the way that
it has been slightly misused by the WebSocket code that leads to problems.
> In regard to the logging above, I've looked again at UdpTransport since I
> was not seeing any of the other logging I would expect to, but it seems
> that the debug logging in there has been commented out. I would bet you $10
> that what you just saw was a truncated UDP datagram come in due to
> fragmentation; the Content-Length says there are 4657 bytes of body, but
> the datagram ends 866 bytes after the headers.
Nathan's example was for WebSocket messages (not UDP), and it was using
the latest version of b-webrtc, which no longer tries to rearrange
mBuffer and force it back into preparseNewBytes (see the changes I made
about a week ago)
http://svn.resiprocate.org/viewsvn/resiprocate/branches/b-webrtc/resip/stack/ConnectionBase.cxx?r1=10045&r2=10046&diff_format=l
In the WebSocket code, ChunkSize has been used as a limit on buffer
size, and this is one of the main things I need to resolve
> Moral of the story? SIP+UDP bad. Just say no. :)
>
>
> Best regards,
> Byron Campen
>
> PS. This is not the only reason SIP+UDP is bad, either.
>
>
>
> On Sun, Mar 24, 2013 at 2:18 PM, Byron Campen <docfaraday@xxxxxxxxx> wrote:
>
>> Chunksize should not be a limit here; I have seen this code accept >16k
>> sdp routinely at sipit. Chunksize is only used for the header, and only on
>> a per-read basis (this can limit the maximum size of a single
>> header-field-value); for the body, we will allocate what the content length
>> specifies up to a limit, and then allocate as needed afterward (to prevent
>> a memory exhaustion attack similar to the one you called out). The warning
>> in the log is supposed to happen when the content length and end of udp
>> datagram are inconsistent (which is extremely likely when sending large sdp
>> over udp due to fragmentation). I can give this a look later today.
>>
>> Best regards,
>> Byron Campen
>> On Mar 24, 2013 5:46 AM, "Daniel Pocock" <daniel@xxxxxxxxxxxxx> wrote:
>>
>>>
>>>
>>> On 21/03/13 21:05, Nathan Stratton wrote:
>>>> I wanted to rule out JsSIP, so I just tried sipml5 and see the same
>>> thing:
>>>>
>>>> DEBUG | 20130321-200133.342 | repro | RESIP:TRANSPORT | 140720189728512
>>> |
>>>> TcpBaseTransport.cxx:263 | Processing write for [ V4
>>>> 176.209.218.197:39628TCP target domain=unspecified mFlowKey=33 ]
>>>> DEBUG | 20130321-200133.342 | repro | RESIP:TRANSPORT | 140720189728512
>>> |
>>>> ConnectionManager.cxx:59 | Found fd 33
>>>> INFO | 20130321-200134.434 | repro | RESIP | 140720189728512 |
>>>> SipMessage.cxx:931 | Content Length (4657) is 3791 bytes larger than
>>> body
>>>> (866)! (We are supposed to 400 this)
>>>
>>>
>>> I've found the root cause of this and can confirm there is an issue here:
>>>
>>> resip/stack/ConnectionBase.cxx
>>>
>>> The original code in the stack makes a read buffer no bigger than
>>> ChunkSize. Released versions of reSIP have ChunkSize 2048. On main
>>> (and b-webrtc), ChunkSize is 4096, but it is still not enough for large
>>> SDP apparently. Even so, the logic appears to be geared for
>>> preparseNewBytes(), which has some logic for expanding the buffer in
>>> certain situations, but not always enough anyway. However, the way that
>>> the original patch pushed WebRTC data into preparseNewBytes() didn't
>>> fully benefit from this anyway.
>>>
>>> The original WebRTC patch also bumped ChunkSize to 0xFFFF, but this has
>>> two problems:
>>>
>>> a) for people who want memory-efficiency, 0xFFFF is a lot of waste
>>>
>>> b) for any future application that sends some kind of jumbo-SDP (or
>>> other SIP message bodies), it may still be less than big enough
>>>
>>> -- so nobody wins
>>>
>>> A simple approach would involve expanding the buffer on demand. For a
>>> WS frame, this would mean looking at the frame header for a clue about
>>> how much space is really needed. This could also expose the stack to a
>>> vicious DoS attack though, somebody could send a small WS frame with an
>>> invalid header specifying an obscenely large payload (up to 2^55 - 1),
>>> and a few mallocs later, the process is out of memory.
>>>
>>> Can anyone else comment on this issue, not just for WebSockets and SDP,
>>> but for any SIP message body?
>>>
>>> My initial feeling about it is that there should be dynamic buffer
>>> allocation up to a configurable limit, just as many web servers impose a
>>> maximum file upload size.
>>>
>>> On a side note, the constraint on NewMessage below appears to require a
>>> workaround implementing any non-textual wire formats such as SigComp and
>>> WebSocket, as they have to start the connection off in NewMessage state
>>> even though they will never operate in that mode.
>>>
>>> Nathan, I've bumped this to 8192 in the b-webrtc branch to see if I have
>>> correctly diagnosed the problem you experience. However, before the 1.9
>>> release, we will need to tune this properly so it doesn't come back to
>>> bite us again in future.
>>>
>>>
>>>
>>> std::pair<char*, size_t>
>>> ConnectionBase::getWriteBuffer()
>>> {
>>> if (mConnState == NewMessage)
>>> {
>>> if (!mBuffer)
>>> {
>>> DebugLog (<< "Creating buffer for " << *this);
>>>
>>> mBuffer =
>>> MsgHeaderScanner::allocateBuffer(ConnectionBase::ChunkSize);
>>> mBufferSize = ConnectionBase::ChunkSize;
>>> }
>>> mBufferPos = 0;
>>> }
>>> return getCurrentWriteBuffer();
>>> }
>>> _______________________________________________
>>> resiprocate-users mailing list
>>> resiprocate-users@xxxxxxxxxxxxxxx
>>> List Archive: http://list.resiprocate.org/archive/resiprocate-users/
>>>
>>
>