SA Bugzilla – Bug 5798
spamd vpopmail support is broke
Last modified: 2008-07-02 13:08:59 UTC
spamd is started thusly /usr/local/bin/spamd -c -v -u vpopmail -d -r /var/run/spamd/spamd.pid This typically allows virtual users to have their own config directories but something in 3.2.4 broke how handle_user is getting its info, causing it not find find valid users. Downgrading to 3.2.3 and no other changes, causes spamd to work as expected. Also, your link to "bug writing guidelines" is broke. (Proves I made an effort to comply!)
Created attachment 4246 [details] Brief Log Snippet
Created attachment 4331 [details] Resolve vpopmail support regression The attached patch re-enables vpopmail support.
*** Bug 5868 has been marked as a duplicate of this bug. ***
*** Bug 5810 has been marked as a duplicate of this bug. ***
OK, this -- and a stack of other bugs -- is now fixed. : jm 47...; svn commit -m "bug 5798: fix vpopmail support, thanks to Daniel Albers" spamd Sending spamd/spamd.raw Transmitting file data . Committed revision 673480. Here's the back story, from the dev list.... From Daniel Albers <daniel lbers.com> Sun, Jun 15, 2008 at 1:30 AM To: dev spamassassin.apache.org -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi everyone, as you probably know, the current vpopmail implementation is broken. This is my attempt to resolve the regression bug 5798 and to shed some light on the remaining issues. The following comment by jmason can be found in bugzilla: | vpopmail note: if anyone who runs vpopmail would like to fix the | assorted bugs in spamd's support for it, and update the documentation | to be correct, please have at it. We currently have a number of bugs: | bug 2536, bug 4568, bug 4714, bug 3120 and bug 2866, all related to | vpopmail, and none of the dev team use this code, so the various | proposed fixes are untested. Basically, we need a vpopmail maintainer. Regarding the mentioned bugs, the following changes would result from this patch: 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 2866: patch: allow user config file in places other than ~/.spa... -- No change ~ - This bug has to do with vpopmail only remotely ~ - Depends on bug 5138, RFE: pluginize spamd user_prefs lookup code 3120: vpopmail user_prefs lookup bug for catch-all users -- No change ~ - The bug proposed to create user_prefs in a domain directory for catch-all accounts. This wouldn't even comply with the vpopmail design I guess, so I'd see this as a wontfix. 4568: vpopmail alias broken -- partly fixed ~ - This is similar to 3120. The remaining parts are described in bug 4717 more precisely. I'd mark it as duplicate of 3120 and 4717 if Bugzilla allows this. 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. 5798: spamd vpopmail support is broke -- fixed I don't really like vpopmail myself and I have only one server using it remaining, but I could help out with testing/resolving further issues. Regards, Daniel -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFIVGKPjG7gjygyQSURAnSzAJ42DnGzd7f7T/mL3+HWSsXZIdrL+gCeJz9y +fYFhBhVcnNk5UQwTOOpNSk= =b3R7 -----END PGP SIGNATURE----- From Justin Mason <jm jmason.org> Mon, Jun 16, 2008 at 12:56 PM To: Daniel Albers <daniel lbers.com> Cc: dev spamassassin.apache.org Thanks! This looks very promising. 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. Either way, I think I'd be happy to apply this ;) but let me know if that's possible. --j. From Daniel Albers <daniel lbers.com> Thu, Jun 19, 2008 at 10:23 PM To: dev spamassassin.apache.org Cc: Justin Mason <jm jmason.org> 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? - Can I safely untaint $username at handle_user_setuid_basic() without further checking? 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 Cheers, Daniel From Justin Mason <jm jmason.org> Fri, Jun 20, 2008 at 3:19 PM To: Daniel Albers <daniel lbers.com> Cc: dev spamassassin.apache.org, Justin Mason <jm jmason.org> 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. /^([-:,.=+A-Za-z0-9_@~]+)$/, 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. From Daniel Albers <daniel lbers.com> Mon, Jun 30, 2008 at 11:45 PM To: dev spamassassin.apache.org Hi again, Euro 2008 is over, so I finally get to do useful things again ;> The mentioned changes should be incorporated now. Functionality stays the same, so my comments on the bugs remain the same, too. Cheers, Daniel From Daniel Albers <daniel lbers.com> Tue, Jul 1, 2008 at 12:25 AM To: dev spamassassin.apache.org 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 From Justin Mason <jm jmason.org> Tue, Jul 1, 2008 at 11:05 AM To: Daniel Albers <daniel lbers.com> Cc: dev spamassassin.apache.org hi Daniel, quick comment: > + no re 'taint'; > + my $username = ( $username =~ /^([-:,.=+A-Za-z0-9_@~]+)$/ ? $1 : undef ); A better way to do that is to leave taint active, and instead do this: if ($username =~ /^([-:,.=+A-Za-z0-9_@~]+)$/) { # 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. From Daniel Albers <daniel lbers.com> Tue, Jul 1, 2008 at 9:03 PM To: Justin Mason <jm jmason.org> Cc: dev spamassassin.apache.org Hi Justin, Justin Mason wrote: + no re 'taint'; + my $username = ( $username =~ /^([-:,.=+A-Za-z0-9_@~]+)$/ ? $1 : undef ); A better way to do that is to leave taint active, and instead do this: if ($username =~ /^([-:,.=+A-Za-z0-9_@~]+)$/) { # 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
*** Bug 4714 has been marked as a duplicate of this bug. ***
fixed in 3.3.0