Hi Justin,
Justin Mason wrote:
+ no re 'taint';
+ my $username = ( $username =~ /^([-:,[EMAIL PROTECTED])$/ ? $1 : undef );
A better way to do that is to leave taint active, and instead do this:
if ($username =~ /^([-:,[EMAIL PROTECTED])$/) {
# note, separate variable for untainted value
$untainted_user = $1;
# ...use that
}
it's a minor difference, but it allows taint to stay on inside the rest
of that scope.
not sure if your concern is the "no re 'taint'" or overwriting
$username, but I agree that both isn't exactly elegant. Therefore I'm
now enabling "re 'taint'" after using the untainted $1.
Also variables were split up as suggested, although I inverted the
naming for $vpopusername + $vpopusername_tainted. IMO that's ok, as the
scope of both is limited to handle_user_vpopmail().
regarding the use of the timeout:
+ $vptimer->run(sub {
+ my $vpopusername = $username;
+ local $SIG{ALRM} = sub { die "failed to resolve vpopmail user/alias '$username'
in time ($vpoptimeout seconds)" };
+ do {
+ $vpopusername = `$vpopdir/bin/valias \Q$vpopusername\E`;
+ if ($vpopusername =~ /.+ -> &?(.+)/) {
+ $vpopusername = $1;
it's important that $SIG{ALRM} not be modified inside the timeout scope --
since the Timeout class uses it. Instead I recommend calling that die()
after the run() call, in the timed_out() action.
And I also removed the alarm 0; I forgot...
Thanks for commenting,
Cheers, Daniel
Index: spamd/spamd.raw
===================================================================
--- spamd/spamd.raw (revision 673160)
+++ spamd/spamd.raw (working copy)
@@ -2084,6 +2084,18 @@
if ($prefsfrom eq $suidto) {
$userdir = $suiddir; # reuse the already-looked-up info
+ } elsif ( $opt{'vpopmail'} ) {
+ #
+ # If vpopmail config enabled then set $userdir to virtual homedir
+ #
+ no re 'taint';
+ my $username_untainted = ( $username =~ /^([-:,[EMAIL PROTECTED])$/ ? $1 : undef );
+ use re 'taint';
+ my $vpopdir = $suiddir; # This should work with common vpopmail setups
+ $userdir = `$vpopdir/bin/vuserinfo -d \Q$username_untainted\E`;
+ if ($? != 0) {
+ $userdir = handle_user_vpopmail($username_untainted,$vpopdir);
+ }
} else {
$userdir = (getpwnam($prefsfrom))[7];
}
@@ -2105,28 +2117,44 @@
handle_user_set_user_prefs($userdir, $username);
}
-sub handle_user_set_user_prefs {
- my ($dir, $username) = @_;
+sub handle_user_vpopmail {
+ #
+ # If vuserinfo failed $username could be an alias
+ # As the alias could be an alias itself we'll try to resolve it recursively
+ # Because we're mistrusting vpopmail we'll set off an alarm
+ #
+ my $username = shift;
+ my $vpopdir = shift;
+ my $userdir;
+ my $vpoptimeout = 5;
+ my $vptimer = Mail::SpamAssassin::Timeout->new({ secs => $vpoptimeout });
- # If vpopmail config enabled then set $dir to virtual homedir
- #
- if ( $opt{'vpopmail'} ) {
- my $vpopdir = $dir;
- $dir = `$vpopdir/bin/vuserinfo -d \Q$username\E`;
- if ($? != 0) {
- #
- # If vuserinfo failed $username could be an alias
- #
- $dir = `$vpopdir/bin/valias \Q$username\E`;
- if ($? == 0 && $dir !~ /.+ -> &/) {
- $dir =~ s,.+ -> (/.+)/Maildir/,$1,;
+ $vptimer->run(sub {
+ my $vpopusername = $username;
+ do {
+ my $vpopusername_tainted = `$vpopdir/bin/valias \Q$vpopusername\E`;
+ no re 'taint';
+ if ($vpopusername_tainted =~ /.+ -> &?(.+)/) {
+ $vpopusername = $1;
} else {
- undef($dir);
+ die "failed to resolve vpopmail user/alias '$username' using vuserinfo/valias";
}
- }
- chomp($dir);
+ } until (($userdir = `$vpopdir/bin/vuserinfo -d \Q$vpopusername\E`) && $? == 0);
+ $userdir =~ s,.+ -> (/.+)/Maildir/,$1,;
+ });
+
+ if ($vptimer->timed_out()) {
+ undef $userdir;
+ die "failed to resolve vpopmail user/alias '$username' in time ($vpoptimeout seconds)";
+ } else {
+ chomp($userdir);
}
+ return $userdir;
+}
+sub handle_user_set_user_prefs {
+ my ($dir, $username) = @_;
+
# don't do this if we weren't passed a directory
if ($dir) {
my $cf_file = $dir . "/.spamassassin/user_prefs";
