Asterisk
  1. Asterisk
  2. ASTERISK-25397

[patch]chan_sip: File descriptor leak with non-default timert1

    Details

      Description

      From the issue reporter:

      For T1, RFC 3261 suggests a default of 500 ms. For mobile networks, 3GPP TS
      24.229 recommends 2000 ms. Nokia used 3000 ms. Therefore and because of
      personal experience, I changed the parameter timert1 in my sip.conf to a
      value higher than 1246. Since then, around 1000 file descriptors (UDP ports)
      leak every 10 days, because my SIP server is attacked/scanned continuously.

      Thanks to CLI "core show fd" and "sip show sched", I was able to break it
      down to a counter overflow in the variable "siptimer_a". That variable was
      treated as unsigned although it is signed. After fixing this, I noticed
      another counter overflow in "sip_pkt->timer_a". I was able to remove
      "sip_pkt->timer_a" completely because it can be calculated from
      "sip_pkt->retrans" at any time. Because of this, the scheduler is not called
      with a negative number anymore. Because of this, the scheduler does not
      convert this negative number into a silly high number, effectively never
      releasing the UDP port. Therefore, I do not face a file descriptor leak
      anymore. To catch similar negative-value issues, I added a WARNING within
      the scheduler.

      Steps to reproduce

      00. Ubuntu 14.04 LTS
      01. apt-get install build-essential libssl-dev libncurses-dev libnewt-dev
      libxml2-dev linux-headers-$(uname -r) libsqlite3-dev uuid-dev libjansson-dev
      libbfd-dev
      02. wget
      downloads.asterisk.org/pub/telephony/asterisk/asterisk-12-current.tar.gz
      03. tar zxf asterisk*
      04. cd asterisk*
      05. ./configure
      06. make menuselect.makeopts
      07. ./menuselect/menuselect --enable DONT_OPTIMIZE --enable
      BETTER_BACKTRACES --enable DEBUG_FD_LEAKS
      08. make install config samples
      09. changed /etc/asterisk/sip.conf to include timert1=1250
      10. CLI: core set debug 4
      11. sendip -p ipv4 -is 89.163.146.62 -p udp -us 5071 -ud 5060 -f
      ./message.txt 127.0.0.1
      You do not have to change the last command and the message to your IP
      addresses. By the way, that is a message send by one of my spammers.

      Expected behavior

      After the re-transmission of the status "404 Not Found", the related UDP
      ports should be closed and the corresponding file descriptors released.

      Actual behavior

      CLI: core show fd
      CLI: sip show sched
      show one packet and two (sometimes only one?) leaked UDP port, even after
      days. The scheduler has one outstanding packet with a re-schedule in more
      than 25 days.

      Additional Notes

      SIP timers got introduced with the feature request/report
      ASTERISK-4257/-5187, supplied by Olle Johansson. Therefore, Olle is CCed on
      this report. Perhaps Olle is able to look-over and comment my proposed patch
      as well. Because this code was added exactly ten years ago, I am curious why
      I am the first one reporting this issue. Perhaps, I misunderstand something
      or the cause is somewhat different (the amount of re-transmissions changed
      since the original code).

      This E-mail is not encrypted because the Asterisk team had issues with my
      PGP mails in the past. Unfortunately, I was not told which PGP software the
      Asterisk team is using. Therefore, I could not debug this. Anyway, this
      issue is already exploited by hackers - I learned about this when my server
      ran out of available file descriptors - therefore this issue is somewhat
      public.

      1. jira_asterisk_25397_v13_v2.patch
        0.6 kB
        Richard Mudgett
      2. jira_asterisk_25397_v13.patch
        0.6 kB
        Richard Mudgett
      3. siptimerA_overflow.patch
        2 kB
        Matt Jordan

        Issue Links

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

          Hide
          Matt Jordan added a comment -

          siptimerA_overflow.patch: Proposed patch by Alexander Traud.

          Show
          Matt Jordan added a comment - siptimerA_overflow.patch : Proposed patch by Alexander Traud.
          Hide
          Richard Mudgett added a comment -

          jira_asterisk_25397_v13.patch - Patch expected to be committed to fix the issue.

          Show
          Richard Mudgett added a comment - jira_asterisk_25397_v13.patch - Patch expected to be committed to fix the issue.
          Hide
          Richard Mudgett added a comment -

          I have been able to reproduce the issue and created jira_asterisk_25397_v13.patch.
          Could you test the updated patch please.

          Show
          Richard Mudgett added a comment - I have been able to reproduce the issue and created jira_asterisk_25397_v13.patch . Could you test the updated patch please.
          Hide
          Alexander Traud added a comment - - edited

          Mhm. My code has one bug because the overflow still happens (and an additional bug because I do not bit-shift, actually). And bit-shifting that far is undefined behaviour. I am thinking about a clean working solution right now. If anyone is faster, please, your are welcome!

          Because multiplication does not create undefined behaviour in C, the other proposed patch is safe in that regard. However, I am investigating if (siptimer_a < pkt->timer_t1 * (pkt->timer_a / 2)) right now. Shouldn’t that be if (pkt->timer_t1 != siptimer_a / pkt->timer_a) to detect the overflow reliably? I am still investigating.

          Show
          Alexander Traud added a comment - - edited Mhm. My code has one bug because the overflow still happens (and an additional bug because I do not bit-shift, actually). And bit-shifting that far is undefined behaviour . I am thinking about a clean working solution right now. If anyone is faster, please, your are welcome! Because multiplication does not create undefined behaviour in C, the other proposed patch is safe in that regard. However, I am investigating if (siptimer_a < pkt->timer_t1 * (pkt->timer_a / 2)) right now. Shouldn’t that be if (pkt->timer_t1 != siptimer_a / pkt->timer_a) to detect the overflow reliably? I am still investigating.
          Hide
          Richard Mudgett added a comment -

          jira_asterisk_25397_v13_v2.patch - Updated patch about concerns of undefined behaviour for the multiplication. The link about undefined behaviour had a response that works better that I've implemented in the second version of the patch.

          Show
          Richard Mudgett added a comment - jira_asterisk_25397_v13_v2.patch - Updated patch about concerns of undefined behaviour for the multiplication. The link about undefined behaviour had a response that works better that I've implemented in the second version of the patch.
          Hide
          Alexander Traud added a comment - - edited

          Patch tested, works here.

          Show
          Alexander Traud added a comment - - edited Patch tested, works here.
          Hide
          Alexander Traud added a comment -

          Just a note, because this patch did not made into Asterisk 13.6.0/11.20.0 and the previous status of this issue here was ‘awaiting response’: Please let me know if you need anything else from me.

          Show
          Alexander Traud added a comment - Just a note, because this patch did not made into Asterisk 13.6.0/11.20.0 and the previous status of this issue here was ‘awaiting response’: Please let me know if you need anything else from me.
          Hide
          Richard Mudgett added a comment -

          No. The patches are just waiting for a security release which is expected soon since there is more involved in security releases.

          Show
          Richard Mudgett added a comment - No. The patches are just waiting for a security release which is expected soon since there is more involved in security releases.
          Hide
          Michael Spiceland added a comment -

          Alexander Traud, thank you very much for the help and your patience. I'm following up on this issue to let you know that we're planning on doing the security release in January, after the Asterisk 13.7 release. I'll be able to give a more specific date closer to the actual date. Again, thank you.

          Show
          Michael Spiceland added a comment - Alexander Traud, thank you very much for the help and your patience. I'm following up on this issue to let you know that we're planning on doing the security release in January, after the Asterisk 13.7 release. I'll be able to give a more specific date closer to the actual date. Again, thank you.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development