Bug 7723 - [review] FromNameSpoof warnings with missing To-header
Summary: [review] FromNameSpoof warnings with missing To-header
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Plugins (show other bugs)
Version: 3.4.2
Hardware: All All
: P2 normal
Target Milestone: 3.4.3
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-19 09:36 UTC by Henrik Krohns
Modified: 2020-11-01 18:02 UTC (History)
6 users (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 Henrik Krohns 2019-06-19 09:36:45 UTC 
Ugly warnings if message missing To: header (many legit mails in my corpus).

warn: Use of uninitialized value $toaddrs[0] in lc at Mail/SpamAssassin/Plugin/FromNameSpoof.pm line 366.
warn: Use of uninitialized value $owner in string ne at Mail/SpamAssassin/Plugin/FromNameSpoof.pm line 425.
warn: Use of uninitialized value $2 in string ne at Mail/SpamAssassin/Plugin/FromNameSpoof.pm line 425.
warn: Use of uninitialized value $owner in pattern match (m//) at Mail/SpamAssassin/Plugin/FromNameSpoof.pm line 429.

Honestly the overall code flow / state machine could use quite a bit of polishing, there's repeated _check_fromnamespoof calls doing duplicate stuff from every eval function and the function itself seems to be returning randomly from different points. I'd rather leave sorting this out to Paul. ;-)

For 3.4.3 perhaps just add below change and don't risk breaking other stuff, for RTC please vote to commit (I say +1)

  my @toaddrs = $pms->all_to_addrs();
+ return 0 unless @toaddrs;
Comment 1 Bill Cole 2019-06-19 12:36:22 UTC 
+1
Comment 2 Paul Stead 2019-06-19 12:37:41 UTC 
+1 

I'll look over other bits of logic and cleanup
Comment 3 Giovanni Bechis 2019-06-19 15:30:47 UTC 
+1 for me
Comment 4 Henrik Krohns 2019-06-19 15:44:24 UTC 
Closing this as topic is fixed, but hopefully Paul can polish things up a bit after RTC in trunk :-)

Sending        spamassassin-3.4/lib/Mail/SpamAssassin/Plugin/FromNameSpoof.pm
Sending        trunk/lib/Mail/SpamAssassin/Plugin/FromNameSpoof.pm
Transmitting file data ..done
Committing transaction...
Committed revision 1861634.
Comment 5 Kevin A. McGrail 2019-06-19 16:29:16 UTC 
+1 though will we have the review and polish from Steamdragon soon?  It's been awesome going through copious emails for bugzilla but I do want to roll rc3 and keep the momentum going.
Comment 6 Paul Stead 2019-06-19 16:45:31 UTC 
FYI, multiple checks to _check_fromnamespoof are cached with a if defined check within the function, this doesn't cause evals to be fired multiple times.
Comment 7 Henrik Krohns 2019-06-19 16:52:13 UTC 
sub _check_fromnamespoof
{
  my ($self, $pms) = @_;

  return if (defined $pms->{fromname_contains_email});

  my $conf = $pms->{conf};

  $pms->{fromname_contains_email} = 0;


Ah ok I read that too quickly, missed the "defined" bit..

But I don't think this will work reliably, since _check_fromnamespoof could be called much before the tag ever hits.

  $pms->action_depends_on_tags('DKIMDOMAIN',
      sub { my($pms,@args) = @_;
        $self->_check_fromnamespoof($pms);
Comment 8 Paul Stead 2019-06-19 17:30:43 UTC 
Hmmm, I can't determine if I can actually make this consistent? Tag blocking runs all the way to the end of the run...

Might just be best to drop the DKIM checking within FNS?
Comment 9 Henrik Krohns 2019-06-19 18:02:56 UTC 
For trunk I previously added this hook which should help.

    $plugin->check_cleanup ( { options ... } )
        Called just before message check is finishing and before possible auto-learning. This is guaranteed to be always called, unlike check_tick
        and check_post_dnsbl. Used for cleaning up left callbacks or forked children etc, last chance to make rules hit.

You can simply postpone all checks and got_hit()s until that. If indeed there is any merit in checking DKIM stuff etc. But plenty of time to think before 4.0.0..
Comment 10 Henrik Krohns 2019-06-19 18:18:48 UTC 
But probably this isn't even problem, since DKIM functions aren't async at the moment. As long as you define later priority for FNS rules than DKIM_SIGNED etc, DKIM results should be guaranteed to exist.
Comment 11 John Hardin 2020-11-01 18:02:14 UTC 
This aggressive exit skips *all* from-name spoofing checks (including the internal-to-From-header-only checks) if there is no To address. See bug 7869.