1074980 - Forbid the { foo => $cgi->param() } syntax to prevent data override

Status: RESOLVED FIXED

(Bugzilla :: Testing Suite, enhancement)

Description

[Frédéric Buclin]

Bug 1074812 is not the first one to report issues when writing

  { foo => $cgi->param("bar") }

but it's certainly the most critical bug in this category.

If you pass the 'bar' argument several times in a URL, such as &bar=4&bar=new_field&bar=new_value, then the hash above becomes:

  { foo => 4, new_field => new_value }

which lets you inject new field/value pairs or to override already set field/value pairs. This can lead to security vulnerabilities in case the hash is passed to e.g. a constructor/destructor such as Bugzilla::Foo->create() or ->update() or ->delete() as data has been compromised by the caller. If these methods trust the caller for some of its data, then you are in trouble.

I will attach a test to prevent this syntax. The right way to go is to force the CGI argument to be read in scalar context, i.e.:

  { foo => scalar $cgi->param("bar") }


I'm leaving the security flag for now as 8 files in the Bugzilla codebase are currently using this syntax and need careful audit before being disclosed.

Comment 1

[Dave Miller [:justdave]]

I suggest we resolve these as part of the same security release (or at least determine that they're not exploitable first) as I can pretty much guarantee you that people will go looking for this and want to claim bounties once the one in bug 1074812 is disclosed (that was my first instinct upon seeing it as well).

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

Many files caught by this test!

Comment 3

[David Lawrence [:dkl]]

Comment on attachment 8497643 [details] [diff] [review]
patch, v1

Review of attachment 8497643 [details] [diff] [review]:
-----------------------------------------------------------------

error at the end:

# Looks like you planned 1374 tests but ran 1381.

Comment 4

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #3)
> Comment on attachment 8497643 [details] [diff] [review]
> patch, v1
> 
> Review of attachment 8497643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> error at the end:
> 
> # Looks like you planned 1374 tests but ran 1381.

Looking at the patch, we will not know how many tests will be executed depending on how
many lines in a file match the regexp check for cgi->param.

You may need to use instead:

use Test::More qw(no_plan);

Comment 5

[Frédéric Buclin]

Attachment: patch, v2

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 8497758 [details] [diff] [review]
patch, v2

Review of attachment 8497758 [details] [diff] [review]:
-----------------------------------------------------------------

I would like to see "all" offending lines in a file in the same test run so please implement similar to this:

+    my $lineno = 0;
+    my $error_string = '';
+    while (my $file_line = <FILE>) {
+        $lineno++;
+        $file_line =~ s/^\s*(.+)\s*$/$1/; # Remove leading and trailing whitespaces.
+        if ($file_line =~ /^[^#]+=> \$cgi\->param/) {
+            $error_string .= "$file incorrectly passes a CGI argument to a hash " .
+                             "($file_line) on line $lineno --ERROR\n";
+        }
+    }
+
+    if ($error_string) {
+        ok(0, $error_string);
+    }
+    else {
+        ok(1, "$file has no vulnerable hash syntax");
+    }

Fix on commit. r=dkl

Comment 7

[Gervase Markham [:gerv]]

Is the patch going to fix the 15 occurrences of this pattern, so that the test actually passes?

Gerv

Comment 8

[Frédéric Buclin]

yes, I'm on it

Comment 9

[Frédéric Buclin]

Attachment: patch for trunk, v2.1

Now display all errors.

Comment 10

[Frédéric Buclin]

Attachment: patch for 4.x, v1

This patch applies to all 4.x branches.

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 8498516 [details] [diff] [review]
patch for 4.x, v1

Review of attachment 8498516 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl for all supported versions

Comment 12

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   406dea6..c8f8d1e  4.0 -> 4.0

To ssh://[e-mail]/bugzilla/bugzilla.git
   ce590bf..6d6b390  4.2 -> 4.2

To ssh://[e-mail]/bugzilla/bugzilla.git
   7b8e0ab..6364067  4.4 -> 4.4

To ssh://[e-mail]/bugzilla/bugzilla.git
   9e186bd..f33b119  master -> master