Bug 15747 - Auth.pm flooding error log with "CGI::param called in list context"
Summary: Auth.pm flooding error log with "CGI::param called in list context"
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 11559
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-05 14:46 UTC by Owen Leonard
Modified: 2017-06-14 22:02 UTC (History)
8 users (show)

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


Attachments
Bug 15747 Auth.pm flooding error log with "CGI::param called in list context" (1.94 KB, patch)
2016-02-11 14:56 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15747 Auth.pm flooding error log with "CGI::param called in list context" (1.66 KB, patch)
2016-02-11 15:02 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15747 Auth.pm flooding error log with "CGI::param called in list context" (1.84 KB, patch)
2016-02-12 10:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15747: Do not use CGI->param in list context - Auth.pm (863 bytes, patch)
2016-02-12 11:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15747: Do not use CGI->param in list context - Auth.pm (1.05 KB, patch)
2016-02-15 16:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15747: Do not use CGI->param in list context - Auth.pm (1.20 KB, patch)
2016-02-18 10:47 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 Owen Leonard 2016-02-05 14:46:35 UTC
In my error log for the staff client:

CGI::param called in list context from package C4::Auth line 401, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.

From the OPAC error log:

CGI::param called in list context from package C4::Auth line 399, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.
Comment 1 Mark Tompsett 2016-02-11 14:56:12 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2016-02-11 15:02:00 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2016-02-12 10:54:01 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-02-12 11:46:06 UTC
Jesse, could you explain what you are doing with document.location.hash?

I do not understand the following line in includes/cateditor-ui.inc:

1094         if ( "[% auth_forwarded_hash %]" ) {
1095             document.location.hash = "[% auth_forwarded_hash %]";
1096         }

Should not be:

1094         [% IF auth_forwarded_hash %]
1095             document.location.hash = "[% auth_forwarded_hash %]";
1096         [% END %]

