SA Bugzilla – Bug 4179
[review] user rules are not unique to each user
Last modified: 2007-12-28 05:10:24 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.
changing component/assignee
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.
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?
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).
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.
kicking to 3.2 for now
moving RFEs and low-priority stuff to 3.3.0 target
hmm, I now think this should be 3.2.x
Created attachment 4197 [details] test case here's a test case that demos the bug.
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!
Created attachment 4199 [details] fix as patch for 3.2.x fix as a patch for 3.2.x
(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)
Created attachment 4200 [details] better fix for 3.2.x
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.
+1
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.