Bug 14408 - Path traversal vulnerabilty
Summary: Path traversal vulnerabilty
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: PC Linux
: P1 - high critical (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 14439 14440 14450 14467
  Show dependency treegraph
 
Reported: 2015-06-18 10:19 UTC by Raschin Tavakoli
Modified: 2016-12-05 21:23 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
screenshots (210.63 KB, application/x-compressed-tar)
2015-06-18 10:19 UTC, Raschin Tavakoli
Details
Bug 14408 Path traversal vulnerability (2.16 KB, patch)
2015-06-18 21:11 UTC, Chris Cormack
Details | Diff | Splinter Review
[ COUTER-PATCH - CHANGE DONE IN C4::AUTH ] Bug 14408 Path traversal vulnerability (1.56 KB, patch)
2015-06-19 08:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14408: Add tests to get_template_and_user (1.68 KB, patch)
2015-06-19 08:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED OFF] Bug 14408 Path traversal vulnerability (1.63 KB, patch)
2015-06-19 13:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 14408: Add tests to get_template_and_user (1.75 KB, patch)
2015-06-19 13:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14408 Path traversal vulnerability (1.70 KB, patch)
2015-06-19 14:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 14408: Add tests to get_template_and_user (1.81 KB, patch)
2015-06-19 14:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14408 Path Traversal error (1.88 KB, patch)
2015-06-22 05:51 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 14408 Path Traversal error (1.94 KB, patch)
2015-06-22 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14408: Add tests to get_template_and_user (1.77 KB, patch)
2015-06-22 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14408: Allow integers in template paths (1.95 KB, patch)
2015-06-22 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 14408 Path Traversal error (1.99 KB, patch)
2015-06-22 20:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14408: Add tests to get_template_and_user (1.84 KB, patch)
2015-06-22 20:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14408: Allow integers in template paths (1.97 KB, patch)
2015-06-22 20:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
bug 14408 - itemsearch no longer working (2.37 KB, patch)
2015-06-24 09:44 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 14408 & 14439 - typo fixes for regexes (860 bytes, patch)
2015-06-25 01:02 UTC, Liz Rea
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Raschin Tavakoli 2015-06-18 10:19:30 UTC
Created attachment 40271 [details]
screenshots

A Path Traversal vulnerability exists in the Koha Staff Client. The bug could be reproduced with the current Koha Release 3.20. 

The following pages are affected:

/cgi-bin/koha/svc/virtualshelves/search 
/cgi-bin/koha/svc/members/search

Poor input cleaning allows remote attackers to inject into the template_path variable and read arbitrary files from the file system via .. (dot dot).

Examples:

http://testbox:9002/cgi-bin/koha/svc/virtualshelves/search?template_path=..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd

http://testbox:9002/cgi-bin/koha/svc/members/search?template_path=..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd
Comment 1 Chris Cormack 2015-06-18 21:11:02 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-06-19 08:04:20 UTC
Chris,
It is not enough, test with:

/cgi-bin/koha/svc/members/search?template_path=test%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd

Don't you think we should sanitize in get_template_and_user?
Comment 3 Jonathan Druart 2015-06-19 08:14:58 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-06-19 08:26:29 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2015-06-19 13:38:44 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2015-06-19 13:39:19 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2015-06-19 14:45:06 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2015-06-19 14:45:13 UTC Comment hidden (obsolete)
Comment 9 Liz Rea 2015-06-20 05:22:19 UTC
main patch applies fine on 3.18, but the tests don't. I resolved the conflicts (number of tests, easy one), but the tests don't pass. I'm leaving that off for now until such time as I have a bit more time to look into it.

The problem, however, is resolved, regardless of the test result.
Comment 10 Robin Sheat 2015-06-21 22:59:13 UTC
It is not ideal to sanitise input like this. Instead, we should only accept known good values.

