Bug 6295 - [review] RCVD_ILLEGAL_IP should not be eval rule
Summary: [review] RCVD_ILLEGAL_IP should not be eval rule
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Rules (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: All All
: P2 normal
Target Milestone: 3.3.1
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-19 21:49 UTC by Henrik Krohns
Modified: 2010-01-27 06:01 UTC (History)
4 users (show)



Attachment Type Modified Status Actions Submitter/CLA Status
RelayEval cleanups patch None Henrik Krohns [HasCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description Henrik Krohns 2010-01-19 21:49:46 UTC
Seems pointless to have RCVD_ILLEGAL_IP as eval rule, since it can be matched using X-Spam-Relays-Untrusted.

Proposed patch following..
Comment 1 Henrik Krohns 2010-01-19 21:59:53 UTC
Committing sandbox/hege/20_bug_6295.cf for masschecks to verify changes.

Conversion:

header T_BUG_6295_RCVD_ILLEGAL_IP X-Spam-Relays-Untrusted =~ / (?:by|ip)=(?:[0157]|22[4-9]|2[3-9]\d|[12]\d{3,}|[3-9]\d\d+)\.\d+\.\d+\.\d+ /

Also verifying Res@spamassassin-users: "It's wrong since 1/8 is now allocated"

header T_BUG_6295_RCVD_ILLEGAL_IP_1 X-Spam-Relays-Untrusted =~ / (?:by|ip)=(?:[057]|22[4-9]|2[3-9]\d|[12]\d{3,}|[3-9]\d\d+)\.\d+\.\d+\.\d+ /
Comment 2 Henrik Krohns 2010-01-19 22:08:59 UTC
Created attachment 4652 [details]
RelayEval cleanups


Proposed patch after masschecks verified.

- RCVD_ILLEGAL_IP should not be eval rule
- helo_forgery_whitelisted should be _helo_forgery_whitelisted (helper sub)
- remove obsolete sent_by_applemail (not registered? was it ever used?)
Comment 3 Mark Martinec 2010-01-20 02:20:42 UTC
> header T_BUG_6295_RCVD_ILLEGAL_IP_1 X-Spam-Relays-Untrusted =~ /
> (?:by|ip)=(?:[057]|22[4-9]|2[3-9]\d|[12]\d{3,}|[3-9]\d\d+)\.\d+\.\d+\.\d+ /

Btw, see also Bug 6237,
which at least fixed the 2.0.0.0/8 (and 223.0.0.0/8) for SA 3.3.0
(and added a concern about the viability of the approach).
Comment 4 Henrik Krohns 2010-01-20 02:34:27 UTC
I don't think there is any concern.

http://www.cymru.com/Documents/bogon-list.html

The information is out there, well updated and documented. The changelog shows rare updates. There is even a mailing list for notifications.

I can open up a separate bug for discussing updates etc after we have the rule in an actually updateable form.
Comment 5 Mark Martinec 2010-01-20 02:47:57 UTC
The 2.0.0.0/8 and 1.0.0.0/8 allocations happened closely enough, and
the 2.0.0.0/8 false positives were already reported, necessitating a fix.

The rule should be changed pretty soon now and pushed through sa-update,
before it does more damage. The rest of the change in the proposed patch
can wait for 3.3.1.

I think the proposed rules tarball for 3.3.0 may stay as it is,
people are expected to run sa-update periodically anyway.
Comment 6 Henrik Krohns 2010-01-20 03:06:29 UTC
Sure, we can go with only 20_head_tests.cf change as soon as masschecks run. It's fine to clean up RelayEval.pm in 3.3.1.
Comment 7 Mark Martinec 2010-01-20 03:13:15 UTC
(In reply to comment #6)
> Sure, we can go with only 20_head_tests.cf change as soon as masschecks run.
> It's fine to clean up RelayEval.pm in 3.3.1.

Why wait for masschecks? Semantically, nothing has changed.
The 2.0.0.0/8 and 1.0.0.0/8 were not allocated at the time of
GA runs, and now they are - the changed rule just reflects that.
Comment 8 Henrik Krohns 2010-01-20 03:30:23 UTC
I mean I just want to see that nothing strange happens during the eval->header move. It's also simple to test at the same time how the _1 change affects anything if at all.

I can't push rules anyway, so if someone wants to commit and push right now, feel free.
Comment 9 Justin Mason 2010-01-20 04:15:38 UTC
please commit (you have privs, right?) and the mass-checks will pick that up tonight.
Comment 10 Justin Mason 2010-01-20 04:22:56 UTC
to clarify, please commit as a "T_" testing rule, e.g. T_RCVD_ILLEGAL_IP_BUG6295.

doing it that way means: (a) it won't push to updates, (b) we can track the rule back to this bug, (c) we can easily compare the two rules.
Comment 11 Henrik Krohns 2010-01-20 04:28:13 UTC
Duh, in the haste I didn't get your last post and committed the actual 20_head_tests.cf.. revoked now.

As it stands in my Comment 1, I committed the test rules already. They will be in next mass checks.
Comment 12 Mark Martinec 2010-01-20 07:43:53 UTC
It might be prudent to already exclude the currently still unallocated 5.0.0.0/8,
given the high default score 3.4 of RCVD_ILLEGAL_IP, the potentially slow rules
updates at clients, and the fact that we are running out of IPv4 address space
( http://ipv6.he.net/ ).
Comment 13 Kevin A. McGrail 2010-01-20 08:02:36 UTC
If it is still unallocated, I think it should be in the rules.  Perhaps create a second rule and score it less, though?
Comment 14 Mark Martinec 2010-01-20 08:26:01 UTC
(In reply to comment #8)
> I mean I just want to see that nothing strange happens during the eval->header
> move. It's also simple to test at the same time how the _1 change affects
> anything if at all.

There probably will be some differences, as some companies (illegally) use
unallocated address ranges within their own organizations instead of using
private address ranges, and some of these leak into their mail.
Some examples: lego.com, wrigley.com.  After the change in rule, these
will no longer be false positives.
Comment 15 Mark Martinec 2010-01-20 08:30:26 UTC
If 3.3.0 re-cut is imminent, we should consider:
- at a minimum, fix the existing regexp in sub check_for_illegal_ip,
  regardless of whether the rule is changed or not;
- or, apply the whole Henrik's patch, ripping out
  the sub check_for_illegal_ip
Comment 16 Mark Martinec 2010-01-20 09:08:18 UTC
(In reply to comment #2)
> Proposed patch after masschecks verified.
> - RCVD_ILLEGAL_IP should not be eval rule
> - helo_forgery_whitelisted should be _helo_forgery_whitelisted (helper sub)
> - remove obsolete sent_by_applemail (not registered? was it ever used?)

Opening a review on Henrik's patch proposal,
suggested inclusion into 3.3.0.


Perhaps the rule could do without the following comments:
+# Bug #6295 - RCVD_ILLEGAL_IP should not be eval rule
+#
+# (note this might miss some hits if the Received.pm skips any invalid IPs)
+# do we really want to chase the more recent IANA allocations?
Comment 17 Warren Togami 2010-01-20 12:05:16 UTC
+1 with or without that comment
Comment 18 Mark Martinec 2010-01-20 12:32:05 UTC
+1 from me for the whole patch on code, assuming the rule
   will get folded-in too

Btw, if code gets updated before the rule change is propagated to
sa-update DNS and mirrors (which could take two days as we have seen),
it might be prudent to leave a dummy "check_for_illegal_ip" eval
in place, associated with a routine unconditionally returning 0,
for the benefit of the transition period.
Comment 19 Justin Mason 2010-01-20 15:16:43 UTC
+1 to Mark's suggestion
Comment 20 Mark Martinec 2010-01-20 15:39:59 UTC
Checking in Henrik's patch for code (not rule),
modified to retain a dummy "check_for_illegal_ip"
for compatibility with existing (old) rules.

Bug 6295: RCVD_ILLEGAL_IP should not be eval rule:
make sub check_for_illegal_ip a dummy always returning 0;
helo_forgery_whitelisted -> _helo_forgery_whitelisted;
ditch the nowhere used sub sent_by_applemail
  Sending        lib/Mail/SpamAssassin/Plugin/RelayEval.pm
  Committed revision 901439.



The changed rules still needs to be checked in and verified.
Comment 21 Justin Mason 2010-01-20 15:47:52 UTC
we're good for a 3.3.0 recut, right?
Comment 22 Warren Togami 2010-01-20 15:49:06 UTC
GO!
Comment 23 Mark Martinec 2010-01-20 15:55:06 UTC
Bug 6295 RCVD_ILLEGAL_IP should not be eval rule
- changing the rule itself
Sending        rules/20_head_tests.cf
Committed revision 901446.
Comment 24 Mark Martinec 2010-01-20 16:01:57 UTC
> we're good for a 3.3.0 recut, right?

I believe so. Tomorrow will need to watch the masschecks
on how the new rule behaves, but it looks alright.
Comment 25 Justin Mason 2010-01-20 16:07:53 UTC
(In reply to comment #22)
> GO!

I'm afraid I won't be doing that right now ;)  it's 12:07AM here, and if I cut the release, I'm pretty sure I'll screw something up.  feel free to go ahead and cut tarballs instead if you prefer, or I'll do them in the morning.
Comment 26 Mark Martinec 2010-01-20 16:09:11 UTC
Still testing, wait a sec...
Comment 27 Mark Martinec 2010-01-20 16:23:50 UTC
> Still testing, wait a sec...

Looks like a false alarm - I just received in short succession three messages
hitting this rule. They used a multicast IP address in one of the Received
header fields:

Received: from [238.108.46.180] (account martynov@ir-home.ru
  HELO hchiasvhprhl.cqdugocznbmh.ua)
  by bzq-109-66-199-187.red.bezeqint.net (CommuniGate Pro SMTP 5.2.3)
  with ESMTPA id 719519616 for xxx@ijs.si; Thu, 21 Jan 2010 01:58:49 +0200

Received: from [233.227.237.195] (account e1107ssd@ms21.hinet.net
  HELO cukoxonngxbb.dnmkmdkpmpklux.info)
  by dynamic.totbb.net (CommuniGate Pro SMTP 5.2.3)
  with ESMTPA id 864070145 for <xxx@ijs.si>; Thu, 21 Jan 2010 06:59:52 +0700

Received: from [238.249.38.38] (HELO ETLQVQYI)
  by 200.115.137.149 (CommuniGate Pro SMTP 5.0.11)
  with SMTP id 39661440 for xxx@ijs.si; Wed, 20 Jan 2010 17:29:41 -0300


So, nothing wrong with the rule.


> I'm afraid I won't be doing that right now ;)  it's 12:07AM here, and if I cut
> the release, I'm pretty sure I'll screw something up.

Tomorrow then...
Comment 28 Mark Martinec 2010-01-21 07:09:14 UTC
The changed rule in the 3.3.0 recut seems to work fine.

Let's just see if there are any interesting findings from the
last masscheck, then this can be closed.
Comment 29 Adam Katz 2010-01-21 07:45:13 UTC
(In reply to comment #4)
> I don't think there is any concern.
> 
> http://www.cymru.com/Documents/bogon-list.html
> 
> The information is out there, well updated and documented. The changelog shows
> rare updates. There is even a mailing list for notifications.
> 
> I can open up a separate bug for discussing updates etc after we have the rule
> in an actually updateable form.

According to that site, 223/8 was allocated to APNIC and then returned (in exchange for 060/8 in April 2003).  Verified on ARIN's lookup at https://ws.arin.net/whois?queryinput=223.0.0.1

That was 2003 ... there are a LOT of differences between our version and theirs.  I'm going to rewrite the regex and propose it here in the bug before committing it.

I assume the enforced sa-update nature will allow us to update the rule in step with the bogon updates regardless of major releases, no?
Comment 30 Justin Mason 2010-01-21 08:00:10 UTC
'I assume the enforced sa-update nature will allow us to update the rule in step
with the bogon updates regardless of major releases, no?'

yep!
Comment 31 Henrik Krohns 2010-01-21 09:53:50 UTC
Adam, please open up a new bug if you are interested in enhacing the rule. This bug is IMO only for the eval conversion.

If I had time, I would have already opened it with some cidr-to-regex script so everyone can easily update the rule if needed. There also needs to be masschecks done to every update, so probably pointless to keep this bug open more than necessary.
Comment 32 Adam Katz 2010-01-21 15:58:38 UTC
(In reply to comment #31)
> Adam, please open up a new bug if you are interested in enhacing the rule. This
> bug is IMO only for the eval conversion.

see bug 6299 and my trunk/rulesrc/sandbox/khopesh/20_bug_6299.cf
Comment 33 Justin Mason 2010-01-27 02:20:57 UTC
moving most remaining 3.3.0 bugs to 3.3.1 milestone
Comment 34 Mark Martinec 2010-01-27 02:56:25 UTC
> see bug 6299 and my trunk/rulesrc/sandbox/khopesh/20_bug_6299.cf

Now that we have a bug open for enhancing the rule,
and the eval rule has been removed in 3.3.0, this bug can be closed.