Bug 8171

Summary: Improper escaping of quotes during z39.50 queries leads to broken html
Product: Koha Reporter: William Hurley <bhurley>
Component: Z39.50 / SRU / OpenSearch ServersAssignee: Owen Leonard <oleonard>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: chris, paul.poulain
Version: 3.8   
Hardware: All   
OS: Linux   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Test of proposed patch
Bug 8171 - Improper escaping of quotes during z39.50 queries leads to broken html
Bug 8171 - Improper escaping of quotes during z39.50 queries leads to broken html
Bug 8171 - Improper escaping of quotes during z39.50 queries leads to broken html

Description William Hurley 2012-05-31 18:18:49 UTC
When using Koha v. 3.8 for z39.50 based copy cataloging, if the title being searched for contains quotes, then everything after the first set of quotation marks is not transferred to the z39.50 search.  For example the title The Irish "Americans" only shows up as The Irish in the z39.50 search pop up window.   My understanding is that improper escaping of quotes can cause sql injection attacks, although this would require the malicious coder to be in the staff area (intranet) to conduct such an attack.  It also leads to incorrect z39.50 search results, if one is unaware of the problem.  


I believe the problem is in the C4::Z3950 perl module.  The $term variable doesn't ignore the quotes which may legitimately be in the string.  I believe the error is on line 228 of the c4::Z3950 module, my version (3.8) of which reads 

		
228 $sth=$dbh->prepare("insert into z3950queue (term,type,servers, identifier) values (?, ?, ?, ?)");


			
I think it should read

		

228 $sth=$dbh->prepare("insert into z3950queue (q{term},type,servers, identifier) values (?, ?, ?, ?)");


If there is anything I can do to further this process, or if you believe my report is in error, please let me know.  Keep up the good work.
Comment 1 Chris Cormack 2012-05-31 23:15:28 UTC
Hi William

On line 228 term is the name of the column. It is not the variable.

The variable is in 
$sth->execute($query, $type, $serverlist, $requestid);

The query is escaped by use of placeholders ie the (?,?,?,?)

Which means $query is escaped and replaces the first ? in that list.

So that part is not the problem, I suspect the actual problem is that the "" are not escaped when doing the actual search.

Ill leave this open because it is a valid but, bug I don't think your solution will work (or addresses the problem :))

If you want to read up about placeholders please look here http://search.cpan.org/dist/DBI/DBI.pm#Placeholders_and_Bind_Values
Comment 2 William Hurley 2012-06-01 15:26:15 UTC
You may be correct that I have not identified the source of the bug.  You are incorrect when you say that the problem is that the quotation marks are not escaped during the actual search.  The problem is that the quotation marks are not escaped when one chooses edit-> replace record via z39.50.  If the title contains double quotes, everything from the first set of quotes back disappears, and is not transferred into the search window.  The quoted text never makes it into the actual search.  

It is easy to replicate this bug.  Pick any title in your collection and edit marc field $245a by appending "test quotes" at the end of the title.  Then click on z39.50 search.  The words "test quote" do not appear in the pop-up search window. Therefore, whatever module controls this behavior is not properly escaping the quotes.  If you would kindly identify this module I would be greatly appreciative.  Thanks for your quick response and all your help in this matter.  

(In reply to comment #1)
> Hi William
> 
> On line 228 term is the name of the column. It is not the variable.
> 
> The variable is in 
> $sth->execute($query, $type, $serverlist, $requestid);
> 
> The query is escaped by use of placeholders ie the (?,?,?,?)
> 
> Which means $query is escaped and replaces the first ? in that list.
> 
> So that part is not the problem, I suspect the actual problem is that the ""
> are not escaped when doing the actual search.
> 
> Ill leave this open because it is a valid but, bug I don't think your
> solution will work (or addresses the problem :))
> 
> If you want to read up about placeholders please look here
> http://search.cpan.org/dist/DBI/DBI.pm#Placeholders_and_Bind_Values
Comment 3 Chris Cormack 2012-06-01 20:19:06 UTC
(In reply to comment #2)
> You may be correct that I have not identified the source of the bug.  You
> are incorrect when you say that the problem is that the quotation marks are
> not escaped during the actual search.  The problem is that the quotation
> marks are not escaped when one chooses edit-> replace record via z39.50.  If
> the title contains double quotes, everything from the first set of quotes
> back disappears, and is not transferred into the search window.  The quoted
> text never makes it into the actual search.  

I was just guessing where the problem was, since I haven't tried to replicate.
But I can guarantee that changing the column name in the sql, will have utterly no effect on value that is stored in that column.

> 
> It is easy to replicate this bug.  Pick any title in your collection and
> edit marc field $245a by appending "test quotes" at the end of the title. 
> Then click on z39.50 search.  The words "test quote" do not appear in the
> pop-up search window. Therefore, whatever module controls this behavior is
> not properly escaping the quotes.  If you would kindly identify this module
> I would be greatly appreciative.  Thanks for your quick response and all
> your help in this matter.  
> 

I would look into the perl code that is outputting it and/or the template.

It is not being stored in the database incorrectly, the placeholders are escaping all bad characters.
Comment 4 Chris Cormack 2012-06-01 20:26:39 UTC
Check line 95 in koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/

<ol><li><label for="title">Title: </label><input type="text" id="title"  name="title" value="[% title %]" /></li>

My theory is if [% title %] starts with a " then you get value="" and there is the problem.

So you could try adding [% title |html %]
Comment 5 William Hurley 2012-06-01 20:36:16 UTC
Ok cool. I will try this patch on a fresh install of the latest stable branch on my development box and see if the problem resolves it's self.  Should be able to let you know by early next week, as I almost certainly won't finish that before the end of the work day here in NYC.  

(In reply to comment #4)
> Check line 95 in koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/
> 
> <ol><li><label for="title">Title: </label><input type="text" id="title" 
> name="title" value="[% title %]" /></li>
> 
> My theory is if [% title %] starts with a " then you get value="" and there
> is the problem.
> 
> So you could try adding [% title |html %]
Comment 6 Chris Cormack 2012-06-01 20:46:36 UTC
(In reply to comment #5)
> Ok cool. I will try this patch on a fresh install of the latest stable
> branch on my development box and see if the problem resolves it's self. 
> Should be able to let you know by early next week, as I almost certainly
> won't finish that before the end of the work day here in NYC.  

That's ok, its already 8.30am on Saturday here anyway, if it does work you could make an actual patch and attach it to here, and set the bug to needs signoff
Comment 7 William Hurley 2012-06-04 19:11:48 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2012-06-04 19:25:57 UTC Comment hidden (obsolete)
Comment 9 Paul Poulain 2012-06-05 11:26:12 UTC
Patch tested with a sandbox, by Paul Poulain demoing at KohaCon12 <paul.poulain@biblibre.com>
Comment 10 Paul Poulain 2012-06-05 11:26:31 UTC Comment hidden (obsolete)
Comment 11 Paul Poulain 2012-06-05 11:33:23 UTC
switching back to need signoff, it was just for the KohaCon12 demo
Comment 12 Jared Camins-Esakov 2012-06-10 12:42:01 UTC
Created attachment 10094 [details] [review]
Bug 8171 - Improper escaping of quotes during z39.50 queries leads to broken html

Implementing fix as suggested by Chris Cormack:

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8171#c4

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 13 Paul Poulain 2012-06-11 15:15:29 UTC
QA comment: tiny patch that fixes a problem using T::T features
Comment 14 Chris Cormack 2012-06-14 07:14:36 UTC
Pushed to 3.8.x, will be in 3.8.2