instead?
Comment 5 Jonathan Druart 2016-02-12 11:47:54 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-02-12 11:49:18 UTC
(In reply to Jonathan Druart from comment #5)
> Created attachment 47966 [details] [review] [review]
> Bug 15747: Do not use CGI->param in list context - Auth.pm

Theoretically, this patch should be the right way to remove the warning, but I have no idea how to test it.
Comment 7 Marcel de Rooy 2016-02-12 13:08:13 UTC
Jonathan: What happened here?
Previous patch, status etc ??
Comment 8 Marcel de Rooy 2016-02-12 13:09:17 UTC
The change you make is trivial and is no reason to obsolete what was before.
Comment 9 Jonathan Druart 2016-02-12 13:28:46 UTC
I was working on bug 14075 and bug 15809 at the same time (same issue).
I am trying to find a global solution to fix them.
The previous patch uses an unneeded var. To remove the warnings all around the code, it would be good to have the same pattern everywhere. That's why I have submitted another patch.
To be sure everything is fine (even if I am sure it is), I was trying to understand how I could test there is no regression.
I have asked Jesse to explain the goal of the auth_forwarded_hash and how it can be tested.
Comment 10 Marcel de Rooy 2016-02-12 13:33:07 UTC
(In reply to Jonathan Druart from comment #9)
> I was working on bug 14075 and bug 15809 at the same time (same issue).
> I am trying to find a global solution to fix them.
> The previous patch uses an unneeded var. To remove the warnings all around
> the code, it would be good to have the same pattern everywhere. That's why I
> have submitted another patch.
> To be sure everything is fine (even if I am sure it is), I was trying to
> understand how I could test there is no regression.
> I have asked Jesse to explain the goal of the auth_forwarded_hash and how it
> can be tested.

THx for replying. I would recommend to restore the old patch and status now. No reason to block this. What happens with the auth_forwarded_hash could be considered as outside the scope of this small and effective solution. Please respect the work of the original submitter.
Comment 11 Mark Tompsett 2016-02-12 14:17:16 UTC
(In reply to Jonathan Druart from comment #9)
> I was working on bug 14075 and bug 15809 at the same time (same issue).
> I am trying to find a global solution to fix them.

Oooo... bug 15809 looks promising.
I'm okay with this simplified patch.
Comment 12 Marcel de Rooy 2016-02-12 16:35:31 UTC
(In reply to M. Tompsett from comment #11)
> (In reply to Jonathan Druart from comment #9)
> > I was working on bug 14075 and bug 15809 at the same time (same issue).
> > I am trying to find a global solution to fix them.
> 
> Oooo... bug 15809 looks promising.
> I'm okay with this simplified patch.

That is fine. Well, Jonathan, go for it. Can we close this one?
Comment 13 Mark Tompsett 2016-02-12 16:40:49 UTC
(In reply to Marcel de Rooy from comment #12)
> That is fine. Well, Jonathan, go for it. Can we close this one?

I don't think so, because this doesn't use the 15809 patch.
If it did, then it would be multi_param. :)

Either way, I'm cool with this (without 15809) or
a multi-param version dependent on 15809.
Comment 14 Jesse Weaver 2016-02-12 17:42:25 UTC
(In reply to Jonathan Druart from comment #4)
> Jesse, could you explain what you are doing with document.location.hash?
> 
> I do not understand the following line in includes/cateditor-ui.inc:
> 
> 1094         if ( "[% auth_forwarded_hash %]" ) {
> 1095             document.location.hash = "[% auth_forwarded_hash %]";
> 1096         }
> 
> Should not be:
> 
> 1094         [% IF auth_forwarded_hash %]
> 1095             document.location.hash = "[% auth_forwarded_hash %]";
> 1096         [% END %]
> 
> instead?

I don't quite remember why I did it that way. Your proposed change should be fine.
Comment 15 Jesse Weaver 2016-02-12 17:47:11 UTC
(In reply to Jesse Weaver from comment #14)
> (In reply to Jonathan Druart from comment #4)
> > Jesse, could you explain what you are doing with document.location.hash?
> > 
> > I do not understand the following line in includes/cateditor-ui.inc:
> > 
> > 1094         if ( "[% auth_forwarded_hash %]" ) {
> > 1095             document.location.hash = "[% auth_forwarded_hash %]";
> > 1096         }
> > 
> > Should not be:
> > 
> > 1094         [% IF auth_forwarded_hash %]
> > 1095             document.location.hash = "[% auth_forwarded_hash %]";
> > 1096         [% END %]
> > 
> > instead?
> 
> I don't quite remember why I did it that way. Your proposed change should be
> fine.

And to clarify; this is to make sure that the hash in .../editor.pl#catalog/647 is passed through when you have to log in again.
Comment 16 Jonathan Druart 2016-02-15 16:34:19 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-02-15 16:34:42 UTC
(In reply to Jesse Weaver from comment #14)
> (In reply to Jonathan Druart from comment #4)
> > Jesse, could you explain what you are doing with document.location.hash?
> > 
> > I do not understand the following line in includes/cateditor-ui.inc:
> > 
> > 1094         if ( "[% auth_forwarded_hash %]" ) {
> > 1095             document.location.hash = "[% auth_forwarded_hash %]";
> > 1096         }
> > 
> > Should not be:
> > 
> > 1094         [% IF auth_forwarded_hash %]
> > 1095             document.location.hash = "[% auth_forwarded_hash %]";
> > 1096         [% END %]
> > 
> > instead?
> 
> I don't quite remember why I did it that way. Your proposed change should be
> fine.

I let you fix it if you think it's needed.
Comment 18 David Cook 2016-02-15 23:39:06 UTC
Jonathan's patch looks good - provided that $in->{'query'}->param('auth_forwarded_hash') is a single value and not a multi value. (I assume that it is a single value.)

I'd like to clarify one thing though. The warning "Do not use CGI->param in list context" is not strictly necessary.

The problem isn't with CGI->param() in list context per se but rather with CGI->param("foo") in list context. I'm using "my @param_names = CGI->param()" elsewhere and it doesn't generate warnings. It only generates warnings if you're passing the method an argument, as it introduces some ambiguity and the possibility of buggy behaviour. Here's the example from CGI:

my %user_info = (
    id   => 1,
    name => $query->param('name'),
);

In theory, someone could submit multiple "name" arguments, so the above could actually be interpolated like this:

my %user_info = (
    id   => 1,
    name => "bruce", "wayne", "clark", "kent"
);

If you run the code with that list as the value for "name", you'll get the following hash after interpolation:

'id' => 1,
'name' => 'bruce',
'kent' => undef,
'wayne' => 'clark'

That's definitely a vulnerability. 

So if you know that you only have one "name", you can use "scalar $query->param('name')". 

However, if you know that you have multiple names, but don't want to wind up with a mangled hash, I think you should be able to do the following:

my %user_info = (
    id   => 1,
    name => [$query->param('name')],
);

That should give you the following:

'id' => 1,
'name' => [
    'bruce',
    'wayne',
    'clark',
    'kent'
]

Of course, I think you'd still get the warning even if you did $query->param('name'), which is why CGI->multi_param probably makes more sense as per https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15809.
Comment 19 David Cook 2016-02-15 23:44:54 UTC
So yeah, even if you use the [] to create an arrayref, you'll still get the CGI warning, because CGI just knows you're asking for a single named parameter in list context; it doesn't know that you're going to create a reference using that list. 

But... even if you were using CGI->multi_param... you'd still need to use the [] construct to use an arrayref for a hash value rather than accidentally filling your hash full of other values because you added a list...

Anyway, here's some easy tests to run:

--------
BAD NEWS:

use Modern::Perl;
use CGI;
use Data::Dumper;

my $query = CGI->new();
$query->param('name',"bruce","wayne","clark","kent");

my %hash = (
    id => 1,
    name => $query->param('name')
);

warn Dumper(\%hash);
--------
WELL DONE:

use Modern::Perl;
use CGI;
use Data::Dumper;

my $query = CGI->new();
$query->param('name',"bruce","wayne","clark","kent");

my %hash = (
    id => 1,
    name => [$query->param('name')]
);

warn Dumper(\%hash);

---------
HALFWAY THERE:

use Modern::Perl;
use CGI;
use Data::Dumper;

my $query = CGI->new();
$query->param('name',"bruce","wayne","clark","kent");

my %hash = (
    id => 1,
    name => scalar $query->param('name')
);

warn Dumper(\%hash);
Comment 20 Jonathan Druart 2016-02-16 08:45:31 UTC
(In reply to David Cook from comment #18)
> I'm using "my @param_names = CGI->param()" elsewhere and it doesn't generate warnings.

Yes it does.
Comment 21 David Cook 2016-02-16 23:30:16 UTC
(In reply to Jonathan Druart from comment #20)
> (In reply to David Cook from comment #18)
> > I'm using "my @param_names = CGI->param()" elsewhere and it doesn't generate warnings.
> 
> Yes it does.

No, it doesn't. Here's the evidence:

cat test.pl && perl test.pl
use Modern::Perl;
use CGI;
use Data::Dumper;

my $query = CGI->new();
$query->param('name',"bruce","wayne","clark","kent");

my @name = $query->param();
say Dumper(\@name);
$VAR1 = [
          'name'
        ];

--

cat test.pl && perl test.pl
use Modern::Perl;
use CGI;
use Data::Dumper;

my $query = CGI->new();
$query->param('name',"bruce","wayne","clark","kent");

my @name = $query->param("name");
say Dumper(\@name);
CGI::param called in list context from package main line 8, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/lib/perl5/site_perl/5.20.1/CGI.pm line 437.
$VAR1 = [
          'bruce',
          'wayne',
          'clark',
          'kent'
        ];

--

As you can observe in the warning itself, the warning is for "Fetching the value or values of a single named parameter". In the case of '$query->param("name")', we're fetching the value of a single named parameter. In the case of '$query->param()', we're fetching the names of all the parameters. It's completely different.
Comment 22 David Cook 2016-02-16 23:30:45 UTC
P.S. That is CGI.pm version 4.13.
Comment 23 Jonathan Druart 2016-02-17 09:57:50 UTC
(In reply to David Cook from comment #21)
> As you can observe in the warning itself, the warning is for "Fetching the
> value or values of a single named parameter". In the case of
> '$query->param("name")', we're fetching the value of a single named
> parameter. In the case of '$query->param()', we're fetching the names of all
> the parameters. It's completely different.

I don't understand, we both agree, it generates a warning.
Comment 24 Marcel de Rooy 2016-02-17 12:29:58 UTC
(In reply to Jonathan Druart from comment #23)
> (In reply to David Cook from comment #21)
> > As you can observe in the warning itself, the warning is for "Fetching the
> > value or values of a single named parameter". In the case of
> > '$query->param("name")', we're fetching the value of a single named
> > parameter. In the case of '$query->param()', we're fetching the names of all
> > the parameters. It's completely different.
> 
> I don't understand, we both agree, it generates a warning.

Misunderstanding? He talks about calling it without arguments (and without warning). You mean calls with an argument (and warnings dependent on context).
Comment 25 David Cook 2016-02-18 05:49:38 UTC
(In reply to Marcel de Rooy from comment #24)
> Misunderstanding? He talks about calling it without arguments (and without
> warning). You mean calls with an argument (and warnings dependent on
> context).

Certainly a misunderstanding I think.

Generates no warning:
my @parameter_names = CGI->param();

Generates no warning:

my $name = CGI->param("foo");

Generates a warning:
my @name = CGI->param("foo");
Comment 26 Jonathan Druart 2016-02-18 10:16:34 UTC
Yes sorry, didn't  read carefully enough.
What's missing to get a signoff?
Comment 27 Marcel de Rooy 2016-02-18 10:30:30 UTC
(In reply to Jonathan Druart from comment #26)
> Yes sorry, didn't  read carefully enough.
> What's missing to get a signoff?

I signed off Mark's patch. Your patch is more or less identical..
Comment 28 Marcel de Rooy 2016-02-18 10:47:36 UTC
Created attachment 48204 [details] [review]
Bug 15747: Do not use CGI->param in list context - Auth.pm

Test plan:
Log you in!
Without this patch applied, you will get a warning
"Fetching the value or values of a single named parameter"
With this patch applied, the warning from C4::Auth line 401 should not
appear anymore in the log file.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Note: no warnings for older CGI versions.
Comment 29 Marcel de Rooy 2016-02-18 10:50:16 UTC
QA Comment:
As you can see, this already received (too) much attention :)
Taking the liberty to promote this trivial patch to PQA therefore.

Note: I checked the specific variable auth_forwarded_hash. It is used to store the document.location.hash. Single value!
Comment 30 Brendan Gallagher 2016-02-24 02:01:47 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 31 Julian Maurice 2016-02-26 15:28:52 UTC
Patch pushed to 3.22.x, will be in 3.22.4