Bug 4179 - [review] user rules are not unique to each user
Summary: [review] user rules are not unique to each user
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: All All
: P3 major
Target Milestone: 3.2.4
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard: can be commited
Keywords:
Depends on:
Blocks:
 
Reported: 2005-03-08 23:33 UTC by Daryl C. W. O'Shea
Modified: 2007-12-28 05:10 UTC (History)
2 users (show)



Attachment Type Modified Status Actions Submitter/CLA Status
test case text/plain None Justin Mason [HasCLA]
fix as patch for 3.2.x patch None Justin Mason [HasCLA]
better fix for 3.2.x patch None Justin Mason [HasCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description Daryl C. W. O'Shea 2005-03-08 23:33:31 UTC
Setting to security since it makes it possible for a user to control the scoring
of other users' mail.

If user A define's a user rule (when allow_user_rules is enabled) it affects
user B if they also set a score for that same rule name or create a user rule
with the same name.

You can see this by creating a user rule with one user and setting a score for
that rule with another.

This is a Bad Thing since a determined user could guess another user's rule
name(s) and cause all of their mail to be flagged as spam with a rule that
always matches.


I propose that user rule names be prepended (internally) by their username.
Comment 1 Theo Van Dinter 2005-03-11 05:18:29 UTC
changing component/assignee
Comment 2 Daryl C. W. O'Shea 2005-04-27 22:38:22 UTC
Easy to fix, and needs to be fixed.  I've now actually seen a system where
collisions were accidentally happening.  With a little luck, you could blacklist
all mail to other users who use commonly named custom rules.
Comment 3 David I. Lehn 2005-10-26 21:10:00 UTC
I'm seeing similar problems but with clear failures in the code due to this
problem.  I have user rules for myself setup.  Another user is getting emails
which match _my_ rules, for instance, a "full ST0CK /st0ck/i" rule.  This is
causing a failure and system log message such as:

  Use of uninitialized value in concatenation (.) or string at
/usr/share/perl5/Mail/SpamAssassin/PerMsgStatus.pm line xxxx.

This is happening in hash_line_for_rule where it fails to look up the
source_file for the rule.  With some debugging added it's trying to lookup ST0CK
which is my user_prefs but not the receipient of the emails user_prefs.  The
errors go away if rules are common between users, but that's not really a
practical fix.

Digging a bit through the code it looks like spamd is trying to restore a base
config between user switches using Spamassassin.pm copy_config() which uses
Conf.pm clone().  Is there something in that logic path or those data structures
that is shared between users that shouldn't be?
Comment 4 Daryl C. W. O'Shea 2005-10-26 23:35:10 UTC
Contrary to what I said before... it's not so easy to fix.

The problem is that user rules break copy config because they add keys that
can't be known in advance to the config hash.  So when the config is restored
any new keys caused by the addition of user rules aren't wipped out... and
that's just the config part of it.

Any code created is, AFAIK, there for the life of the child too.  I don't
believe that the generated code is backed up like the config is.

Your only work around at present is to limit the children to one connection
(using --max-conn-per-child=1).
Comment 5 Justin Mason 2005-10-27 00:40:32 UTC
If we have to keep the same $self Conf object (which imo is a bad idea for this
reason), we need to track per-user added keys like this, so they can be "rolled
back" when changing between users.
Comment 6 Daryl C. W. O'Shea 2006-07-18 23:40:17 UTC
kicking to 3.2 for now
Comment 7 Justin Mason 2006-12-12 12:40:21 UTC
moving RFEs and low-priority stuff to 3.3.0 target
Comment 8 Justin Mason 2007-10-11 14:49:11 UTC
hmm, I now think this should be 3.2.x
Comment 9 Justin Mason 2007-12-04 09:21:01 UTC
Created attachment 4197 [details]
test case

here's a test case that demos the bug.
Comment 10 Justin Mason 2007-12-04 15:54:21 UTC
fixed in trunk:

: jm 67...; svn commit -m "bug 4179: user rules are not unique to each user; one
user's user rules can appear in later scans for other users. fix"
Sending        lib/Mail/SpamAssassin/Conf.pm
Transmitting file data .
Committed revision 601154.

: jm 72...; svn commit -m "bug 4179: add test case"
Sending        MANIFEST
Adding         t/spamd_user_rules_leak.t
Transmitting file data ..
Committed revision 601155.


the fix is slightly suboptimal; instead of just recompiling the methods to
*remove* the user rule code on the next scan after the rule-defining user's
scan, all future scans with that spamd child will also recompile those methods
too.  But it works!
Comment 11 Justin Mason 2007-12-04 15:56:47 UTC
Created attachment 4199 [details]
fix as patch for 3.2.x

fix as a patch for 3.2.x
Comment 12 Justin Mason 2007-12-05 02:04:54 UTC
(In reply to comment #10)
> the fix is slightly suboptimal; instead of just recompiling the methods to
> *remove* the user rule code on the next scan after the rule-defining user's
> scan, all future scans with that spamd child will also recompile those methods
> too.  But it works!

ok, I had a thought about how to fix this last night, and it's now implemented as:

: jm 210...; svn commit -m "bug 4179: only recompile once after user rules go
'out of scope'"
Sending        lib/Mail/SpamAssassin/Conf/Parser.pm
Sending        lib/Mail/SpamAssassin/Conf.pm
Sending        lib/Mail/SpamAssassin/Plugin/Check.pm
Sending        t/spamd_user_rules_leak.t
Transmitting file data ....
Committed revision 601283.


Here's how it works: the want_rebuild_for_type 'flag' is actually a counter; it
is decremented after each scan.  This ensures that we always recompile at least
once more; once to *define* the rule, and once afterwards to *undefine* the rule
in the compiled ruleset again.

If two consecutive scans use user rules, that's ok -- the second one will
reset the counter, and we'll still recompile just once afterwards to undefine
the rule again.

(I renamed 'user_rules_to_compile' to the clearer 'want_rebuild_for_type', as
you can see)
Comment 13 Justin Mason 2007-12-05 02:05:17 UTC
Created attachment 4200 [details]
better fix for 3.2.x
Comment 14 Sidney Markowitz 2007-12-21 11:41:07 UTC
t/spamd_user_rules_leak.t is missing from the patch. It works if I copy the file
that was checked into trunk.

Other than that, and since test cases are CTR, +1 for the fix as long as
t/spamd_user_rules_leak.t is checked in with it.

Comment 15 Doc Schneider 2007-12-22 16:28:30 UTC
+1
Comment 16 Justin Mason 2007-12-28 05:10:24 UTC
thanks, fixed:

: jm 219...; svn commit -m "bug 4179: if allow_user_rules is 1, user rules are
not unique to each user; one user's user rules can appear in later scans for
other users that are run using the same spamd process. fix"
Sending        MANIFEST
Sending        lib/Mail/SpamAssassin/Conf/Parser.pm
Sending        lib/Mail/SpamAssassin/Conf.pm
Sending        lib/Mail/SpamAssassin/Plugin/Check.pm
Adding         t/spamd_user_rules_leak.t
Transmitting file data .....
Committed revision 607232.