Bug 7787

Summary: SIP server closes the TCP connection when it receives an invalid message
Product: Koha Reporter: Marc Balmer <marc>
Component: SIP2Assignee: Marc Balmer <marc>
Status: CLOSED FIXED QA Contact: Ian Walls <koha.sekjal>
Severity: normal    
Priority: P5 - low CC: dpavlin, paul.poulain, veron
Version: 3.8   
Hardware: All   
OS: All   
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.
Make the SIP server more robust by not exiting on invalid input, but rather issue a resend response.
RAW login can be as robust as telnet login
Combined diff for telnet and raw connections
Reformatted Patch
Signed off patch
[SIGNED OFF] Bug 7787: Make the SIP server much more robust.

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.