Bug 5798 - spamd vpopmail support is broke
Summary: spamd vpopmail support is broke
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: spamc/spamd (show other bugs)
Version: 3.2.4
Hardware: PC FreeBSD
: P3 normal
Target Milestone: 3.3.0
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
: 4714 5810 5868 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-01-25 13:17 UTC by Jack Scagnetti
Modified: 2008-07-02 13:08 UTC (History)
3 users (show)



Attachment Type Modified Status Actions Submitter/CLA Status
Brief Log Snippet text/plain None Jack Scagnetti [NoCLA]
Resolve vpopmail support regression patch None Daniel Albers [HasCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description Jack Scagnetti 2008-01-25 13:17:16 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!)
Comment 1 Jack Scagnetti 2008-01-25 13:18:31 UTC
Created attachment 4246 [details]
Brief Log Snippet
Comment 2 Daniel Albers 2008-06-14 17:33:12 UTC
Created attachment 4331 [details]
Resolve vpopmail support regression

The attached patch re-enables vpopmail support.
Comment 3 Aledr 2008-06-26 10:30:50 UTC
*** Bug 5868 has been marked as a duplicate of this bug. ***
Comment 4 Justin Mason 2008-07-02 09:08:52 UTC
*** Bug 5810 has been marked as a duplicate of this bug. ***
Comment 5 Justin Mason 2008-07-02 13:04:00 UTC
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

Comment 6 Justin Mason 2008-07-02 13:07:22 UTC
*** Bug 4714 has been marked as a duplicate of this bug. ***
Comment 7 Justin Mason 2008-07-02 13:08:59 UTC
fixed in 3.3.0