Bug 6629 - [security] insecure use of Cookie for language selection
Summary: [security] insecure use of Cookie for language selection
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: Galen Charlton
QA Contact: Bugs List
URL:
Keywords:
: 7265 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-07-25 09:59 UTC by Sébastien Marie
Modified: 2019-06-27 09:24 UTC (History)
7 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
quick patch (820 bytes, patch)
2011-11-25 05:51 UTC, Sébastien Marie
Details | Diff | Splinter Review
Bug 6629 : Sanitizing input from language cookie (1.27 KB, patch)
2011-11-25 06:15 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6629 : Sanitizing input from language cookie (1.27 KB, patch)
2011-11-25 06:33 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6629 : Sanitizing input from language cookie (1.27 KB, patch)
2011-11-25 06:38 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6629 : Sanitizing input from language cookie (1.27 KB, patch)
2011-11-25 06:42 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6629 : Sanitizing input from language cookie (1.40 KB, patch)
2011-11-25 07:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
[3.4.x] Bug 6629 fix for vulnerability (754 bytes, patch)
2011-11-25 07:39 UTC, Chris Cormack
Details | Diff | Splinter Review
[3.2.x] Bug 6629 vulnerability fix (749 bytes, patch)
2011-11-25 07:44 UTC, Chris Cormack
Details | Diff | Splinter Review
[3.4.x] Bug 6629 fix for vulnerability (1.07 KB, patch)
2011-11-25 07:50 UTC, Chris Cormack
Details | Diff | Splinter Review
[3.4.x] Bug 6629 fix for vulnerability (1.07 KB, patch)
2011-11-25 18:18 UTC, Chris Cormack
Details | Diff | Splinter Review
[3.2.x] Bug 6629 vulnerability fix (750 bytes, patch)
2011-11-25 18:20 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6629 : Follow up to trap vuln in webinstaller and fixing the error (1.93 KB, patch)
2011-11-27 08:19 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6629 : Follow up to trap vuln in webinstaller and fixing the error (2.03 KB, patch)
2011-11-27 08:27 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6629 : Follow up to trap vuln in webinstaller and fixing the error (2.05 KB, patch)
2011-11-27 08:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 6629 : Follow up, sanitising in a couple more places (1.33 KB, patch)
2011-11-27 08:59 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6629 : Follow up, sanitising in a couple more places (1.33 KB, patch)
2011-11-27 09:01 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6629 : Follow up, sanitising in a couple more places (1.44 KB, patch)
2011-11-27 15:06 UTC, Jared Camins-Esakov
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:59:20 UTC
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'
Comment 1 Sébastien Marie 2011-11-25 05:34:31 UTC
An exploit have been published here: http://1337day.com/exploits/17246

It seems it exploit this vulnerability: so I increase the priority.
Comment 2 Sébastien Marie 2011-11-25 05:35:18 UTC
*** Bug 7265 has been marked as a duplicate of this bug. ***
Comment 3 Sébastien Marie 2011-11-25 05:51:57 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2011-11-25 06:15:59 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2011-11-25 06:33:54 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2011-11-25 06:38:49 UTC Comment hidden (obsolete)
Comment 7 Chris Cormack 2011-11-25 06:42:00 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2011-11-25 07:10:10 UTC Comment hidden (obsolete)
Comment 9 Sébastien Marie 2011-11-25 07:16:06 UTC
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_-]*//
Comment 10 Sébastien Marie 2011-11-25 07:36:52 UTC
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.
Comment 11 Chris Cormack 2011-11-25 07:39:20 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2011-11-25 07:41:51 UTC
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
Comment 13 Chris Cormack 2011-11-25 07:44:12 UTC Comment hidden (obsolete)
Comment 14 Chris Cormack 2011-11-25 07:50:12 UTC Comment hidden (obsolete)
Comment 15 Chris Cormack 2011-11-25 08:42:18 UTC
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
Comment 16 Paul Poulain 2011-11-25 10:05:47 UTC
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
Comment 17 Fridolin SOMERS 2011-11-25 12:10:46 UTC
(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.
Comment 18 Chris Cormack 2011-11-25 18:16:25 UTC
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.
Comment 19 Chris Cormack 2011-11-25 18:18:47 UTC
Created attachment 6411 [details] [review]
[3.4.x] Bug 6629 fix for vulnerability
Comment 20 Chris Cormack 2011-11-25 18:20:43 UTC
Created attachment 6412 [details] [review]
[3.2.x] Bug 6629 vulnerability fix
Comment 21 Chris Cormack 2011-11-27 08:19:43 UTC Comment hidden (obsolete)
Comment 22 Sébastien Marie 2011-11-27 08:26:35 UTC
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
Comment 23 Katrin Fischer 2011-11-27 08:27:05 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2011-11-27 08:28:25 UTC
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.
Comment 25 Chris Cormack 2011-11-27 08:31:30 UTC
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.
Comment 26 Sébastien Marie 2011-11-27 08:47:10 UTC
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).
Comment 27 Chris Cormack 2011-11-27 08:55:16 UTC
Havent been able to exploit it, but doesn't hurt to sanitise it anyway.
Comment 28 Chris Cormack 2011-11-27 08:59:02 UTC Comment hidden (obsolete)
Comment 29 Chris Cormack 2011-11-27 09:01:26 UTC Comment hidden (obsolete)
Comment 30 Jared Camins-Esakov 2011-11-27 15:06:44 UTC
Created attachment 6432 [details] [review]
Bug 6629 : Follow up, sanitising in a couple more places

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 31 Jared Camins-Esakov 2011-11-27 15:08:32 UTC
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.
Comment 32 Paul Poulain 2011-11-28 08:47:17 UTC
The two follow-up have been pushed on master (attachments 6624 and 6432)

Please test again
Comment 33 Chris Nighswonger 2011-12-04 02:39:08 UTC
Patches also pushed to 3.2.x and 3.4.x and 3.6.x.