Fix for bug 59349: Processmail now runs in taint (perl -T and $db->{Taint}=1) mode...

Patch by Jake Steenhagen <jake@acutex.net>
r= justdave@syndicomm.com

diff --git a/globals.pl b/globals.pl
index aa50355a05d0ff98425c771b28650b032a7cc4db..4abb4a065787165d3eafcdf3d9b9c2df8203cfa8 100644 (file)
--- a/globals.pl
+++ b/globals.pl
@@ -68,9 +68,12 @@ use DBI;
 
 use Date::Format;               # For time2str().
 use Date::Parse;               # For str2time().
-# use Carp;                       # for confess
+use Carp;                       # for confess
 use RelationSet;
 
+# $ENV{PATH} is not taint safe
+delete $ENV{PATH};
+
 # Contains the version string for the current running Bugzilla.
 $::param{'version'} = '2.13';
 
@@ -84,6 +87,13 @@ $::dbwritesallowed = 1;
 # Joe Robins, 7/5/00
 $::superusergroupset = "9223372036854775807";
 
+sub die_with_dignity {
+    my ($err_msg) = @_;
+    print $err_msg;
+    confess($err_msg);
+}
+$::SIG{__DIE__} = \&die_with_dignity;
+
 sub ConnectToDatabase {
     my ($useshadow) = (@_);
     if (!defined $::db) {
@@ -649,6 +659,12 @@ sub DBID_to_real_or_loginname {
 
 sub DBID_to_name {
     my ($id) = (@_);
+    # $id should always be a positive integer
+    if ($id =~ m/^([1-9][0-9]*)$/) {
+        $id = $1;
+    } else {
+        $::cachedNameArray{$id} = "__UNKNOWN__";
+    }
     if (!defined $::cachedNameArray{$id}) {
         PushGlobalSQLState();
         SendSQL("select login_name from profiles where userid = $id");
@@ -668,10 +684,12 @@ sub DBname_to_id {
     SendSQL("select userid from profiles where login_name = @{[SqlQuote($name)]}");
     my $r = FetchOneColumn();
     PopGlobalSQLState();
-    if (!defined $r || $r eq "") {
+    # $r should be a positive integer, this makes Taint mode happy
+    if (defined $r && $r =~ m/^([1-9][0-9]*)$/) {
+        return $1;
+    } else {
         return 0;
     }
-    return $r;
 }
 
 
@@ -699,6 +717,18 @@ sub DBNameToIdAndCheck {
     exit(0);
 }
 
+# Use detaint_string() when you know that there is no way that the data
+# in a scalar can be tainted, but taint mode still bails on it.
+# WARNING!! Using this routine on data that really could be tainted
+#           defeats the purpose of taint mode.  It should only be
+#           used on variables that cannot be touched by users.
+
+sub detaint_string {
+    my ($str) = @_;
+    $str =~ m/^(.*)$/s;
+    $str = $1;
+}
+
 # This routine quoteUrls contains inspirations from the HTML::FromText CPAN
 # module by Gareth Rees <garethr@cre.canon.co.uk>.  It has been heavily hacked,
 # all that is really recognizable from the original is bits of the regular
@@ -965,6 +995,8 @@ sub SqlQuote {
 #     }
     $str =~ s/([\\\'])/\\$1/g;
     $str =~ s/\0/\\0/g;
+    # If it's been SqlQuote()ed, then it's safe, so we tell -T that.
+    $str = detaint_string($str);
     return "'$str'";
 }
 

diff --git a/processmail b/processmail
index de0f4c7fe183f74f1130189a7fa26ed05b222e7a..0fcdbbdde50f1c6d92202eed9b80bcfc50ac5a65 100755 (executable)
--- a/processmail
+++ b/processmail
@@ -1,4 +1,4 @@
-#!/usr/bonsaitools/bin/perl -w
+#!/usr/bonsaitools/bin/perl -wT
 # -*- Mode: perl; indent-tabs-mode: nil -*-
 #
 # The contents of this file are subject to the Mozilla Public
@@ -27,11 +27,19 @@
 
 use diagnostics;
 use strict;
+use lib ".";
 
 require "globals.pl";
 
 use RelationSet;
 
+
+# Shut up misguided -w warnings about "used only once".
+sub processmail_sillyness {
+    my $zz;
+    $zz = $::db;
+}
+
 $| = 1;
 
 umask(0);
@@ -102,6 +110,10 @@ sub ProcessOneBug {
         $values{$i} = shift(@row);
     }
     my ($start, $end) = (@row);
+    # $start and $end are considered safe because users can't touch them
+    $start = detaint_string($start);
+    $end = detaint_string($end);
+
     my $ccSet = new RelationSet();
     $ccSet->mergeFromDB("SELECT who FROM cc WHERE bug_id = $id");
     $values{'cc'} = $ccSet->toString();
@@ -471,22 +483,20 @@ sub filterEmailGroup ($$$) {
 
     foreach my $person (@emailList) {
         
-        my $userid;
         my $lastCount = @filteredList;
 
         if ( $person eq '' ) { next; }
 
-        SendSQL("SELECT userid FROM profiles WHERE login_name = " 
-                . SqlQuote($person) );
+        my $userid = DBname_to_id($person);
 
-        if ( !($userid = FetchSQLData()) ) {
+        if ( ! $userid ) {
             push(@filteredList,$person);
             next;
         }
         
         SendSQL("SELECT emailflags FROM profiles WHERE " .
                 "userid = $userid" );
-        
+
         my ($userFlagString) = FetchSQLData();
         
         # If the sender doesn't want email, exclude them from list
@@ -622,6 +632,12 @@ sub NewProcessOnePerson ($$$$$$$$$$) {
       return;
     }
         
+    # Sanitize $values{'groupset'}
+    if ($values{'groupset'} =~ m/(\d+)/) {
+        $values{'groupset'} = $1;
+    } else {
+        $values{'groupset'} = 0;
+    }
     SendSQL("SELECT userid, groupset & $values{'groupset'} " .
             "FROM profiles WHERE login_name = " . SqlQuote($person));
     my ($userid, $groupset) =  (FetchSQLData());
@@ -706,6 +722,9 @@ sub NewProcessOnePerson ($$$$$$$$$$) {
 # Code starts here
 
 ConnectToDatabase();
+# Set Taint mode for the SQL
+$::db->{Taint} = 1;
+# ^^^ Taint mode is still a work in progress...
 GetVersionTable();
 
 if (open(FID, "<data/nomail")) {
@@ -762,7 +781,14 @@ if ($ARGV[0] eq "rescanall") {
         ProcessOneBug($ARGV[0]);
     }
 } else {
-    ProcessOneBug($ARGV[0]);
+    my $bugnum;
+    if ($ARGV[0] =~ m/^([1-9][0-9]*)$/) {
+        $bugnum = $1;
+    } else {
+        print "Error calling processmail (bug id is not an integer)<br>\n";
+        exit;
+    }
+    ProcessOneBug($bugnum);
 }
 
 exit;