search

aboutsip / pkts

Pure java based pcap library capable of reading and writing to/from pcaps.
Other
198 stars 92 forks source link

Infinite loop on Sip Parser #106

Closed danielzt closed 5 years ago

danielzt commented 5 years ago

I'm having an infinite loop problem. Seems that the parser is trying to consume LWS or SWS.

It's kind rare to happen, I'm not sure if it's a mal formed SIP message, but the thread seems to enter in a loop.

Could you guys please take a look?

`"atf_wrk-3" #16 daemon prio=5 os_prio=0 tid=0x00007fc5004dd000 nid=0x408 runnable [0x00007fc4e830c000] java.lang.Thread.State: RUNNABLE at java.lang.Throwable.fillInStackTrace(Native Method) at java.lang.Throwable.fillInStackTrace(Throwable.java:783)

jonbo372 commented 5 years ago

If you could give me the pcap that experience the issue, that would be helpful. Without it, I can't do much.

danielzt commented 5 years ago

I'll try to get one. It happens only during weekend, probably an attack with mal formed sip message.

jonbo372 commented 5 years ago

why don't you add some defensive code in your function "checkMinimumFields" and when you catch the SipParseException then log the full SIP message.

danielzt commented 5 years ago

Actually, this is exactly the problem. I'm checking if FROM-TAG exists but cannot confirm it, since the function never brings the result back =( This is the code that lead to the loop:

try 
{
    if(msg.getFromHeader() == null)
        return "From invalid";

    if(msg.getFromHeader().getTag() == null) // Line 288
        return "From-Tag invalid";
    ...
}
catch (Exception e)
{
    return false;
}

Do you have any idea? I

jonbo372 commented 5 years ago

I will take a look at some point, feel free to check the code as well of course.

Also, i would strongly recommend not to catch all but rather to catch the SipParserException, return false from it and then all the other exceptions you need to log/handle appropriately. Perhaps there are lot of exceptions before you finally get stuck, which could shed some light into what's going on.

jonbo372 commented 5 years ago

btw, are you sure it's an infinite loop? based on the thread dump above, it doesn't look like it (because the stack trace is short and non-repetitive). If you get this, let's assume, bad SIP message (so missing from-tag), what does your stack do? Do you return a 400 to the UAC or do you perhaps just drop it? If you just drop it, is this over UDP? If yes, then the UAC will re-transmit and then the "attack" will increase.

jonbo372 commented 5 years ago

btw2, in your example above, that code can't compile. In the two if-statements you are returning String, in the exception clause, you're returning a boolean. So what is the actual real code you're using?

danielzt commented 5 years ago

First of all, thanks for the feedback. btw1 -> I'm waiting for the problem to happen again. Did inserted some defense code to break if there is a loop. I believe it's a loop, will have to wait until it happens again. btw2 -> You caught it =) I had to put some return, just put false to have something but it's clearly wrong.

danielzt commented 5 years ago

I believe the problem is here:

    private Buffer consumeUntil(final Buffer name) {
        try {
            while (this.params.hasReadableBytes()) {
                SipParser.consumeSEMI(this.params);
                final Buffer[] keyValue = SipParser.consumeGenericParam(this.params);
                ensureParamsMap();
                final Buffer value = keyValue[1] == null ? Buffers.EMPTY_BUFFER : keyValue[1];
                this.paramMap.put(keyValue[0], value);

                if (name != null && name.equals(keyValue[0])) {
                    return value;
                }
            }
            return null;
        } catch (final IndexOutOfBoundsException e) {
            throw new SipParseException(this.params.getReaderIndex(),
                    "Unable to process the value due to a IndexOutOfBoundsException", e);
        } catch (final IOException e) {
            throw new SipParseException(this.params.getReaderIndex(),
                    "Could not read from the underlying stream while parsing the value");
        }

    }

Because if it still has bytes to read (hasReadableBytes()) but during parse it finds some exception internally (consumeSWS has a try/catch exactly for the SipParseException) , it could enter in a loop. I inserted a defense code here and if it reach 2k iterations, it will force an exception.

Will report as soon problem happens. Thanks

jonbo372 commented 5 years ago

Alright, found out what potentially can be the issue. At least I'm able to verify the existence of a loop in a unit test. See the unit tests added in this branch: https://github.com/aboutsip/pkts/tree/issue-106_Infinite_loop_on_Sip_Parser and the commented out test cases here: https://github.com/aboutsip/pkts/blob/90145db74409b4d3400263f7532fc5a2a988bd20/pkts-sip/src/test/java/io/pkts/packet/sip/SipRequestTest.java#L105

jonbo372 commented 5 years ago

Alright, found several issues. All confirmed though unit tests. Fixed them all and everything is now passing. Now, I'm not sure if that is exactly what you ran into but even so, it needed to get fixed and hopefully it is the same issue. See https://github.com/aboutsip/pkts/pull/108

danielzt commented 5 years ago

Hi Jonas The servers that I was monitoring didn't had any problem this weekend. I'll put the fixed code you just commited and check if it happens in the future. Thanks for working on a solution!

jonbo372 commented 5 years ago

ok, I'll merge the pull request in the next few days unless you report an issue with it. If you don't, I assume it may have fixed things but if you do find out exactly what happened in your case, please report it so we can write a unit test for it and verify it is actually what happened.

danielzt commented 5 years ago

Hi Jonas Looks like the problem is solved, I had no single problem like this on past days. Thanks for fixing! Regards

jonbo372 commented 5 years ago

I merged the fix and made a new release (3.0.5). It should be out on maven central in a few hours. Thanks for reporting!