Subject: Re: Insecure dependency in eval
To: None <tech-pkg@netbsd.org>
From: Roland Illig <rillig@NetBSD.org>
List: tech-pkg
Date: 11/22/2005 21:29:18
This is a multi-part message in MIME format.
--------------030003040702050001060306
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

Roland Illig wrote:
> I'm going to prepare a good patch and put that up here for discussion.

And here it is. I don't think it is security-related since the name of 
the file indicates that it is only used for reading configuration files. 
I have already reported it upstream.

Roland

--------------030003040702050001060306
Content-Type: text/plain;
 name="patch-ba"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="patch-ba"

$NetBSD$

--- lib/Mail/SpamAssassin/Conf/Parser.pm.orig	Fri Aug 12 02:38:46 2005
+++ lib/Mail/SpamAssassin/Conf/Parser.pm	Tue Nov 22 14:41:09 2005
@@ -871,53 +871,35 @@ sub is_delimited_regexp_valid {
 
 sub is_regexp_valid {
   my ($self, $name, $re) = @_;
+  my ($saferetext);
 
   # OK, try to remove any normal perl-style regexp delimiters at
   # the start and end, and modifiers at the end if present,
   # so we can validate those too.
-  my $origre = $re;
-  my $safere = $re;
-  my $mods = '';
-  if ($re =~ s/^m{//) {
-    $re =~ s/}([a-z]*)$//; $mods = $1;
-  }
-  elsif ($re =~ s/^m\(//) {
-    $re =~ s/\)([a-z]*)$//; $mods = $1;
-  }
-  elsif ($re =~ s/^m<//) {
-    $re =~ s/>([a-z]*)$//; $mods = $1;
-  }
-  elsif ($re =~ s/^m(\W)//) {
-    $re =~ s/\Q$1\E([a-z]*)$//; $mods = $1;
-  }
-  elsif ($re =~ s/^\/(.*)\/([a-z]*)$/$1/) {
-    $mods = $2;
-  }
-  else {
-    $safere = "m#".$re."#";
-  }
-
-  # now prepend the modifiers, in order to check if they're valid
-  if ($mods) {
-    $re = "(?".$mods.")".$re;
-  }
-
-  # note: this MUST use m/...${re}.../ in some form or another, ie.
-  # interpolation of the $re variable into a code regexp, in order to test the
-  # security of the regexp.  simply using ("" =~ $re) will NOT do that, and
-  # will therefore open a hole!
-  if (eval { ("" =~ m#${re}#); 1; }) {
-
-    # now double-check -- try with the user-supplied delimiters as well
-    my $evalstr = '("" =~ '.$safere.'); 1;';
-    if (eval $evalstr) {
-      return 1;
-    }
+  if ($re =~ qr"^m(\{)(.*)\}([a-z]*)$"
+  ||  $re =~ qr"^m(\()(.*)\)([a-z]*)$"
+  ||  $re =~ qr"^m(<)(.*)>([a-z]*)$"
+  ||  $re =~ qr"^m(\W)(.*)\1([a-z]*)$"
+  ||  $re =~ qr"^(/)(.*)/([a-z]*)$") {
+    $saferetext = "(?$3)$2";
+
+  } elsif ($re =~ qr"^(.*)$") {
+    # untaint $re.
+    $saferetext = $1;
+
+  } else {
+    # cannot happen.
+    die;
+  }
+
+  my $safere;
+  if (eval { $safere = qr"${saferetext}"; 1; }) {
+    return $safere;
   }
 
   my $err = $@;
   $err =~ s/ at .*? line \d.*$//;
-  warn "config: invalid regexp for rule $name: $origre: $err\n";
+  warn "config: invalid regexp for rule $name: $re: $err\n";
   $self->{conf}->{errors}++;
   return 0;
 }

--------------030003040702050001060306--