Daniel Albers writes:
> Justin Mason wrote:
>
> > Would it be possible to incorporate some additional recursion? 2 levels
> > of recursion may be too little, in my opinion, and it'd just require
> > an additional "for" loop scope in the patch below to fix bug 4714 too.
>
> Okay, not a problem, here we go.
>
> First off two questions:
> - Is it okay to set alarm()s in spamd.raw or am I at risk of resetting
> other alarms?
There's a risk alright -- so use the Mail::SpamAssassin::Timeout wrapper
class, instead:
my $timer = Mail::SpamAssassin::Timeout->new({ secs => 10 });
$timer->run(sub {
# the code you want to time-limit
});
if ($timer->timed_out) {
# ...
}
Note that the code is a closure, so can safely share vars with the code
outside that scope etc., very handy ;)
> - Can I safely untaint $username at handle_user_setuid_basic() without
> further checking?
If you untaint it using a strict set of "allowed" characters, e.g.
/^([-:,[EMAIL PROTECTED])$/, I can't see a problem there. (I know the use
of \Q...\E will take care of safe quoting in the valias etc. command
lines, but it's better to be safe than sorry and apply multiple levels of
defence).
> These are the updated changes:
>
> > Daniel Albers writes:
> >> 2536: vpopmail/qmail code neither warning- nor 100% taint-safe -- No change
> >> ~ - The proposed change is far too extensive to be included in spamd
> >> imho. I would rather switch to a VPopmail implementation from CPAN. ->
> >> wontfix
>
> Well, the former implementation would have also raised taint mode
> warnings, but it was never executed in handle_user_set_user_prefs() I guess.
> The vpopmail part is taint-safe now, but that's mainly because of
> unrelated changes that happened in between the bug report and now.
>
> >> 4714: Vpopmail handling of aliases fails -- partly fixed
> >> ~ - Resolving vpopmail aliases is supported with the attached patch,
> >> although it doesn't do recursion, but only one additional try. Again I
> >> don't think this should be done in spamd or spamassassin codebase at all.
>
> The patch now attempts to recursively resolve aliases. It doesn't
> iterate any certain number of times, but tries for $vpoptimeout = 5
> seconds, though.
> This is meant to deal with situations where
> a) vpopmail commands have become slow/unresponsive, which could happen
> due to various issues, like database backend issues
> b) a larger number of aliases were chained
Cool, sounds great ;)
One trivial refactor: could you move the new code into its own function,
and simply call that from handle_user_setuid_basic()? it's just better
practice to keep function bodies short. -- not that the rest of spamd
is particularly well coded in that respect :(
it looks good btw...
--j.
> Cheers, Daniel
>
> Index: spamd/spamd.raw
> ===================================================================
> --- spamd/spamd.raw (revision 669697)
> +++ spamd/spamd.raw (working copy)
> @@ -2084,6 +2084,39 @@
>
> 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
> + #
> + untaint_var(\$username);
> + my $vpopdir = $suiddir; # This should work with common vpopmail setups
> + $userdir = `$vpopdir/bin/vuserinfo -d \Q$username\E`;
> + if ($? != 0) {
> + #
> + # 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
> + #
> + eval {
> + my $vpoptimeout = 5;
> + my $vpopusername = $username;
> + local $SIG{ALRM} = sub { die "in time ($vpoptimeout seconds)" };
> + alarm $vpoptimeout;
> + do {
> + $vpopusername = `$vpopdir/bin/valias \Q$vpopusername\E`;
> + $vpopusername =~ s/.+ -> &?(.+)/$1/
> + or die "using vuserinfo/valias";
> + untaint_var(\$vpopusername);
> + } until (($userdir = `$vpopdir/bin/vuserinfo -d
> \Q$vpopusername\E`) && $? == 0);
> + alarm 0;
> + $userdir =~ s,.+ -> (/.+)/Maildir/,$1,;
> + };
> + if ($@) {
> + info("spamd: failed to resolve vpopmail user/alias '$username'
> [EMAIL PROTECTED]");
> + undef $userdir;
> + }
> + }
> + chomp($userdir) if defined($userdir);
> } else {
> $userdir = (getpwnam($prefsfrom))[7];
> }
> @@ -2108,25 +2141,6 @@
> sub handle_user_set_user_prefs {
> my ($dir, $username) = @_;
>
> - # 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,;
> - } else {
> - undef($dir);
> - }
> - }
> - chomp($dir);
> - }
> -
> # don't do this if we weren't passed a directory
> if ($dir) {
> my $cf_file = $dir . "/.spamassassin/user_prefs";
