applied! thanks ;)
https://issues.apache.org/SpamAssassin/show_bug.cgi?id=5798
Daniel Albers writes:
> 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
> part 2 text/x-patch 2782
> 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";
