Bug 15809 - versions of CGI < 4.08 do not have multi_param
Summary: versions of CGI < 4.08 do not have multi_param
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 14076 16154 16259 16476 16645
  Show dependency treegraph
 
Reported: 2016-02-12 11:30 UTC by Jonathan Druart
Modified: 2017-12-07 22:16 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15809: Redefine multi_param is CGI < 4.08 is used (1.85 KB, patch)
2016-02-12 11:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15809: Redefine multi_param is CGI < 4.08 is used (1.99 KB, patch)
2016-02-12 11:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15809: Redefine multi_param is CGI < 4.08 is used (2.06 KB, patch)
2016-02-15 06:51 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15809: Redefine multi_param is CGI < 4.08 is used (2.37 KB, patch)
2016-03-18 08:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-02-12 11:30:51 UTC
On debian Jessie, the CGI version is >= 4.08
Since this version, the param method raise a warning "CGI::param called in list context".
Indeed, it can cause vulnerability if called in list context

https://metacpan.org/pod/CGI#Fetching-the-value-or-values-of-a-single-named-parameter
http://blog.gerv.net/2014/10/new-class-of-vulnerability-in-perl-web-applications/

There is a long journey to get rid of these warnings. First I suggest to redefine the multi_param method when the CGI version installed is < 4.08, it will allow us to move the wrong ->param calls to ->multi_param without waiting for everybody to upgrade.

The different ways to call these 2 methods are:

my $foo = $cgi->param('foo'); # OK

my @foo = $cgi->param('foo'); # NOK, will raise the warning
my @foo = $cgi->multi_param('foo'); #OK

$template->param( foo => $cgi->param('foo') ); # NOK, will raise the warning and vulnerable
$template->param( foo => scalar $cgi->param('foo') ); # OK
Comment 1 Jonathan Druart 2016-02-12 11:35:44 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-02-12 11:46:48 UTC Comment hidden (obsolete)
Comment 3 Mark Tompsett 2016-02-15 06:51:20 UTC Comment hidden (obsolete)
Comment 4 Mark Tompsett 2016-02-15 06:54:16 UTC
This works well in solving the sample case in bug 14076.
At some point in the future (2+ years), perhaps we'll up the required CGI version, such that multi_param will be included, and we can remove this code.
Comment 5 Marcel de Rooy 2016-02-17 12:57:34 UTC
I am having the impression that we do not completely tackle the problem (read vulnerability given) here.
Because just switching param to multi_param (without looking to the context) does not really solve it. You only suppress the warning.

We could still be vulnerable with calls like: 
my $hash = { a => multi_param('b'), c => 'd' )
If multi_param b returns ( b1, b2, b3), your hash is 'injected' with b2 => b3, just the same as param b would have done.

So we should check (before or after this patch) if we are calling params in a hash context. If so, prepend with scalar.

Redefining methods/routines for lower versions of a module is not the most elegant solution (from QA perspective). If we could prevent doing so, we should. 
Since we do not need to add calls to multi_param yet and we do not address the actual vulnerability in this patch, I would propose to not add this redefinition. We should concentrate on the calls to param in a hash context and scalarize them. (The warnings in the log show us where these calls are.) 

Failed QA
I will also ask for another (QA) opinion on the dev list.
Comment 6 Marcel de Rooy 2016-02-17 13:20:08 UTC
Still we have the other situation that we do want multiple values:
my @a = param( 'b' );

Should this actually be solved in CGI? It generates a lot of false warnings.
For reasons of compatibility param cannot just return a scalar.

