Bug 25903 - Sending a SIP patron information request with a summary field flag in indexes 6-9 will crash server
Summary: Sending a SIP patron information request with a summary field flag in indexes...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-30 18:36 UTC by Kyle M Hall
Modified: 2021-06-14 21:31 UTC (History)
6 users (show)

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


Attachments
Bug 25903: Sending a SIP patron information request with a summary field flag in indexes 6-9 will crash server (2.51 KB, patch)
2020-06-30 18:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25903: Sending a SIP patron information request with a summary field flag in indexes 6-9 will crash server (2.61 KB, patch)
2020-07-01 12:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25903: Sending a SIP patron information request with a summary field flag in indexes 6-9 will crash server (2.66 KB, patch)
2020-08-19 13:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25903: Add unit tests (2.63 KB, patch)
2020-08-20 11:56 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2020-06-30 18:36:59 UTC
The 'summary' field in the patron information request specifies if detail information should be send for holds, overdues, fines, etc. The field is 10 characters in length (0-9). However, the SIP2 spec only defines indexes 0 though 5, leave 6 though 9 undefined. Some ILSs specify behavior for these undefined indexes. Apparently the 7th field is often used to request 'Fees', as opposed to 'Fines' in some ILS. Some software that integrate via SIP try both the 5th and 7th indexes to ensure they get all fines and fees.

The problem is that Koha's SIP server crashes if any 'summary' index beyond 5 is flagged. We should simply ignore flags beyond 5 and act as if no flags were sent.
Comment 1 Kyle M Hall 2020-06-30 18:41:39 UTC
Created attachment 106443 [details] [review]
Bug 25903: Sending a SIP patron information request with a summary field flag in indexes 6-9 will crash server

The 'summary' field in the patron information request specifies if detail information should be send for holds, overdues, fines, etc. The field is 10 characters in length (0-9). However, the SIP2 spec only defines indexes 0 though 5, leave 6 though 9 undefined. Some ILSs specify behavior for these undefined indexes. Apparently the 7th field is often used to request 'Fees', as opposed to 'Fines' in some ILS. Some software that integrate via SIP try both the 5th and 7th indexes to ensure they get all fines and fees.

The problem is that Koha's SIP server crashes if any 'summary' index beyond 5 is flagged. We should simply ignore flags beyond 5 and act as if no flags were sent.

Test Plan:
1) Enable SIP for your instance
2) Send a patron information request with a summary flag in any index beyond 5.
   i.e.: 6300120200617    124846      Y   AOMIDAY|AA21030050054321
3) Note the SIP server just closes the connection without a response
4) Apply this patch
5) Restart the SIP server
6) Send the same request
7) Note you get back the patron information response!
Comment 2 Kyle M Hall 2020-07-01 12:01:12 UTC
Created attachment 106452 [details] [review]
Bug 25903: Sending a SIP patron information request with a summary field flag in indexes 6-9 will crash server

The 'summary' field in the patron information request specifies if detail information should be send for holds, overdues, fines, etc. The field is 10 characters in length (0-9). However, the SIP2 spec only defines indexes 0 though 5, leave 6 though 9 undefined. Some ILSs specify behavior for these undefined indexes. Apparently the 7th field is often used to request 'Fees', as opposed to 'Fines' in some ILS. Some software that integrate via SIP try both the 5th and 7th indexes to ensure they get all fines and fees.

The problem is that Koha's SIP server crashes if any 'summary' index beyond 5 is flagged. We should simply ignore flags beyond 5 and act as if no flags were sent.

Test Plan:
1) Enable SIP for your instance
2) Send a patron information request with a summary flag in any index beyond 5.
   i.e.: 6300120200617    124846      Y   AOMIDAY|AA21030050054321
3) Note the SIP server just closes the connection without a response
4) Apply this patch
5) Restart the SIP server
6) Send the same request
7) Note you get back the patron information response!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jeff Gaines <jgaine@arlingtonva.us>
Comment 3 Katrin Fischer 2020-08-16 11:22:57 UTC
Hi Kyle and others,

patch applies, QA script passes, but I have some trouble setting up SIP on kohadevbox as it doesn't appear to work automatically for me anymore (error on reset_all?).

Can you help me with the steps for "1) Enable SIP for your instance"?
Comment 4 Kyle M Hall 2020-08-18 12:02:33 UTC
It's working for me in koha-testing-docker.

root@e9e637b7f066:koha(bug25903-qa)$ ./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su koha -sp koha -l LIB -m patron_information --patron 42
Attempting socket connection to 127.0.0.1:6001...connected!
SEND: 9300CNkoha|COkoha|CPLIB|
READ: 941

Trying 'patron_information'
SEND: 6300120200818    120149          AOLIB|AA42|ACkoha|
READ: 64              00120200818    120151000000000000000000000000AOkohalibrary|AA42|AE koha|BLY|CC5|PCS|PIY|AFGreetings from Koha. |


Can you verify your SIP server is running?


(In reply to Katrin Fischer from comment #3)
> Hi Kyle and others,
> 
> patch applies, QA script passes, but I have some trouble setting up SIP on
> kohadevbox as it doesn't appear to work automatically for me anymore (error
> on reset_all?).
> 
> Can you help me with the steps for "1) Enable SIP for your instance"?
Comment 5 Martin Renvoize 2020-08-19 13:28:06 UTC
Created attachment 108632 [details] [review]
Bug 25903: Sending a SIP patron information request with a summary field flag in indexes 6-9 will crash server

The 'summary' field in the patron information request specifies if detail information should be send for holds,
overdues, fines, etc. The field is 10 characters in length (0-9). However, the SIP2 spec only defines indexes 0
though 5, leave 6 though 9 undefined. Some ILSs specify behavior for these undefined indexes. Apparently the
7th field is often used to request 'Fees', as opposed to 'Fines' in some ILS. Some software that integrate via
SIP try both the 5th and 7th indexes to ensure they get all fines and fees.

The problem is that Koha's SIP server crashes if any 'summary' index beyond 5 is flagged. We should simply
ignore flags beyond 5 and act as if no flags were sent.

Test Plan:
1) Enable SIP for your instance
2) Send a patron information request with a summary flag in any index beyond 5.
   i.e.: 6300120200617    124846      Y   AOMIDAY|AA21030050054321
3) Note the SIP server just closes the connection without a response
4) Apply this patch
5) Restart the SIP server
6) Send the same request
7) Note you get back the patron information response!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jeff Gaines <jgaine@arlingtonva.us>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2020-08-19 13:29:19 UTC
Patch works as described.. I feel like in the future perhaps we should add support for these 'special' value that fall outside the standard range.. but preventing an explosion seems to be the correct initial behaviour here.

Passing QA.
Comment 7 Jonathan Druart 2020-08-20 10:25:10 UTC
Can we have tests for this change please?
Comment 8 Kyle M Hall 2020-08-20 11:56:43 UTC
Created attachment 108734 [details] [review]
Bug 25903: Add unit tests
Comment 9 Jonathan Druart 2020-08-24 10:33:20 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 10 Lucas Gass 2020-09-04 18:04:51 UTC
backported to 20.05.x for 20.05.04
Comment 11 Aleisha Amohia 2020-09-10 04:08:10 UTC
backported to 19.11.x for 19.11.10
Comment 12 Victor Grousset/tuxayo 2020-09-20 18:17:43 UTC
Not backported to oldoldstable (19.05.x). Feel free to ask if it's needed.