Bug 6628 - [security] help system use insecure REFERRER for file inclusion
Summary: [security] help system use insecure REFERRER for file inclusion
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.6
Hardware: All All
: PATCH-Sent (DO NOT USE) critical (vote)
Assignee: Chris Cormack
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-25 09:47 UTC by Sébastien Marie
Modified: 2019-06-27 09:24 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 6628 : Stopping a potential vulnerability (761 bytes, patch)
2011-11-25 18:42 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6628 : Stopping a potential vulnerability (921 bytes, patch)
2011-11-26 09:32 UTC, Sébastien Marie
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6628 : Stopping a potential vulnerability (1.28 KB, patch)
2011-11-28 07:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
[3.2.x] Bug 6628 fixing security vulnerability (656 bytes, patch)
2011-11-28 10:15 UTC, Chris Cormack
Details | Diff | Splinter Review
[3.4.x] Bug 6628 fixing security vulnerability (1.50 KB, patch)
2011-11-28 10:32 UTC, Sébastien Marie
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Sébastien Marie 2011-07-25 09:47:21 UTC
The 'help.pl' use REFERRER HTTP-Header for determine which template should be displayed to user.

This HTTP-Header could be forged by user.

CWE-73: External Control of File Name or Path (see http://cwe.mitre.org/data/definitions/73.html)


The template name is extracted from REFERRER which regex:

> our $refer = $query->referer();
> $refer =~ /koha\/(.*)\.pl/;
> my $from = "modules/help/$1.tt";

So it is possible to have a referer like: http://xxx/koha/EVIL-THING.plxxx
and $from should be egal to "modules/help/EVIL-THING.tt"


Some suggestions:
 - It should be better to have a better regex for extract the help template: like only allow a-zA-Z, '-' and '/' (not including '.' [for ".."], but including '/' for help templates like "members/boraccount.tt")

 - It will be better to rename all help templates from: "members/boraccount.tt" to "members-boraccount.tt", in order to remove need of '/' in regex. And all '/' in value taken from referer must be remplaced by '-'. ('-' in path is less worst than '/').
Comment 1 Chris Cormack 2011-11-25 18:42:16 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2011-11-25 18:46:47 UTC
Please test

You can test by fudging the referrer, or by passing a url=bogus value

http://localhost:8080/cgi-bin/koha/help.pl?url=koha/../../../../../../etc/passwd%00.pl
Comment 3 Sébastien Marie 2011-11-26 09:12:09 UTC
The regexp isn't correct, as it disallow numbers in filename, like these:
  acqui/z3950_search
  admin/z3950servers
  admin/koha2marclinks

So we should use: 
s/[^a-zA-Z0-9_\-\/]//g

Else, works well.
Thanks.
Comment 4 Sébastien Marie 2011-11-26 09:32:38 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2011-11-28 07:40:49 UTC
Created attachment 6440 [details] [review]
[SIGNED-OFF] Bug 6628 : Stopping a potential vulnerability

Signed-off-by: Frère Sébastien Marie <semarie-koha@latrappe.fr>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
- verified help pages still work
- verified /cgi-bin/koha/help.pl?url=koha/../catalogue/advsearch.pl does not
show the template file (did work on master, not after applying patch)
- verified cgi-bin/koha/help.pl?url=koha/../../../../../../etc/passwd%00.pl does not work (didn't work on master or after applying patch)
Comment 6 Paul Poulain 2011-11-28 09:09:39 UTC
This vulnerability would allow anyone reading any .tt file on the server. As /etc/password is not ending by .tt, this problem is much less critical than the 6629 one !
That's why, Katrin, you didn't see any difference before and after applying the patch.

The fix is usefull though, as it's a vulnerability, so it's pushed.

Patch pushed, please test
Comment 7 Sébastien Marie 2011-11-28 09:40:47 UTC
(In reply to comment #6)
> This vulnerability would allow anyone reading any .tt file on the server. As
> /etc/password is not ending by .tt, this problem is much less critical than the
> 6629 one !

Paul, here I disagree with you :-)

Under 3.4.x (sorry, no master deployed for test), I could successfully exploit this vulnerability to echo /etc/passwd. The ".tt" at the end is normally discarded by %00 (the meaning is the same that \0 in C-string, it is stand for end-of-string).

Katrin, you could try to add more ../ to url (here, we traversal should go back from "$htdocs/$theme/$lang/modules/help/", and depending where is located $htdocs, there are a couple of parent before the root's filesystem).

My test against 3.4.x: /cgi-bin/koha/help.pl?url=koha/../../../../../../../../../../../etc/passwd%00.pl
Comment 8 Sébastien Marie 2011-11-28 09:48:00 UTC
oh, and more important: the patch successfully stop the vulnerability !
Comment 9 Paul Poulain 2011-11-28 09:51:00 UTC
Youre Right Frère Sébastien-Marie !!!

So, chris_n, it's a new security issue that must be fixed as well !
Comment 10 Chris Cormack 2011-11-28 10:00:11 UTC
The patch will apply for 3.4.x as well.

Please cherry pick, I will do one for 3.2.x for people who want to patch their systems.
Comment 11 Chris Cormack 2011-11-28 10:15:20 UTC
Created attachment 6443 [details] [review]
[3.2.x] Bug 6628 fixing security vulnerability
Comment 12 Sébastien Marie 2011-11-28 10:32:32 UTC
Created attachment 6444 [details] [review]
[3.4.x] Bug 6628 fixing security vulnerability

The patch correct also two invalid calls to themelanguage: tests are not possible else (Undefined subroutine &main::themelanguage)
Comment 13 Jared Camins-Esakov 2012-05-23 12:51:42 UTC
This fix was included in the 3.6.x branch prior to 3.6.4.