Bug#946829: Patch works!

[Magnus Holmgren]

torsdag 19 december 2019 kl. 11:41:21 CET skrev du:
> I can confirm that patch works as expected.
> 
> Patch does not apply cleanly on my SA (3.4.2-1~deb9u2) but only for
> cosmetic differences, attached a patch that wok on SA 3.4.2-1~deb9u2.
> 
> 
> Thanks!

I came up with the following RE loop to parse the options string. It should 
allow a few more things that admins might potentially be using, like non-
integer values (for dontgreylistthreshold), while barfing on bad syntax.

while ($optionhash =~ /(?:\G(?(?['"])(?.*?)
\g{quot1})
   \s*=>\s*
   (?>(?['"])(?.*?)\g{quot2}
  |
  (?-?(?:\d+(?:\.\d*)?|(?:\d*\.)?\d+))
   )\s*(?:;?\s*\)\s*$|;(?!$))/gxc) {
$option{$+{opt}} = $+{val};
}
if ((pos($optionhash) // 0) < length $optionhash) {
die "Syntax error";
}

I just need to add the variable untainting.

-- 
Magnus Holmgrenholmg...@debian.org
Debian Developer 

signature.asc
Description: This is a digitally signed message part.

Bug#946829: Patch works!

[Marco Gaiarin]

I can confirm that patch works as expected.

Patch does not apply cleanly on my SA (3.4.2-1~deb9u2) but only for
cosmetic differences, attached a patch that wok on SA 3.4.2-1~deb9u2.


Thanks!

-- 
dott. Marco Gaiarin GNUPG Key ID: 240A3D66
  Associazione ``La Nostra Famiglia''  http://www.lanostrafamiglia.it/
  Polo FVG   -   Via della Bontà, 7 - 33078   -   San Vito al Tagliamento (PN)
  marco.gaiarin(at)lanostrafamiglia.it   t +39-0434-842711   f +39-0434-842797

Dona il 5 PER MILLE a LA NOSTRA FAMIGLIA!
  http://www.lanostrafamiglia.it/index.php/it/sostienici/5x1000
(cf 00307430132, categoria ONLUS oppure RICERCA SANITARIA)
--- Greylisting.pm.orig	2019-12-19 11:27:35.535866138 +0100
+++ Greylisting.pm	2019-12-19 11:30:59.132703809 +0100
@@ -21,6 +21,7 @@
 
 use strict;
 use Mail::SpamAssassin::Plugin;
+use Mail::SpamAssassin::Util qw(untaint_var);
 use NetAddr::IP;
 use File::Path qw(mkpath);
 our @ISA = qw(Mail::SpamAssassin::Plugin);
@@ -71,9 +72,25 @@
 }
 Mail::SpamAssassin::Plugin::dbg("GREYLISTING: called function");
 
-$optionhash  =~ s/;/,/g;
+#$optionhash  =~ s/;/,/g;
 # This is safe, right? (users shouldn't be able to set it in their config)
-%option=eval $optionhash;
+#%option=eval $optionhash;
+
+# ... no, evaling random strings is not safe!!!
+# Ditch eval and parse hash string manually to maintain backwards compatibility
+$optionhash =~ s/^\s*\(\s*//;
+$optionhash =~ s/\s*\)\s*$//;
+foreach my $opt (split(/\s*;\s*/, $optionhash)) {
+   my @vals = split(/\s*=>\s*/, $opt, 2);
+   next unless defined $vals[1];
+   # Sanitize away quotes and any unneeded characters, then untaint
+   foreach (@vals) {
+   s/[^\w\/-]//gs;
+   $_ = untaint_var($_);
+   }
+   $option{$vals[0]} = $vals[1];
+}
+
 $self->{'rangreylisting'}=1;
 
 foreach my $reqoption (qw ( method greylistsecs dontgreylistthreshold

Bug#946829: Patch

[Henrik Krohns]

Hello,

This was really a vulnerability which allowed running any perl code or
commands (even as root), for anyone able to write .cf files/rules.

The bug is mitigated in SpamAssassin 3.4.3, which properly taints
configuration strings, and results in Perl complaining and not running
Greylisting.pm at all.

I've made a proper patch which addresses both the vulnerability and 3.4.3
compatibility.

=
--- Greylisting.pm.orig 2019-12-18 17:49:40.351383764 +0200
+++ Greylisting.pm  2019-12-18 22:30:03.745497552 +0200
@@ -21,6 +21,7 @@

 use strict;
 use Mail::SpamAssassin::Plugin;
+use Mail::SpamAssassin::Util qw(untaint_var);
 our @ISA = qw(Mail::SpamAssassin::Plugin);

 sub new
@@ -65,9 +66,25 @@

 Mail::SpamAssassin::Plugin::dbg("GREYLISTING: called function");

-$optionhash  =~ s/;/,/g;
+#$optionhash  =~ s/;/,/g;
 # This is safe, right? (users shouldn't be able to set it in their config)
-%option=eval $optionhash;
+#%option=eval $optionhash;
+
+# ... no, evaling random strings is not safe!!!
+# Ditch eval and parse hash string manually to maintain backwards 
compatibility
+$optionhash =~ s/^\s*\(\s*//;
+$optionhash =~ s/\s*\)\s*$//;
+foreach my $opt (split(/\s*;\s*/, $optionhash)) {
+   my @vals = split(/\s*=>\s*/, $opt, 2);
+   next unless defined $vals[1];
+   # Sanitize away quotes and any unneeded characters, then untaint
+   foreach (@vals) {
+   s/[^\w\/-]//gs;
+   $_ = untaint_var($_);
+   }
+   $option{$vals[0]} = $vals[1];
+}
+
 $self->{'rangreylisting'}=1;

 foreach my $reqoption (qw ( method greylistsecs dontgreylistthreshold
=

Cheers,
Henrik