Bug 7787 - SIP server closes the TCP connection when it receives an invalid message
Summary: SIP server closes the TCP connection when it receives an invalid message
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low normal (vote)
Assignee: Marc Balmer
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-22 08:08 UTC by Marc Balmer
Modified: 2013-12-05 20:05 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Make the SIP server more robust by not exiting on invalid input, but rather issue a resend response. (4.23 KB, patch)
2012-03-22 09:38 UTC, Marc Balmer
Details | Diff | Splinter Review
Make the SIP server more robust by not exiting on invalid input, but rather issue a resend response. (4.24 KB, patch)
2012-03-22 11:40 UTC, Marc Balmer
Details | Diff | Splinter Review
RAW login can be as robust as telnet login (2.73 KB, patch)
2012-03-22 11:41 UTC, Marc Balmer
Details | Diff | Splinter Review
Combined diff for telnet and raw connections (5.94 KB, patch)
2012-03-22 13:12 UTC, Marc Balmer
Details | Diff | Splinter Review
Reformatted Patch (5.92 KB, patch)
2012-06-09 11:12 UTC, Colin Campbell
Details | Diff | Splinter Review
Signed off patch (5.77 KB, patch)
2012-06-19 11:48 UTC, Colin Campbell
Details | Diff | Splinter Review
[SIGNED OFF] Bug 7787: Make the SIP server much more robust. (5.83 KB, patch)
2012-06-20 19:36 UTC, Ian Walls
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Balmer 2012-03-22 08:08:52 UTC
When the SIP server receives an invalid message, it closes the TCP connection.  It should keep the connection open, but ignore the message and reply with a RESEND message (96).
Comment 1 Marc Balmer 2012-03-22 09:38:45 UTC Comment hidden (obsolete)
Comment 2 Marc Balmer 2012-03-22 09:39:31 UTC
Tested by me, needs signoff.
Comment 3 Marc Balmer 2012-03-22 10:16:16 UTC
To test this patch, you need to first start the SIP server, use the following command:

# perl -IC4/SIP/ -MILS C4/SIP/SIPServer.pm C4/SIP/SIPconfig.xml

In SIPconfig, you need to add a login and institution to be able to login:

  <accounts>
      <login id="arcapos" password="arcapos" delimiter="|" error-detect="enabled"
          institution="kohadev" />
  </account>

<institutions>
    <institution id="kohadev" implementation="ILS" parms="">
          <policy checkin="true" renewal="true" checkout="true"
                  status_update="false" offline="false"
                  timeout="100" retries="5" />
    </institution>
</institutions>

Then you need to create a patron with the same name (arcapos in the example above) and give that patron circulation rights.  Now you can use telnet to login to the SIP server:

# telnet localhost 8023

It will prompt for username/password.

Note how the server closes the connection when you just hit enter 3 time or send any garbage.  With the patch applied, the server will not close the connection.
Comment 4 Marc Balmer 2012-03-22 11:40:47 UTC Comment hidden (obsolete)
Comment 5 Marc Balmer 2012-03-22 11:41:23 UTC Comment hidden (obsolete)
Comment 6 Marc Balmer 2012-03-22 13:12:50 UTC Comment hidden (obsolete)
Comment 7 Marc Véron 2012-03-23 14:33:49 UTC
I applied patch 8511.

Then I followed  Marc Balmers test senario with following differences:

- Took /debian/templates/SIPconfig.xml as configuration file for SIPServer.pm (without changes)

- Created a patron koha/koha with circulation permisssions (as found in SIPconfig.xml)

- run script SIPServer.pm in console #1
-> console #1 displays something like:
--Snip-------------------
SIPServer config:
$VAR1 = bless( {
(...)
          'syslog_facility=local6'
        ];
--Snip-------------------

- then started telnet in console #2
- telnet localhost 8023
- was prompted for login

->console #1 says:
Use of uninitialized value $var in hash element at /usr/share/kohaclone/C4/Context.pm line 1024, <STDIN> chunk 2.

- Enter some garbage value in (telnet) console #2
-> telnet answer in console #2: 96
-> repeat several times, same answer
- Hit return (without input)
->Telnet connection closes
(Connection closed by foreign host.)

If I repeat again an hit return just after login, connection closes immediately.

Fazit:
- Entering garbage does not close connection
- Hit "Enter" immediately closes connection

- Error (Warning?) in Context.pm as mentioned above.
Comment 8 Colin Campbell 2012-06-09 11:12:08 UTC Comment hidden (obsolete)
Comment 9 Colin Campbell 2012-06-09 11:15:39 UTC
Not sure of the telnet logic as the connection is not broken after three failed logins sending garbage to a telnet port will permanently occupy that session(?)
could it generate a kind of DOS against a sip port in this way. Will try testing this.
Comment 10 Colin Campbell 2012-06-19 11:48:27 UTC Comment hidden (obsolete)
Comment 11 Colin Campbell 2012-06-19 11:51:53 UTC
I've run this against various scenarios with a test client and it does behave in a more standard compliant way. Shutdown is cleaner in most cases and it avoids the 'die mysteriously' behaviour that can cause clients to hang
Comment 12 Ian Walls 2012-06-20 19:36:02 UTC
Removes 3 strikes logic in favour of continuous monitoring of the input.  If bad input is passed, the RESEND message is returned, rather than the system hanging.  This seems like a solid way to go, in my opinion, but I don't have sufficient SIP clients to test against.

Personally, I'm inclined to sign off on this, but I'm not 100% it should pass QA just on my say so.  Would another member of the QA team please review this patch as well, preferably running it through some kind of live SIP client?  This is one of those occasions where having an extra set of QA-eyes will be very helpful.
Comment 13 Ian Walls 2012-06-20 19:36:08 UTC
Created attachment 10392 [details] [review]
[SIGNED OFF] Bug 7787: Make the SIP server much more robust.

Be liberal in what we accept, but strict in what we send:
Never exit the server process, but send a SC_RESEND message (96)
to the client if we received anything we don't understand.
 This is consistent with SIP server implementations of other ILSs.

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Signed-off-by: Ian Walls <koha.sekjal@gmail.com>
Comment 14 Ian Walls 2012-06-20 19:36:36 UTC
Comment on attachment 10362 [details] [review]
Signed off patch

Minor conflict with perl critic fixes... fixed now
Comment 15 Paul Poulain 2012-07-06 13:37:23 UTC
QA comment: This patch removes more code than it adds, ++

I can't test it on a true SIP server, but looking at the code, I agree with Ian, so I set passed QA
Comment 16 Paul Poulain 2012-07-06 13:38:08 UTC
QA comment: This patch removes more code than it adds, ++

I can't test it on a true SIP server, but looking at the code, I agree with Ian, so I set passed QA
Comment 17 Jared Camins-Esakov 2012-12-31 01:18:24 UTC
There have been no further reports of problems so I am marking this bug resolved.