Hello,
the data lumps system is critically affected if content-length is wrong for UDP. The anchor_lump() calls abort() in the case content-length is higher than actually body length. This can be prevented by called sanity module to check the content length, however, I consider being too drastic to have abort in such case, it would be better to return an error and let the sip router process other messages. Opinions?
Checking sip-router sources, it faces same issue.
Another option would be to correct the C-L value locally, but the right one is that phone vendor fixes its side.
Cheers, Daniel
Daniel-Constantin Mierla writes:
i vote for returning an error.
Another option would be to correct the C-L value locally, but the right one is that phone vendor fixes its side.
this is such a radical bug in the UA that fixing it in the proxy does not make sense.
-- juha
On Mar 30, 2009 at 16:06, Daniel-Constantin Mierla miconda@gmail.com wrote:
It's true that anchor_lump() calls abort if the offset passed to it is outside the message, but I don't see where anchor_lump() is called with a value depending on Content-Length (at least in sip-router and ser).
It's fixed automatically in sip-router, if the destination protocol is tcp or tls.
Andrei
Hello,
On 03/30/2009 05:24 PM, Andrei Pelinescu-Onciul wrote:
I haven't checked the modules in ser, just the data_lump.c file for anchor_lump() in sip-router core. In k most of the modules takes the length from header. Probably a wrapper that corrects it would be good.
The issue remains, is this a case of runtime abort()?
Others reaction was to 400-reply the request, being a basic bug that should be fixed in uac side...
Cheers, Daniel
On Mar 30, 2009 at 17:46, Daniel-Constantin Mierla miconda@gmail.com wrote:
It's an abort() to quickly catch bugs (the content length value should always be checked and _never_ trusted) and to force people to fix them.
We could eliminate the abort() but then the incentive for fixing the real bug will be reduced :-)
It costs us very little to fix it and so I think it's better to be nice, even with broken UACs. If one wants to drop them, it can always use the sanity module.
Andrei
On Mar 30, 2009 at 18:12, Juha Heinanen jh@tutpro.com wrote:
It's not a bug in the UA, it's a bug in the proxy code that uses a Content-Length received from the network without checking if it's valid. All such code instances must be changed and Content-Length must always be checked and never trusted, before using it for anything. That's what the abort() is for.
So removing the abort() it would fix the symptom, but not the real bug.
Andrei
On 03/30/2009 06:27 PM, Juha Heinanen wrote:
This trust of content-length needs be fixed I agree. However it looks to me too radical to call abort() on purpose. A developer can fix that quickly, but users having deployed the sip router cannot coper properly with. Like in buffer overflow cases, the code detects the case and returns error, does not call abort(). I see this being similar. I would avoid abort() on purpose anywhere at runtime, but write error messages, avoid crash and keep running.
Cheers, Daniel
On Mar 30, 2009 at 21:48, Daniel-Constantin Mierla miconda@gmail.com wrote:
If the abort() wouldn't have been there, you wouldn't have discovered this bug. In general abort() is used only for important bugs and one shouldn't expect the proxy to survive using the api in the wrong way. We could try to workaround SIGSEGV too, but it's much better to let it coredump.
What can we do is to use some define, e.g.: #ifndef RELEASE abort() #endif
but this still would have delayed finding this bug a lot.
Andrei
On 03/30/2009 10:21 PM, Andrei Pelinescu-Onciul wrote:
well, this is questionable, lot of bugs are reported by error messages in syslog.
maybe this is better, a sr_abort(code) marcro that does either "abort()" or "return code" depending of compile mode (release or not).
but this still would have delayed finding this bug a lot.
The bug was identified manly by the syslog message printed before the abort(), not by the crash ...
Cheers, Daniel
Andrei Pelinescu-Onciul schrieb:
How can this be fixed automatically? How will the proxy know when then body ends and the next message starts?
regards klaus
PS: IMO the server should respond with "400 wrong content length" if accessing of the body is required and the length is wrong
On Mar 31, 2009 at 10:57, Klaus Darilion klaus.mailinglists@pernau.at wrote:
If the destination is tcp or tls, but the source is not (source is udp or sctp). One more clarification: it does not the fix the value of the content length visible from sr, it will fix the Content-Length header in the forwarded message.
Why would anyone use content-length for accessing the body in the first place? You always know (in sr/ser/k) where the message ends (msg->buf+msg->len) and where the body starts (get_body(msg)).
Andrei
Andrei Pelinescu-Onciul schrieb:
Ups. I have overseen the word "destination", sorry.
hmmm.
If input protocol is stream oriented, I guess the body-len is always correct, as SR has to use the content-length header to get the body.
When using UDP the question is: What should be used as body boundary - from CRLFCRLF to end of datagram or from CRLFCRLF "content-length" bytes? In any case, if SR detects a problem that those 2 methods return different result IMO rejecting with 400 is best.
klaus Stream oriented
Andrei
2009/3/30 Daniel-Constantin Mierla miconda@gmail.com:
Is this error the cause of so many segmentfaults in a reported bug?: https://sourceforge.net/tracker/?func=detail&aid=2649470&group_id=13...
I've tested it by sending spoofed SIP messages via UDP with wrong Content-Lenght and it's forwarded properly by Kamailio trunk (before the today commits), without warnings or errors. How to reproduce the problem?
On 03/30/2009 05:37 PM, Iñaki Baz Castillo wrote:
was content length higher than body length?
Cheers, Daniel
2009/3/30 Daniel-Constantin Mierla miconda@gmail.com:
I've sent a MESSAGE (UDP) with Content-Length lower and also higher than the real body size. Nothing occurred... But let me please check it again.
On 03/30/2009 05:55 PM, Iñaki Baz Castillo wrote:
wrong test. Send an invite and call force_rtp_proxy().
Cheers, Daniel
2009/3/30 Daniel-Constantin Mierla miconda@gmail.com:
wrong test. Send an invite and call force_rtp_proxy().
ohhh, right!
When Content-Lenght is higher than the real body size it causes a segmentfault!
Mar 30 17:03:24 [28414] CRITICAL:core:anchor_lump: offset exceeds message size (762 > 662) aborting... Mar 30 17:03:24 [28412] INFO:core:handle_sigs: child process 28414 exited by a signal 6 Mar 30 17:03:24 [28412] INFO:core:handle_sigs: core was generated Mar 30 17:03:24 [28412] INFO:core:handle_sigs: terminating due to SIGCHLD Mar 30 17:03:24 [28491] CRITICAL:core:receive_fd: EOF on 14
2009/3/30 Iñaki Baz Castillo ibc@aliax.net:
I've checked the last commit fixing this problem. The SDP is in fact modified in the request (not entirely since "a=nortpproxy" line is not added) but force_rtpproxy() returns -1. Is it the expected behaviour? I would prefer that SDP is not modified at all.
2009/3/30 Iñaki Baz Castillo ibc@aliax.net:
Let me a question: Does this bug mean that *any* OpenSer/Kamailio version using "force_rtpproxy()" would crash if Content-Length is higher than the real body size? If so it's good time for individual consultans XD
On 03/30/2009 06:33 PM, Iñaki Baz Castillo wrote:
:-) kind of ... fixed now on latest three versions of kamailio, going to backport back to 1.3 and 1.2.
Cheers, Daniel
On 03/30/2009 06:27 PM, Iñaki Baz Castillo wrote:
too late to revert some previous changes... :-)
Cheers, Daniel
-
Alex Hermann -
Andrei Pelinescu-Onciul -
Daniel-Constantin Mierla -
Iñaki Baz Castillo -
Jan Janak -
jh＠tutpro.com -
Klaus Darilion