hi Daniel, quick comment:
> + 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.
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.
--j.
Daniel Albers writes:
> Daniel Albers wrote:
>
> > Euro 2008 is over, so I finally get to do useful things again ;>
>
> Ah, well, maybe I'm not back to full speed yet. The second untaint is
> now done properly as well.
>
> Cheers, Daniel
> part 2 text/x-patch 2747
> Index: spamd/spamd.raw
> ===================================================================
> --- spamd/spamd.raw (revision 672847)
> +++ spamd/spamd.raw (working copy)
> @@ -2084,6 +2084,17 @@
>
> 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 = ( $username =~ /^([-:,[EMAIL PROTECTED])$/ ? $1 : undef
> );
> + my $vpopdir = $suiddir; # This should work with common vpopmail setups
> + $userdir = `$vpopdir/bin/vuserinfo -d \Q$username\E`;
> + if ($? != 0) {
> + $userdir = handle_user_vpopmail($username,$vpopdir);
> + }
> } else {
> $userdir = (getpwnam($prefsfrom))[7];
> }
> @@ -2105,28 +2116,45 @@
> 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
> + #
> + no re 'taint';
> + 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;
> + 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;
> } 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);
> + alarm 0;
> + $userdir =~ s,.+ -> (/.+)/Maildir/,$1,;
> + });
> +
> + if ($vptimer->timed_out()) {
> + undef $userdir;
> + } 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";