See: https://www.owasp.org/index.php/File_System#Path_traversal
Comment 11 Chris Cormack 2015-06-21 23:05:45 UTC
True, I attempted to do that in my original patch, but was too lenient I think.

+ $template_path =~ s/[^a-zA-Z_-]*//;

The current patch leaves us open to a null byte attack (which we had in the language cookie, which we solved by accepting only valid chars)
Comment 12 Chris Cormack 2015-06-21 23:22:58 UTC
Turns out, the problem with my patch was I was missing the g on the end of the regex.

But yes, we do need a patch that only accepts valid chars (which might be different depending on the use case, for instance in this case / is a valid char)

This evening after work I'll resurrect my patch and fix the regexp. Unless someone beats me.
Comment 13 Chris Cormack 2015-06-22 05:51:35 UTC Comment hidden (obsolete)
Comment 14 Chris Cormack 2015-06-22 05:54:10 UTC
Please test thoroughly, I did the patch in a hurry, and im happy to see a better whitelisting approach if someone has one.
Comment 15 Jonathan Druart 2015-06-22 08:25:42 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2015-06-22 08:25:48 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2015-06-22 08:25:53 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2015-06-22 20:32:10 UTC
Created attachment 40491 [details] [review]
[PASSED QA] Bug 14408 Path Traversal error

Counter counter patch
Please test well, including with the null byte %00, this uses a whitelisting to only allow files ending with .tt
and not allowing ../etc

Note the previous patch tries to protect against /etc/passwd
but //etc/passwd is now vulnerable.  I do think a whitelist is safer than trying to do a blacklist

/cgi-bin/koha/svc/virtualshelves/search
/cgi-bin/koha/svc/members/search

Are vulnerable

To test:
1/ Hit /cgi-bin/koha/svc/members/search?template_path=members/tables/members_results.tt
  Notice you get a valid JSON response
2/ Hit
/search?template_path=..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd
  (You may have add more ..%2f or remove them to get the correct path)
  Notice you can see the contents of the /etc/passwd file
3/ Hit
/cgi-bin/koha/svc/members/search?template_path=test%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd
4/ Apply patch
5/ Hit the first url again, notice it still works
6/ Hit the second url notice it now errors with a file not found
7/ Hit the third url notice it now errors with a file not found

Repeat for the other script also

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Katrin Fischer 2015-06-22 20:32:14 UTC
Created attachment 40492 [details] [review]
[PASSED QA] Bug 14408: Add tests to get_template_and_user

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Katrin Fischer 2015-06-22 20:32:17 UTC
Created attachment 40493 [details] [review]
[PASSED QA] Bug 14408: Allow integers in template paths

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Tomás Cohen Arazi 2015-06-22 20:50:31 UTC
Patches pushed to master.

Thanks Chris and Jonathan!
Comment 22 Chris Cormack 2015-06-22 22:19:00 UTC
Pushed to 3.20.x will be in 3.20.1
Comment 23 Liz Rea 2015-06-22 23:39:19 UTC
Hi, 

I'm having trouble with the tests patch for 3.18.x, I've left it off for now but have tested the patch itself and it seems to correct the issue.

Liz
Comment 24 Liz Rea 2015-06-23 01:55:18 UTC
Pushed to 3.18.x will be in 3.18.08
Comment 25 Fridolin SOMERS 2015-06-23 11:40:12 UTC
There is a problem with some pages calling get_template_and_user with empty string in template_name :

acqui/updatesupplier.pl
opac/opac-ratings.pl
tools/quotes/quotes_ajax.pl
tools/quotes/quotes-upload_ajax.pl

Should we accept empty string or correct those pages to use has_permission() ?
Comment 26 Fridolin SOMERS 2015-06-23 11:48:51 UTC
also, minor error : 
dot must be espaced in regexp and I dont understand why there is a "?" at the end :
^[$safe_chars]+\.tt$