Would it be simpler to only clear $CGI::LIST_CONTEXT_WARN for individual cases?
Comment 7 Jonathan Druart 2016-02-17 13:43:48 UTC
(In reply to Marcel de Rooy from comment #5)
> I am having the impression that we do not completely tackle the problem
> (read vulnerability given) here.
> Because just switching param to multi_param (without looking to the context)
> does not really solve it. You only suppress the warning.

Yes of course, but we are now aware of this problem, and it's the QA's job to catch them if they are wrongly used.

> Redefining methods/routines for lower versions of a module is not the most
> elegant solution (from QA perspective). If we could prevent doing so, we
> should. 
> Since we do not need to add calls to multi_param yet and we do not address
> the actual vulnerability in this patch, I would propose to not add this
> redefinition. We should concentrate on the calls to param in a hash context
> and scalarize them. (The warnings in the log show us where these calls are.) 

The goal was to remove the warnings for production installs using CGI < 4.08.
As a developer, I am using > 4.08 and will see the warnings in my logs.

(In reply to Marcel de Rooy from comment #6)
> Still we have the other situation that we do want multiple values:
> my @a = param( 'b' );
> 
> Should this actually be solved in CGI? It generates a lot of false warnings.
> For reasons of compatibility param cannot just return a scalar.

How do you want to fix that, I don't think I understand what you meant here.

> Would it be simpler to only clear $CGI::LIST_CONTEXT_WARN for individual
> cases?

I don't think so, developers won't see the warnings, we should keep them to know where the issues could come from.
Comment 8 Marcel de Rooy 2016-02-17 14:07:01 UTC
(In reply to Jonathan Druart from comment #7)
> The goal was to remove the warnings for production installs using CGI < 4.08.
> As a developer, I am using > 4.08 and will see the warnings in my logs.

Older CGI versions do not have this warning. I am not sure about the exact version, but it could well be that the introduction of multi_param also added this warning.
How does your solution differentiate production from other installs?
Comment 9 Jonathan Druart 2016-02-17 15:12:08 UTC
(In reply to Marcel de Rooy from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > The goal was to remove the warnings for production installs using CGI < 4.08.
> > As a developer, I am using > 4.08 and will see the warnings in my logs.
> 
> Older CGI versions do not have this warning. I am not sure about the exact
> version, but it could well be that the introduction of multi_param also
> added this warning.

You might be right. In this case, we could remove $CGI::LIST_CONTEXT_WARN = 0; from the patch.

> How does your solution differentiate production from other installs?

It does not
Comment 10 Galen Charlton 2016-02-17 17:25:02 UTC
So, ->param() starts displaying warnings when evaluated in list context as of CGI.pm 4.05.  ->multi_param() was added in 4.08 as a way of saying "I really want multiple parameter values, don't make me do { $CGI::LIST_CONTEXT_WARN = 0; @f = $q->param('foo'); } just to quell the warning."

To deal with the most common exploit scenario, "git grep '=>.*->param'" turns up ~270 cases where we most likely *don't* want multi_param(); rather, we want to wrap ->param in scalar(...).  I think that should be the first priority.

"git grep '@.*->param'" turns up 332 places in 120 files where a parameter is intentionally being fed into a list.  I'm not keen about monkey-patching a core module, though I recognize the expediency of it; but if we go that route so that we can start using ->multi_param() across the board, I think we *shouldn't* set $CGI::LIST_CONTEXT_WARN.  An alternative would be adding a bunch of "local $CGI::LIST_CONTEXT_WARN = 0;" and making a note to ourselves to replace that with ->multi_param() once we're past the point where stable Linux distros ship CGI.pm older than 4.08.  We could also do it like this:

Change:

   @f = $cgi->param('foo');

To:

  @f = Koha::CGI::multi_param($cgi, 'foo');

where Koha::CGI::multi_param looks something like this:

sub multi_param {
    my ($cgi, $param) = @_;
    local $CGI::LIST_CONTEXT_WARN = 0;
    return $cgi->param($param);  
}

That way, we're not monkey-patching a core module and we get something that we can mechanically translate to $cgi->multi_param once we're assured of having a recent enough version of CGI.pm.
Comment 11 Galen Charlton 2016-02-17 17:27:54 UTC
Yet another way to do it would be to make Koha::CGI be a full (but thin) OO wrapper around CGI.pm.
Comment 12 Jonathan Druart 2016-02-17 19:27:54 UTC
(In reply to Galen Charlton from comment #10)

I totally agree with the "what to do next" part, but what about now? My patch seems to match your requirements.

It could be very easy and quick to replace the different occurrences. I would like to avoid a temporary solution. Even if we need one, something different than "FIXME bug 2505" would be better ;)
Defining the CGi->multi_param method for < 4.08 and use CGI->param instead looks, to me, the easiest and cleanest solution.
We will just need to remove it when >= 4.08 will be widely used.
Comment 13 Marcel de Rooy 2016-02-18 07:43:39 UTC
(In reply to Jonathan Druart from comment #12)
> (In reply to Galen Charlton from comment #10)
> 
> I totally agree with the "what to do next" part, but what about now? My
> patch seems to match your requirements.

Jonathan, I am reading: "I am not keen about monkey patching a core module", which imo does not seem to translate to "match your requirements" :)
I would not opt for a complete Koha::CGI at this point. But I would (slightly) prefer the Koha::CGI::multi_param above "*CGI::multi_param = \&CGI::param". It will make any developer immediately aware of our "interception". The switchover to the real multi_param at some point in time will be completely painless.

Also note the different handling of the LIST_CONTEXT_WARN var.

> It could be very easy and quick to replace the different occurrences. I
> would like to avoid a temporary solution. Even if we need one, something
> different than "FIXME bug 2505" would be better ;)
Agreed, altough some code is temporary in any solution for now.
Comment 14 Jonathan Druart 2016-02-18 10:12:11 UTC
(In reply to Marcel de Rooy from comment #13)
> (In reply to Jonathan Druart from comment #12)
> > (In reply to Galen Charlton from comment #10)
> > 
> > I totally agree with the "what to do next" part, but what about now? My
> > patch seems to match your requirements.
> 
> Jonathan, I am reading: "I am not keen about monkey patching a core module",
> which imo does not seem to translate to "match your requirements" :)

Sorry, I thought Galen was talking about patching CGI (solution suggested on the ML).

> I would not opt for a complete Koha::CGI at this point. But I would
> (slightly) prefer the Koha::CGI::multi_param above "*CGI::multi_param =
> \&CGI::param". It will make any developer immediately aware of our
> "interception". The switchover to the real multi_param at some point in time
> will be completely painless.
> 
> Also note the different handling of the LIST_CONTEXT_WARN var.

Is anyone ready to submit a counter-patch?
Comment 15 Galen Charlton 2016-02-18 12:48:16 UTC
(In reply to Marcel de Rooy from comment #13)
> Jonathan, I am reading: "I am not keen about monkey patching a core module",
> which imo does not seem to translate to "match your requirements" :)
> I would not opt for a complete Koha::CGI at this point. But I would
> (slightly) prefer the Koha::CGI::multi_param above "*CGI::multi_param =
> \&CGI::param". It will make any developer immediately aware of our
> "interception". The switchover to the real multi_param at some point in time
> will be completely painless.

Indeed, Marcel has correctly identified what I was trying to say -- I meant "core module" in the sense of a module typically distributed with Perl, not a Koha module.
Comment 16 Galen Charlton 2016-02-18 12:52:49 UTC
(In reply to Jonathan Druart from comment #14)
> Is anyone ready to submit a counter-patch?

Working on one that is intended to actually fix at least some of the potential vulnerabilities.  I feel that this bug's title is not ideal, as the focus should be on resolving potential misuse of CGI parameters, not merely on quelling noisy warnings in the logs.
Comment 17 Jonathan Druart 2016-02-19 15:42:37 UTC
(In reply to Galen Charlton from comment #16)
> (In reply to Jonathan Druart from comment #14)
> > Is anyone ready to submit a counter-patch?
> 
> Working on one that is intended to actually fix at least some of the
> potential vulnerabilities.  I feel that this bug's title is not ideal, as
> the focus should be on resolving potential misuse of CGI parameters, not
> merely on quelling noisy warnings in the logs.

The purpose of this first patch was to provide a quick fix to allow the use of multi_param anywhere else.
The second step will be to get rid of the misuses of CGI->param
Comment 18 MJ Ray (software.coop) 2016-02-25 10:47:12 UTC
I think it's best to review and amend the param calls if there's only 332 as Galen suggests.  We did many more when facing the first SQL injection problem.
Comment 19 Jonathan Druart 2016-02-29 09:06:21 UTC
I really would like to move forward here to provide the next patch...
Comment 20 Marcel de Rooy 2016-02-29 09:23:46 UTC
(In reply to Galen Charlton from comment #16)
> Working on one that is intended to actually fix at least some of the
> potential vulnerabilities.  I feel that this bug's title is not ideal, as
> the focus should be on resolving potential misuse of CGI parameters, not
> merely on quelling noisy warnings in the logs.

Could you tell us along what lines you are doing that, and when you plan to submit?
Based on that, we can decide if this patch in some form can be pushed in advance..
Comment 21 Marcel de Rooy 2016-03-18 08:57:21 UTC
Created attachment 49300 [details] [review]
Bug 15809: Redefine multi_param is CGI < 4.08 is used

On debian Jessie, the CGI version is >= 4.08
Since this version, the param method raise a warning
"CGI::param called in list context".
Indeed, it can cause vulnerability if called in list context

https://metacpan.org/pod/CGI#Fetching-the-value-or-values-of-a-single-named-parameter
http://blog.gerv.net/2014/10/new-class-of-vulnerability-in-perl-web-applications/

There is a long journey to get rid of these warnings.
First I suggest to redefine the multi_param method when the CGI version
 installed is < 4.08, it will allow us to move the wrong ->param calls to
 ->multi_param without waiting for everybody to upgrade.

The different ways to call these 2 methods are:

my $foo = $cgi->param('foo'); # OK

my @foo = $cgi->param('foo'); # NOK, will raise the warning
my @foo = $cgi->multi_param('foo'); #OK

$template->param( foo => $cgi->param('foo') ); # NOK, will raise the warning
                                               # and vulnerable
$template->param( foo => scalar $cgi->param('foo') ); # OK

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested a call to multi_param with CGI < 4.08.
With reference to the comments on Bugzilla, this workaround is arguable,
but provides a base to move to multi_param. If we come up with a better
solution, it should be easy to adjust.
Comment 22 Marcel de Rooy 2016-03-18 08:57:42 UTC
QA Comment:
With reference to the discussion above, I am moving the status to PQA. This workaround is arguable however.
Note that Galen was working on a solution too, but did not yet reply.
In the meantime we could use this, if the RM agrees too.
Also note that this workaround depends on C4::Context. We have some scripts and tests using CGI without Context (or perhaps implicit).
It might be safer too to put the multi_param redefinition outside the if ($ENV{'HTTP_USER_AGENT'}) block?
The nature of this change makes that adding multi_param in some special places could lead to undefined subroutine errors in older production systems while developers do not catch it on newer systems.
Comment 23 Brendan Gallagher 2016-03-22 23:24:10 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 24 Jonathan Druart 2016-03-29 09:36:26 UTC
The job is going to continue on bug 16154.
Comment 25 Julian Maurice 2016-04-08 05:43:06 UTC
Patch pushed to 3.22.x, will be in 3.22.6
Comment 26 Frédéric Demians 2016-04-27 15:35:22 UTC
Pushed to 3.22.x, will be in 3.20.11.