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:
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?
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 30-03 16:06, Daniel-Constantin Mierla wrote:
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?
Yes, I agree, returning an error seems appropriate to me.
Jan.
On Mar 30, 2009 at 16:06, Daniel-Constantin Mierla miconda@gmail.com wrote:
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.
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).
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.
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:
On Mar 30, 2009 at 16:06, Daniel-Constantin Mierla miconda@gmail.com wrote:
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.
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).
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()?
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.
It's fixed automatically in sip-router, if the destination protocol is tcp or tls.
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:
Hello,
On 03/30/2009 05:24 PM, Andrei Pelinescu-Onciul wrote:
On Mar 30, 2009 at 16:06, Daniel-Constantin Mierla miconda@gmail.com wrote:
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.
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).
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()?
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 :-)
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.
It's fixed automatically in sip-router, if the destination protocol is tcp or tls.
Others reaction was to 400-reply the request, being a basic bug that should be fixed in uac side...
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
Andrei Pelinescu-Onciul writes:
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 is very bad idea to make proxy owner to pay for bugs in UAs. if proxy returns an error to UA, it is the best incentive for the UA owner to fix the bug.
-- juha
On Mar 30, 2009 at 18:12, Juha Heinanen jh@tutpro.com wrote:
Andrei Pelinescu-Onciul writes:
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 is very bad idea to make proxy owner to pay for bugs in UAs. if proxy returns an error to UA, it is the best incentive for the UA owner to fix the bug.
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
Andrei Pelinescu-Onciul writes:
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.
if that is the case, then i agree with you. proxy code should not do such thing and if it does t is clearly a bug in the proxy code. i wonder in how many places k currently trusts content-length.
-- juha
On 03/30/2009 06:27 PM, Juha Heinanen wrote:
Andrei Pelinescu-Onciul writes:
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.
if that is the case, then i agree with you. proxy code should not do such thing and if it does t is clearly a bug in the proxy code. i wonder in how many places k currently trusts content-length.
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:
On 03/30/2009 06:27 PM, Juha Heinanen wrote:
Andrei Pelinescu-Onciul writes:
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.
if that is the case, then i agree with you. proxy code should not do such thing and if it does t is clearly a bug in the proxy code. i wonder in how many places k currently trusts content-length.
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.
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:
On Mar 30, 2009 at 21:48, Daniel-Constantin Mierla miconda@gmail.com wrote:
On 03/30/2009 06:27 PM, Juha Heinanen wrote:
Andrei Pelinescu-Onciul writes:
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.
if that is the case, then i agree with you. proxy code should not do such thing and if it does t is clearly a bug in the proxy code. i wonder in how many places k currently trusts content-length.
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.
If the abort() wouldn't have been there, you wouldn't have discovered this bug.
well, this is questionable, lot of bugs are reported by error messages in syslog.
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
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:
On Mar 30, 2009 at 16:06, Daniel-Constantin Mierla miconda@gmail.com wrote:
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.
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).
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.
It's fixed automatically in sip-router, if the destination protocol is tcp or tls.
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:
Andrei Pelinescu-Onciul schrieb:
On Mar 30, 2009 at 16:06, Daniel-Constantin Mierla miconda@gmail.com wrote:
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.
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).
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.
It's fixed automatically in sip-router, if the destination protocol is tcp or tls.
How can this be fixed automatically? How will the proxy know when then body ends and the next message starts?
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.
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
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:
On Mar 31, 2009 at 10:57, Klaus Darilion klaus.mailinglists@pernau.at wrote:
Andrei Pelinescu-Onciul schrieb:
On Mar 30, 2009 at 16:06, Daniel-Constantin Mierla miconda@gmail.com wrote:
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.
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).
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.
It's fixed automatically in sip-router, if the destination protocol is tcp or tls.
How can this be fixed automatically? How will the proxy know when then body ends and the next message starts?
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.
Ups. I have overseen the word "destination", sorry.
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
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)).
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:
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?
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:
2009/3/30 Daniel-Constantin Mierla miconda@gmail.com:
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?
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?
was content length higher than body length?
Cheers, Daniel
2009/3/30 Daniel-Constantin Mierla miconda@gmail.com:
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?
was content length higher than body length?
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:
2009/3/30 Daniel-Constantin Mierla miconda@gmail.com:
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?
was content length higher than body length?
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.
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:
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
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:
2009/3/30 Iñaki Baz Castillo ibc@aliax.net:
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
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:
2009/3/30 Iñaki Baz Castillo ibc@aliax.net:
2009/3/30 Iñaki Baz Castillo ibc@aliax.net:
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
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
:-) 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:
2009/3/30 Iñaki Baz Castillo ibc@aliax.net:
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
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.
too late to revert some previous changes... :-)
Cheers, Daniel
On Monday 30 March 2009 15:06:05 Daniel-Constantin Mierla wrote:
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.
The proxy could also jump to error_route when it find such an error. The admin can then decide what to do with the message.
-
Alex Hermann -
Andrei Pelinescu-Onciul -
Daniel-Constantin Mierla -
Iñaki Baz Castillo -
Jan Janak -
jh＠tutpro.com -
Klaus Darilion