Bug 6295

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+ /

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?)

> 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).

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.

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.

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.

(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.

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.

please commit (you have privs, right?) and the mass-checks will pick that up tonight.

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.

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.

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/ ).

If it is still unallocated, I think it should be in the rules.  Perhaps create a second rule and score it less, though?

(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.

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

(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?

+1 with or without that comment

+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.

+1 to Mark's suggestion

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.

we're good for a 3.3.0 recut, right?

GO!

Bug 6295 RCVD_ILLEGAL_IP should not be eval rule
- changing the rule itself
Sending        rules/20_head_tests.cf
Committed revision 901446.

> 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.

(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.

Still testing, wait a sec...

> 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...

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.

(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?

'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!

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.

(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

moving most remaining 3.3.0 bugs to 3.3.1 milestone

> 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.