CWE-73: External Control of File Name or Path (see http://cwe.mitre.org/data/definitions/73.html) In C4/Templates.pm, in the function themelanguage, the user language is obtain from a cookie 'KohaOpacLanguage', and use 'as-it' in the path's construction for the template filename. In the next piece of code, $la is 'KohaOpacLanguage' cookie value: > # searches through the themes and languages. First template it find it returns. > # Priority is for getting the theme right. > THEME: > foreach my $th (@themes) { > foreach my $la (@languages) { > if ( -e "$htdocs/$th/$la/modules/$tmpl" ) { > $theme = $th; > $lang = $la; > last THEME; > } > last unless $la =~ /[-_]/; > } > } In opac/opac-main.pl, same: if cookie 'KohaOpacLanguage' exists, it used. Here, the page also used HTTP_ACCEPT_LANGUAGE sanitized with regex. In koha/installer/install.pl and koha/installer/InstallAuth.pm the cookie is also used (but need verification if it used in manner that permit exploit). As the cookie could be forged (user input), and contains any characters, it could embed '../' for path-traversable. The exploitation of this problem is mitigated by the fact that the perl function '-e' seems to be resultant to '\0' inclusion (in order to strip all strings after the variable). Suggestions: - A regex sanitization should be used, or, should used C4::Output::getlanguagecookie function, which take only the first two characters (I will prefer a regex like /^[a-zA-Z]*$/ ) - An unified method should be used: a function somewhere (C4:Templates ?) that return a list of possible languages: - first element: cookie value sanitized (if exist) - next sanitized list of ENV{HTTP_ACCEPT_LANGUAGE} - next 'en'
An exploit have been published here: http://1337day.com/exploits/17246 It seems it exploit this vulnerability: so I increase the priority.
*** Bug 7265 has been marked as a duplicate of this bug. ***
Created attachment 6398 [details] [review] quick patch it is quick patch (not git, sorry: no time now)
Created attachment 6399 [details] [review] Bug 6629 : Sanitizing input from language cookie I dont think we can use only 2 digits, some languages is much longer zh-hans-TW for example But the regex should stop it bening able handle nasty chars, whitelisting safe ones instead
Created attachment 6400 [details] [review] Bug 6629 : Sanitizing input from language cookie I dont think we can use only 2 digits, some languages is much longer zh-hans-TW for example But the regex should stop it bening able handle nasty chars, whitelisting safe ones instead
Created attachment 6401 [details] [review] Bug 6629 : Sanitizing input from language cookie I dont think we can use only 2 digits, some languages is much longer zh-hans-TW for example But the regex should stop it bening able handle nasty chars, whitelisting safe ones instead
Created attachment 6402 [details] [review] Bug 6629 : Sanitizing input from language cookie I dont think we can use only 2 digits, some languages is much longer zh-hans-TW for example But the regex should stop it bening able handle nasty chars, whitelisting safe ones instead
Created attachment 6403 [details] [review] Bug 6629 : Sanitizing input from language cookie I dont think we can use only 2 digits, some languages is much longer zh-hans-TW for example But the regex should stop it bening able handle nasty chars, whitelisting safe ones instead Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> I checked the patch doesn't break language switching and language selection.
about the patch (6403): - I think we should also remove (or sanitize) $ENV{ HTTP_ACCEPT_LANGUAGE }, as is it a user controlled string also. - the regexp is not accurate (I think): use s/[^a-zA-Z_-]*//g instead of s/[^a-zA-Z_-]*//
In order to check the patch against the vulnerability, here a little poc using curl (a shell tool): > curl -v -b 'KohaOpacLanguage=../../../../../../../../etc/passwd%00' 'http://myopac.example.com/cgi-bin/koha/opac-main.pl' A vulnerable result show the /etc/passwd file A non vulnerable result show 'opac-main' in the default Language (en). The test suppose a linux/BSD server.
Created attachment 6404 [details] [review] [3.4.x] Bug 6629 fix for vulnerability
Yes the patch for master does sanitize HTTP_ACCEPT_LANGUAGE because the sanitize is done in getlanguagecookie, ill update the 3.4.x to do that also
Created attachment 6405 [details] [review] [3.2.x] Bug 6629 vulnerability fix
Created attachment 6406 [details] [review] [3.4.x] Bug 6629 fix for vulnerability
It turns out, master is not vulnerable, nor is the 3.6.x branch, but the patch should be applied anyway. Its a more elegant solution to checking the cookie. But 3.2.x and 3.4.x are vulnerable
QA comment: nothing to say. Critical security issue, dealing with it without any delay Patch pushed, please test & apply on 3.2/3.4 as well
(In reply to comment #9) > - the regexp is not accurate (I think): > use s/[^a-zA-Z_-]*//g > instead of s/[^a-zA-Z_-]*// I agree : $lang =~ s/[^a-zA-Z_-]*//; It replaces only first occurence of pattern. So it will clean heading special characters but not within or after. Global matching : $lang =~ s/[^a-zA-Z_-]//g; There every special character is removed. Thanks for your work. Best regards.
The patch as its stands prevents file traversal, you could only read a file in the current dir with it, but yes you are right g is better. It turns out master and 3.6 are not vulnerable anyway, as if the value of $lang does not match a valid lanuage, it is made undef. So I will do new patches for 3.2.x and 3.4.x So to be clear on 3.4.6 and lower are vulnerable. 3.6.0 and master are not.
Created attachment 6411 [details] [review] [3.4.x] Bug 6629 fix for vulnerability
Created attachment 6412 [details] [review] [3.2.x] Bug 6629 vulnerability fix
Created attachment 6422 [details] [review] Bug 6629 : Follow up to trap vuln in webinstaller and fixing the error
When I check versus git, the following patchs are applied: - master (not vulnerable) : patch applied - 3.6.x (not vulnerable) : patch not found - 3.4.x *vulnerable* : patch not found - 3.2.x *vulnerable* : patch not found Could you push patch for vulnerables versions too ? As an exploit was published, and the problem quickly corrected, it is not very good to have patch not pushed quickly too for vulnerable versions. Thanks
Created attachment 6423 [details] [review] Bug 6629 : Follow up to trap vuln in webinstaller and fixing the error Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Patch fixes problem occuring in web installer.
Created attachment 6424 [details] [review] [SIGNED-OFF] Bug 6629 : Follow up to trap vuln in webinstaller and fixing the error Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Patch fixes problem occuring in web installer.
Frère Sébastien Marie 3.2.x is end of life, so that patch won't be pushed. As soon as Chris Nighswonger is back I am sure he will push the 3.4.x one. And 3.4.7 will be released very soon including the fix.
In installer, there are another script that use cookie directly: installer/install.pl on line 268 and 231. On line 267-268: > my $langchoice = $query->param('fwklanguage'); > $langchoice = $query->cookie('KohaOpacLanguage') unless ($langchoice); (fwklanguage is also used without sanitization). On line 230-231, it is a duplicated code of previous, but I don't not if it is possible to exploit here (language not used for template inclusion).
Havent been able to exploit it, but doesn't hurt to sanitise it anyway.
Created attachment 6427 [details] [review] Bug 6629 : Follow up, sanitising in a couple more places
Created attachment 6428 [details] [review] Bug 6629 : Follow up, sanitising in a couple more places
Created attachment 6432 [details] [review] Bug 6629 : Follow up, sanitising in a couple more places Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
I can also confirm the patch fixing the error in the webinstaller works as advertised. I didn't manage to attach my sign off to the bug, but Katrin already did that so it doesn't matter.
The two follow-up have been pushed on master (attachments 6624 and 6432) Please test again
Patches also pushed to 3.2.x and 3.4.x and 3.6.x.