Item13125: CGI::param called in list context from package Foswiki::Engine::CGI line 222,

Recent versions of CGI complain about use of list context.

#### Method: param / multi_param
# Returns the value(s)of a named parameter.
# If invoked in a list context, returns the
# entire list.  Otherwise returns the first
# member of the list.
# If name is not provided, return a list of all
# the known parameters names available.
# If more than one argument is provided, the
# second and subsequent arguments are used to
# set the value of the parameter.
#
# note that calling param() in list context
# will raise a warning about potential bad
# things, hence the multi_param method
####

This was added in CGI version 4.08. Earlier versions do not generate the warning. See also http://seclists.org/vulnwatch/2006/q4/6. TWiki's take on the error at http://twiki.org/cgi-bin/view/Support/SID-01980.

[Sun Nov 30 12:55:38 2014] login: CGI::param called in list context from package Foswiki::Engine::CGI line 222, 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.

-- GeorgeClark - 30 Nov 2014

Since it's possible for extensions to call CGI directly, rather than disable the warning, we need to:

• Review code and double check that all CGI::param in list context is acceptable.
• Test CGI version, or ->can(multi_param) and make the correct call for the CGI version

This allows the code to still catch and warn about direct CGI calls that might not be handling multi-valued parameters correctly

-- GeorgeClark - 30 Nov 2014

... and for the impatient, use this patch to quiet the noise.

diff --git a/core/lib/Foswiki/Engine.pm b/core/lib/Foswiki/Engine.pm
index a9a64a2..dac1527 100644
--- a/core/lib/Foswiki/Engine.pm
+++ b/core/lib/Foswiki/Engine.pm
@@ -19,6 +19,7 @@ use warnings;
 use Error qw( :try );
 use Assert;
 use Scalar::Util ();
