Bug 7015 - untaint_var() broken for undefined variables
Summary: untaint_var() broken for undefined variables
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: spamc/spamd (show other bugs)
Version: 3.4.0
Hardware: PC Windows 7
: P2 normal
Target Milestone: 3.4.1
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-20 17:10 UTC by Kevin A. McGrail
Modified: 2014-02-21 19:46 UTC (History)
1 user (show)


Attachment Type Modified Status Actions Submitter/CLA Status
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin A. McGrail 2014-02-20 17:10:19 UTC 
From Tomasz Potega:

I have run into some problems trying to upgrade to 3.4.0.

The '-x' ('--nouser-config') option of spamd doesn't seem to work
correctly.

Tracing the spamd child process I can see it tries to open a user_prefs
file of the pattern (absolute path):

spamc_user_name/.spamassassin/user_prefs

As '-x' is in effect, there will be no $userdir set in
handle_user_setuid_basic().
That's OK. But handle_user_set_user_prefs() ends up called with the
user name in place of the directory name.

The cause can be traced to changes in untaint_var() - instead of
returning undef for $userdir, it just... returns, shifting the
arguments.

Shouldn't the code read more like:

--- Util.pm.orig        2014-02-07 09:36:28.000000000 +0100
+++ Util.pm     2014-02-20 14:47:09.879293333 +0100
@@ -285,7 +285,7 @@
   my $r = ref $_[0];
   if (!$r) {
     no re 'taint';  # override a  "use re 'taint'"  from outer scope
-    return if !defined $_[0];
+    return undef if !defined $_[0];
     local($1); # avoid Perl taint bug: tainted global $1 propagates
taintedness
     $_[0] =~ /^(.*)\z/s;
     return $1; 

Mark Martinec; Agreed, it's a bug. Your fix is alright.
While in most other calls it does not matter, in this case it is wrong. 

Looks like a good fix, thanks Tomasz.
Comment 1 Kevin A. McGrail 2014-02-20 18:03:02 UTC 
svn commit -m 'disabling hash rules correctly in 20_ac_rules_test.cf until we have a plugin, bug 7013 to change BAYES_99 back and make BAYES_999 an overlap rule, bug 7015 for untaint var bug thanks to Tomasz Potega'
Sending        lib/Mail/SpamAssassin/Plugin/AutoLearnThreshold.pm
Sending        lib/Mail/SpamAssassin/Util.pm
Sending        rules/23_bayes.cf
Sending        rules/30_text_de.cf
Sending        rules/30_text_fr.cf
Sending        rules/30_text_nl.cf
Sending        rules/30_text_pl.cf
Sending        rules/30_text_pt_br.cf
Sending        rules/50_scores.cf
Sending        rules/60_shortcircuit.cf
Sending        rulesrc/sandbox/kmcgrail/20_ac_rules_test.cf
Transmitting file data ...........
Committed revision 1570286.
Comment 2 Tom Schulz 2014-02-21 15:33:15 UTC 
The description and the fix do not match each other.
Comment 3 Kevin A. McGrail 2014-02-21 15:49:04 UTC 
(In reply to Tom Schulz from comment #2)
> The description and the fix do not match each other.

How so?
Comment 4 Tom Schulz 2014-02-21 18:45:29 UTC 
The description describes a problem with --nouser-config not working and gives a patch to Util.pm. The fix is not about patching Util.pm
Comment 5 Kevin A. McGrail 2014-02-21 18:50:52 UTC 
(In reply to Tom Schulz from comment #4)
> The description describes a problem with --nouser-config not working and
> gives a patch to Util.pm. The fix is not about patching Util.pm

According to the reporter, the fix is in Util.pm.  Are you able to recreate a problem with -x using svn trunk?
Comment 6 Tom Schulz 2014-02-21 19:38:58 UTC 
I am not having any problem. It was just that I read the description mentioning a proposed patch in util.pm and then read comment 1 which seemed to me to have nothing to do with that.Sorry if I am completely misunderstanding things.
Comment 7 Kevin A. McGrail 2014-02-21 19:46:47 UTC 
(In reply to Tom Schulz from comment #6)
> I am not having any problem. It was just that I read the description
> mentioning a proposed patch in util.pm and then read comment 1 which seemed
> to me to have nothing to do with that.Sorry if I am completely
> misunderstanding things.

No worries. I truly appreciate the vetting.