Description
Sébastien Marie
2011-07-25 09:59:20 UTC
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. |