+$CGI::LIST_CONTEXT_WARN = 0;
 
 BEGIN {
     if ( $Foswiki::cfg{UseLocale} ) {

-- MichaelDaum - 01 Dec 2014

The basic coding pattern to watch out for is

my $hashref = {name => $q->param('name')};

Problems arise when param('name') returns a list instead of a scalar which is the case when name is multi-valued. As a consequence the inner expression of the hash constructor will potentially initialize additional properties besides name.

Example: http://example.com/somecgi?name=1&name=2&name=evilkey&name=evilvalue

Hash:

$VAR1 = {
          'evilkey' => 'evilvalue',
          'name' => '2'
        };

See http://seclists.org/vulnwatch/2006/q4/6 for details.

Analyzing the Foswiki core code there are at least 4 spots matching a grep

grep -r -- "->param([^)][^)]*) *}" lib/Foswiki

(please help improving this grep)

Result:

lib/Foswiki/Request.pm:        if ( my $upload = $this->{uploads}{ $this->param($p) } ) {
lib/Foswiki/Request.pm:            CORE::delete $this->{uploads}{ $this->param($p) };
lib/Foswiki/Request.pm:    my $upload = $this->{uploads}{ $this->param($name) };
lib/Foswiki/Configure/Wizards/Save.pm:        while ( my ( $k, $v ) = each %{ $this->param('set') } ) {

This with my limitted understanding of the issue.

Another spot to consider:

# Handle approver action; either approve or deny
sub _checkApproval {
...
        my $data = {
            EmailAddress => $session->{request}->param('email'),
            Referee      => $session->{request}->param('referee'),
            WikiName     => Foswiki::Sandbox::untaint(
                $session->{request}->param('wikiname') || 'UnknownUser',
                \&Foswiki::Sandbox::validateTopicName
            ),
            Feedback => $session->{request}->param('feedback')
        };
...

-- MichaelDaum - 01 Dec 2014

Discussion from IRC - Release meeting & Security channels. I redacted the discussion from the release meeting minutes.

-- GeorgeClark - 01 Dec 2014

I've got a possible fix started that so far tests backwards compatible to older versions of CGI. So far running master on the latest CGI, this seems to be the only place triggering the use of param in a list context.

Rather than suppressing the errors, this lets us find questionable uses while remaining backwards compatible with older versions of CGI.

One issue I see is that our Foswiki::Request is how we access CGI parameters. Other than in Engine, the calls to param() are made against Request::param(), and not CGI::param(). So this is probably a deeper than simply handling calls to CGI. Our implementation of Request::param() needs to be checked against the vulnerability in CGI::param() to make sure we don't have a similar problem. Given that Request mirrors CGI, we should consider implementing Request::multi_param() and following the same checks done in CGI.

diff --git a/core/lib/Foswiki/Engine/CGI.pm b/core/lib/Foswiki/Engine/CGI.pm
index 6200f8d..69612bb 100644
--- a/core/lib/Foswiki/Engine/CGI.pm
+++ b/core/lib/Foswiki/Engine/CGI.pm
@@ -207,6 +207,12 @@ sub prepareBody {
     $CONSTRUCTOR_PID = $$;
 
     my $cgi = new CGI();
+    unless ($cgi->can('multi_param')) {
+        no warnings 'redefine';
+        print STDERR "REDEFINED CGI multi_param\n";
+        *CGI::multi_param = \&CGI::param;
+        use warnings 'redefine';
+    }
     my $err = $cgi->cgi_error;
     throw Foswiki::EngineException( $1, $2 )
       if defined $err && $err =~ /\s*(\d{3})\s*(.*)/;
@@ -217,9 +223,9 @@ sub prepareBodyParameters {
     my ( $this, $req ) = @_;
 
     return unless $ENV{CONTENT_LENGTH};
-    my @plist = $this->{cgi}->param();
+    my @plist = $this->{cgi}->multi_param();
     foreach my $pname (@plist) {
-        my @values = map { "$_" } $this->{cgi}->param($pname);
+        my @values = map { "$_" } $this->{cgi}->multi_param($pname);
         $req->bodyParam( -name => $pname, -value => \@values );
         $this->{uploads}{$pname} = 1 if scalar $this->{cgi}->upload($pname);
     }

-- GeorgeClark - 06 Dec 2014

And.. This diff implements the same checks in Foswiki::Request ... Triggers a lot of warinings. This will be more reliable than code inspection:

diff --git a/core/lib/Foswiki/Request.pm b/core/lib/Foswiki/Request.pm
index f2dffd2..dab43c5 100644
--- a/core/lib/Foswiki/Request.pm
+++ b/core/lib/Foswiki/Request.pm
@@ -421,12 +421,32 @@ Resonably compatible with CGI.
 
 =cut
 
+sub multi_param {
+
+    my @list_of_params = param(@_);
+    return @list_of_params;
+}
+
 sub param {
-    my ( $this, @p ) = @_;
+    my ( $this, @p ) = (@_);
 
     my ( $key, @value ) = rearrange( [ 'NAME', [qw(VALUE VALUES)] ], @p );
 
     return @{ $this->{param_list} } unless defined $key;
+
+# list context can be dangerous so warn:
+# http://blog.gerv.net/2014.10/new-class-of-vulnerability-in-perl-web-applications
+    if (wantarray) {
+        my ( $package, $filename, $line ) = caller;
+        if ( $package ne 'Foswiki::Request' ) {
+            warn
+"Foswiki::Request::param called in list context from package $package line $line, declare as scalar, or call multi_param. ";
+        }
+    }
+
     if ( defined $value[0] ) {
         push @{ $this->{param_list} }, $key
           unless exists $this->{param}{$key};

-- GeorgeClark - 06 Dec 2014

More complete patch attached. In addition to warning if ::param() is called in a list context, it also warns when called as a scalar and the parameter is multivalued.

Fixed errors reported for routine view, edit, save, and user registration, table sorting ...

• cgi-param.patch: Test patch for handling multivalue parameters

-- GeorgeClark - 06 Dec 2014

Hi all, opinions on the attached patch? Check it in now? I don't think we have exposed any vulnerabilities but I'm not certain.

-- GeorgeClark - 09 Dec 2014

Calling ::param in list context isn't dangerous per se. There are three conditions that must match:

1. ::param is used in list context
2. the returned list is used to initialized a hash
3. hash is being read in a generic way processing all key-value pairs (e.g. while key,value = each %hash; $file .= $key . $value; ... # stupid)

These three factors may lead to the hash being initialized and used in undesirable ways ... which is the vulnerability.

Again, the following code alone is NOT vulnerable by any means:

@list = $cgi->param("key");

It is 100% secure and THE most common way to read multiple params from CGI. Only when @list is then being used to initialized a HASH might it cause problems as it is not clear which key-value pairs are contained.

As such replacing any used of ::param with ::multi_param might not shut down this attack vector at all. While coders might now be more aware of a @list being returned, I doubt they were not before. Still, the returned @list of params could be used as a HASH initializer causing exactly the same problems as using it the old way before:

@list = $cgi->multi_param("key");

What holds me back to misuse this @list the same way it has been constructed before using ::param ???

Frankly, I don't really understand the recommended fix in the advisory. The only thing it does is shutting down a warning by replacing param with multi_param in list context. The misuse of the returned list is not prevented.

Wrt the patch, I am puzzled by things like

-        $req->queryParam( $param, $params{$param} );
+        ( undef ) = $req->queryParam( $param, $params{$param} );

This only for the sake of a list context? Wtf.

-- MichaelDaum - 09 Dec 2014

There is an additional way param can become dangerous in list context: when it's used in the parameter list of a function call, like so (contrived example):

Foswiki::Func::saveTopicText($web, $topic, $query->param('newtext'), 0);

In that function, the parameter after $text is $ignorePermissions. If someone passes a POST body containing newtext=my%20body;newtext=1... guess what, we're now overwriting topics the user isn't actually allowed to overwrite.

@Main.MichaelDaum: I don't see any mention of multi_param in the advisory linked above, and you're right that blindly replacing param everywhere would not help. The purpose of multi_param is to communicate the intention of the developer. Use of multi_param asserts that the developer knows that multiple values may be returned. Blindly substitung multi_param everywhere would completely destroy that.

Our current approach is to warn when param is used in list context. This is only helpful if core/extension developers notice the warnings, figure out what they mean, and change their code. If they don't, the warning is only window dressing and there is no actual security.

A stronger protection would be to die in the same case. This is probably a bit extreme... especially since there are several cases in which calling param in list context is perfectly fine.

The way some other frameworks, e.g. Mojolicious, do it: make param return one value no matter what context it's called in, and make multi_param return a list no matter what context it's called in.

The impact of that approach on previously written code is that, in many cases, code that expects to get a list will now get only a single value. It will be not too enjoyable to fix all the code, but at least this is actually secure and does not force us to make scalar context explicit all over the place.

-- JanKrueger - 09 Dec 2014

Why only CGI::param? Doesn't any function that behaves differently in scalar and list context cause an issue? The problem is either bigger as suggested or potemkinsh.

-- MichaelDaum - 09 Dec 2014

My fix was not blindly replacing param with multi_param. Where I changed it I:

• looked to see if a list was expected / desired, and made that multi_param.
• Looked to see if a list was NOT expected, and changed that to an explicit scalar
• And the ( wtf ) was because calling it as a null resulted in the warning,

I chose to go with the multi_param instead of just turning off warnings so that we would find any possible misuse, and fix them rather than depending on grep and code inspection. Also this finds future misuse of Request->param() rather than trusting that we got them all on the first time and turning off warnings. So my implementation strategy was to implement the same checks in our cloned param() code, test, find a "warn", review list or scalar, fix, ... repeat.

-- GeorgeClark - 09 Dec 2014

I had to make several more changes that were due to calls in a list context. While in there, I bracketed all uses of scalar() to reduce ambiguity.

-- CrawfordCurrie - 16 Jan 2015

Setting to waiting for release. Reopen if more issues arise.

-- GeorgeClark - 02 Mar 2015

Commenting is disabled while not logged in