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

pencil
Priority: Security
Current State: Closed
Released In: 2.0.0
Target Release: major
Applies To: Engine
Component: FoswikiEngineCGI
Branches: master
Reported By: GeorgeClark
Waiting For:
Last Change By: GeorgeClark
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.

(08:40:39 AM) MichaelDaum: http://foswiki.org/Tasks/Item13125 ... how do we deal with it during the release? 
(08:41:13 AM) MichaelDaum: I am not really sure I know what the new warning messages are ... but provided a patch to quiet it ;)
..
(08:43:27 AM) gac410: Item13125 - The solution is to explicitly use multi-param   if you expect multi-valued paramegers,     But need to can->('multi-param')   since it's a new addition.
(08:43:59 AM) gac410: Basically cgi->param   in a @list context will throw the warning,  since behavior is unpredictable based upon browser input.
(08:45:02 AM) gac410: So adding the workaround is certainly safe ... we will work exactly like we always worked.   But going with the new call without disabling the warning might help plugin developers avoid cgi errors.
(08:45:33 AM) gac410: I wouldnt' hold up the release for a "good fix"     
(08:45:39 AM) MichaelDaum: @vals = $cgi->param("multi") is unexpected with only one multi urlparam ? why's that?
(08:46:02 AM) MichaelDaum: I'd expect a list with one item in it
(08:47:01 AM) gac410: tbh I'm not really sure.      Yes that's what happens.     Maybe a tempest in teapot.   
(08:47:26 AM) gac410: But I guess there were some security implications,  and the cgi maintainers decided to change the behavior
...
(08:53:47 AM) MichaelDaum: http://seclists.org/vulnwatch/2006/q4/6 is the security report to consider while checking our code 
(08:54:12 AM) MichaelDaum: ... for the cgi param thing
(08:54:46 AM) gac410: Thanks MichaelDaum    Maybe a review of that and we can decide just to add the error suppress.  
(08:56:14 AM) MichaelDaum: the pattern to look for is %{$cgi->param("multi")} ... that is cgi->param as a HASH constructor returning a ref to it ... wild wild assumption. never seen anywhere in the code. though not 1000% sure.
(08:57:00 AM) gac410: Okay  worth a review.   I agree I think we handle the multi-value parms correctly.
(08:58:03 AM) MichaelDaum: plus: their scenario is adding Data::FormValidator; to it... i.e. its _validate_ method being called in list context ... not cgi->param directly
(08:58:50 AM) MichaelDaum: this after reading the seclist entry cursory

.... Discussion moved to #Foswiki-security

