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 '/').
Created attachment 6413 [details] [review] Bug 6628 : Stopping a potential vulnerability
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
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.
Created attachment 6417 [details] [review] Bug 6628 : Stopping a potential vulnerability Here a proposed replacement: - using regex including 0-9 - remove trailing whitespace
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)
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
(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
oh, and more important: the patch successfully stop the vulnerability !
Youre Right Frère Sébastien-Marie !!! So, chris_n, it's a new security issue that must be fixed as well !
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.
Created attachment 6443 [details] [review] [3.2.x] Bug 6628 fixing security vulnerability
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)
This fix was included in the 3.6.x branch prior to 3.6.4.