Bug 7292 - FSL_HELO_BARE_IP_* meta rule logic is wrong
Summary: FSL_HELO_BARE_IP_* meta rule logic is wrong
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Rules (show other bugs)
Version: unspecified
Hardware: All All
: P2 normal
Target Milestone: Undefined
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-26 16:06 UTC by RW
Modified: 2018-08-22 09:33 UTC (History)
4 users (show)


Attachment Type Modified Status Actions Submitter/CLA Status
helo_bare_ip fix and rcvd_numeric removal patch None Giovanni Bechis [HasCLA]
helo_bare_ip fix and rcvd_numeric removal patch None Giovanni Bechis [HasCLA]
Show Obsolete (1) 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 RW 2016-01-26 16:06:48 UTC 
The current FSL_HELO_BARE_IP_* rules are

meta    FSL_HELO_BARE_IP_1        __FSL_HELO_BARE_IP_1 && !FSL_HELO_BARE_IP_2
meta    FSL_HELO_BARE_IP_2      __FSL_HELO_BARE_IP_2 && !__VIA_ML && !__HAS_ERRORS_TO

which means that the lower-scoring, general case suppresses the higher-scoring special case. It should be:


meta    FSL_HELO_BARE_IP_1        __FSL_HELO_BARE_IP_1  
meta    FSL_HELO_BARE_IP_2      __FSL_HELO_BARE_IP_2 && !__VIA_ML && !__HAS_ERRORS_TO && ! FSL_HELO_BARE_IP_1
Comment 1 John Hardin 2016-01-26 17:11:10 UTC 
Suppression reversed, other minor tweaks per current (rather heated) users mailing list discussion.

$ svn commit
Sending        99_doc_test.cf
Transmitting file data .
Committed revision 1726846.
Comment 2 RW 2016-01-27 21:47:52 UTC 
A couple of points:

1) Rather than using && !ALL_TRUSTED in FSL_HELO_BARE_IP_2, it would be better to only check untrusted relays in __FSL_HELO_BARE_IP_2 since this eliminates other kinds of FP as well. FSL_HELO_BARE_IP_1 may benefit from the all-trusted test, and because it's a last-external check it could also benefit from a "auth= " check. So:

meta    FSL_HELO_BARE_IP_1        __FSL_HELO_BARE_IP_1 && !ALL_TRUSTED

meta    FSL_HELO_BARE_IP_2      __FSL_HELO_BARE_IP_2 && !FSL_HELO_BARE_IP_1 &&!__VIA_ML && !__HAS_ERRORS_TO

header  __FSL_HELO_BARE_IP_1      X-Spam-Relays-External =~ /^[^\]]+ helo=(?!127)\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3} [^\]]*auth= /i

header  __FSL_HELO_BARE_IP_2    X-Spam-Relays-Untrusted =~ /helo=(?!127)\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3} /i

2) I notice that there is a comment:

  # score limit due to partial overlap with RCVD_NUMERIC_HELO

I had a look at RCVD_NUMERIC_HELO and, despite the name and possibly the intent, it's a test on HELO bare IP addresses in the untrusted networks. It's a duplicate of the modified version of __FSL_HELO_BARE_IP_2 I quoted above. I think it should go.
Comment 3 John Hardin 2016-01-27 23:05:37 UTC 
Reopening bug to address RW's further suggestions.
Comment 4 Giovanni Bechis 2018-01-01 22:05:47 UTC 
Created attachment 5508 [details]
helo_bare_ip fix and rcvd_numeric removal
Comment 5 Giovanni Bechis 2018-03-07 07:58:55 UTC 
Created attachment 5551 [details]
helo_bare_ip fix and rcvd_numeric removal

Remove score limit on FSL_HELO_BARE_IP_2 now that it doesn't overlap with RCVD_NUMERIC_HELO
Comment 6 Kevin A. McGrail 2018-08-22 03:23:34 UTC 
Giovanni, did you want to commit this?
Comment 7 Giovanni Bechis 2018-08-22 09:30:44 UTC 
Committed with commitid #1838621.
Comment 8 Giovanni Bechis 2018-08-22 09:33:28 UTC 
kmcgrail, maybe CK_HELO_DYNAMIC_SPLIT_IP in your sandbox should be reconsidered now that RCVD_NUMERIC_HELO is gone ?