(10:24:50 AM) ***MichaelDaum reviewing all use of ->param() in the core code
(10:25:11 AM) MichaelDaum: things to look for is cgi->param() being called inside a hash constructor
(10:25:22 AM) MichaelDaum: such as %{ $this->param(
(10:25:27 AM) MichaelDaum: such as %{ $this->param('set) }
(10:25:41 AM) MichaelDaum: ^set^set'
(10:26:14 AM) MichaelDaum: I've found 3 in Request.pm and one in ./Configure/Wizards/Save.pm
(10:26:37 AM) jast: or inside a function's arg list
(10:26:49 AM) gac410: wonderful   That's not good news
(10:28:13 AM) MichaelDaum: jast right
(10:28:36 AM) MichaelDaum: the first is: if ( my $upload = $this->{uploads}{ $this->param($p) } ) {
(10:28:56 AM) MichaelDaum: in Request.pm
(10:29:58 AM) MichaelDaum: followed by CORE::delete $this->{uploads}{ $this->param($p) }; one line below
(10:30:07 AM) gac410: I'm glad I didn't just go with the TWiki solution - quiet the messages and ignore it.
(10:32:22 AM) MichaelDaum: jast, have you got any idea how to deal with these code spots?
(10:33:30 AM) MichaelDaum: if there is at least a 10% chance that our code indeed is affected by http://seclists.org/vulnwatch/2006/q4/6 then we have to raise http://foswiki.org/Tasks/Item13125 to security level until further clarified
(10:33:49 AM) ***MichaelDaum doing so docu'ing the findings
(10:37:37 AM) jast: actually your example from Request.pm should be "fine" – hash lookups are scalar context afaik
(10:37:50 AM) jast: you're not accessing a hash slice unless you do @myhash{...}
(10:38:43 AM) jast: it's hash literals where you run into problems
(10:42:30 AM) MichaelDaum: any code example?
(10:42:45 AM) MichaelDaum: in core?
(10:43:23 AM) jast: I haven't really looked
(10:43:23 AM) MichaelDaum: see http://foswiki.org/Tasks/Item13125
(10:43:40 AM) MichaelDaum: could you comment on the 4 code spots I listed there?
(10:44:29 AM) jast: I don't see an issue with the first three
(10:44:46 AM) jast: and I think the fourth is deliberate
(10:44:57 AM) jast: there'd be no sense in iterating over something we expect to be a single value
(10:45:15 AM) MichaelDaum: y
(10:46:00 AM) MichaelDaum: however {uploads}{ $this->param($p)} could potentially construct {upload}{lots of evil props} inside
(10:46:58 AM) MichaelDaum: the first two are part of a delete()
(10:47:37 AM) MichaelDaum: not that I understand the code at all
(10:48:29 AM) jast: as I said... @foo{list here} is a thing... $foo{list here} isn't
(10:50:17 AM) jast: perl -e '%a=(1,2,3,4); print defined($a{1,3})."\n";'
(10:50:56 AM) jast: do the same with @a{1,3} and it does give a result
(10:51:19 AM) MichaelDaum: the code pattern on  seclists is my $hashref = {name => $q->param('name')};
(10:51:30 AM) MichaelDaum: which is constructing a hash ref
(10:51:31 AM) jast: yeah, and that one is definitely a problem
(10:51:43 AM) jast: it would be a problem for constructing a normal hash, too
(10:51:59 AM) jast: %h = (name => $q->param('exploitmepls'));
(10:54:35 AM) MichaelDaum: ah ok
(10:55:51 AM) MichaelDaum: greping for any "=>.*-<param"
(10:55:56 AM) MichaelDaum: greping for any "=>.*->param"
(10:57:49 AM) MichaelDaum: lib/Foswiki/UI/Register.pm:            Referee      => $session->{request}->param('referee'),
(10:58:20 AM) MichaelDaum: line 242ff
(10:58:27 AM) MichaelDaum: a couple of them
(11:03:55 AM) jast: yeah, that's probably a problem
(11:04:27 AM) jast: fix is straightforward: ... => scalar($session->{request}->param(...))
(11:05:09 AM) MichaelDaum: the properties of the $data hash are then used to call _sendEmail which expands macros n stuff
(11:05:12 AM) jast: in this case the impact is low because the code only renders an e-mail to be sent to the person
(11:05:35 AM) jast: well unless the code double-expands macros, I guess
(11:05:42 AM) MichaelDaum: so you could email any result of a %macro?
(11:57:11 AM) gac410: Any verdict yet? ... do we have an exposure,  and if so, is it CVE-worthy?
(11:58:35 AM) MichaelDaum: dunno, it needs somebody more security savvy
(12:00:12 PM) jast: we'd need to comb through the code to be sure
(12:01:36 PM) jast: the grep only catches "likely" problems
(12:38:32 PM) MichaelDaum left the room (quit: ).

-- 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 ...

-- 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
 

Topic attachments
I Attachment Action Size Date Who Comment
cgi-param.patchpatch cgi-param.patch manage 9 K 06 Dec 2014 - 19:30 GeorgeClark Test patch for handling multivalue parameters
Topic revision: r28 - 05 Jul 2015, GeorgeClark - This page was cached on 24 Aug 2016 - 03:35.

The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License