Do I create a new Bug ?
Comment 27 Jonathan Druart 2015-06-23 11:54:55 UTC
(In reply to Fridolin SOMERS from comment #26)
> also, minor error : 
> dot must be espaced in regexp and I dont understand why there is a "?" at
> the end :
> ^[$safe_chars]+\.tt$
> 
> Do I create a new Bug ?

Chris, I would say this changes could do the trick:

-    die "bad template path" unless $in->{'template_name'} =~ m/^[$safe_chars]+.tt?$/ig; #sanitize input
+    die "bad template path" unless $in->{'template_name'} =~ m/^[$safe_chars]+\.tt$/ig; #sanitize input
Comment 28 Fridolin SOMERS 2015-06-23 12:21:59 UTC
Pushed to 3.14.x, will be in 3.14.16
Comment 29 Jonathan Druart 2015-06-23 12:57:50 UTC
(In reply to Fridolin SOMERS from comment #25)
> There is a problem with some pages calling get_template_and_user with empty
> string in template_name :
> 
> acqui/updatesupplier.pl
> opac/opac-ratings.pl
> tools/quotes/quotes_ajax.pl
> tools/quotes/quotes-upload_ajax.pl
> 
> Should we accept empty string or correct those pages to use has_permission()
> ?

Yes, I think so. Did you open a new bug report?
Comment 30 Jacek Ablewicz 2015-06-23 13:57:05 UTC
(In reply to Liz Rea from comment #24)
> Pushed to 3.18.x will be in 3.18.08

Correction from the last patch in this set (Bug 14408: Allow integers in template paths) seems to be missing in 3.18.8 / 3.18.x branch (not 100% sure if it is or isn't an issue in 3.18, but it may be so, as there are plenty of *.tt files in 3.18.x with numbers in their names: addorderiso2709.tt, z3950_*.tt, marc21_field_*.tt and so on).
Comment 31 Mason James 2015-06-23 14:04:42 UTC
Pushed to 3.16.x, will be in 3.16.12
Comment 32 Fridolin SOMERS 2015-06-23 14:07:43 UTC
I've created Bug 14439 and Bug 14440
Comment 33 Fridolin SOMERS 2015-06-23 15:53:19 UTC
(In reply to Mason James from comment #31)
> Pushed to 3.16.x, will be in 3.16.12

Ok, the changes are different from 3.18.x and upper.
I've reverted my patches in 3.14 to copy those from 3.16.x
Comment 34 Liz Rea 2015-06-23 22:23:27 UTC
Hi, 

I've created a patch for 3.18 that contains that last fix, but doesn't use the tests.

It's pushed and rolled up to the download site.

Liz
Comment 35 Jacek Ablewicz 2015-06-24 09:08:38 UTC
Another side effect of this patches is that item search gets broken (3.18.x and up seems to be affected), because we have at least two templates like this:

   - catalogue/itemsearch.json.tt
   - catalogue/itemsearch.csv.tt
Comment 36 Liz Rea 2015-06-24 09:14:52 UTC
Hi Jacob, 

Please report this as a separate bug, you can mark it as a blocker for this one.

Cheers,

Liz
Comment 37 Liz Rea 2015-06-24 09:15:33 UTC
Jacek... I'm so sorry my contacts are getting sticky.

Liz
Comment 38 Liz Rea 2015-06-24 09:44:42 UTC Comment hidden (obsolete)
Comment 39 Liz Rea 2015-06-25 01:02:07 UTC
Created attachment 40610 [details] [review]
Bug 14408 & 14439 - typo fixes for regexes

As applied to 3.18.8 - still need tests for these but the meat is there.
Comment 40 Mason James 2015-07-25 05:41:08 UTC
Pushed to 3.16.x, will be in 3.16.13
Comment 41 Mason James 2015-07-25 05:43:16 UTC
(In reply to Mason James from comment #40)
> Pushed to 3.16.x, will be in 3.16.13

oops, was actually included in 3.16.12 release