Hmm, I'm seeing similar occasional FPs here with numerically-named mail relays. Example:

dbg: metadata: X-Spam-Relays-Untrusted: [ ip=213.186.36.34 rdns=5.mail-out.ovh.net helo=5.mail-out.ovh.net by=extranet.tradoc.fr ident= envfrom= intl=0 id=79A4E334033 auth= msa=0 ] [ ip=213.186.33.62 rdns=a2.ovh.net helo=mozg.ha.ovh.net by=5.mail-out.ovh.net ident= envfrom= intl=0 id= auth= msa=0 ]
dbg: rules: ran header rule RCVD_ILLEGAL_IP ======> got hit: " by=5."


I'd suggest that the regex needs to ensure that it is hitting on an IP address rather than a host name that happens to start with a number and a dot.

(In reply to comment #1)
> Hmm, I'm seeing similar occasional FPs here with numerically-named mail relays.
> Example:
> 
> dbg: metadata: X-Spam-Relays-Untrusted: [ ip=213.186.36.34
> rdns=5.mail-out.ovh.net helo=5.mail-out.ovh.net by=extranet.tradoc.fr ident=
> envfrom= intl=0 id=79A4E334033 auth= msa=0 ] [ ip=213.186.33.62 rdns=a2.ovh.net
> helo=mozg.ha.ovh.net by=5.mail-out.ovh.net ident= envfrom= intl=0 id= auth=
> msa=0 ]
> dbg: rules: ran header rule RCVD_ILLEGAL_IP ======> got hit: " by=5."
> 
> 
> I'd suggest that the regex needs to ensure that it is hitting on an IP address
> rather than a host name that happens to start with a number and a dot.

That's exactly it.

We've has 395 rule hits (on corpus of 55253) over the last 39.5 hours, ALL false-positives.

Latest mass checks:

RCVD_ILLEGAL_IP:  bad, avg S/O=0.67 avg Spam%=0.11 avg Ham%=0.05

It will do badly until it is fixed...

(In reply to comment #1)
> I'd suggest that the regex needs to ensure that it is hitting on an IP address
> rather than a host name that happens to start with a number and a dot.

Inserting a positive look-ahead after the equal in the RE would do, only accepting an IP by ensuring a space as delimiter.
  /(?=\d+\.\d+\.\d+\.\d+ )/

Another approach would be, to append a trailing part to the RE, enforcing only IP style, non-hostname chars until the delimiting space.
  /[0-9.]+ /

Devs, comments, thoughts?

Went ahead with the first variant, which is more elegant IMHO. Committed to trunk, along with the fix for bug 6465. Please vote for 3.3.

Ensure RCVD_ILLEGAL_IP only matches on IPs, bug 6460.  Remove 169.254/16
assigned as "link local" block, bug 6465.

Sending        rules/20_head_tests.cf
Transmitting file data .
Committed revision 967120.

> Went ahead with the first variant, which is more elegant IMHO. Committed to
> trunk, along with the fix for bug 6465. Please vote for 3.3.
> Ensure RCVD_ILLEGAL_IP only matches on IPs, bug 6460.  Remove 169.254/16
> assigned as "link local" block, bug 6465.

+1

(In reply to comment #6)
> > Went ahead with the first variant, which is more elegant IMHO. Committed to
> > trunk, along with the fix for bug 6465. Please vote for 3.3.
> > Ensure RCVD_ILLEGAL_IP only matches on IPs, bug 6460.  Remove 169.254/16
> > assigned as "link local" block, bug 6465.
> 
> +1

This regex has gotten so complex I am wondering if a test case should be created for it?

I can't really read it fast enough anymore to know what it should and shouldn't be hitting on to vote without writing a test stub.

Regards,
KAM

> This regex has gotten so complex I am wondering if a test case should be
> created for it?

Maybe it should just be dropped or restricted to private address space,
considering that IANA Unallocated Address Pool Exhaustion is estimated
for March 2011, and considering the 'fast' deployment rate of SA in
various distributions. The next allocation blocks released are about
to happen at a steady rate, so the rule is just waiting for its next
round of false positives.

(In reply to comment #8)
> > This regex has gotten so complex I am wondering if a test case should be
> > created for it?
> 
> Maybe it should just be dropped or restricted to private address space,
> considering that IANA Unallocated Address Pool Exhaustion is estimated
> for March 2011, and considering the 'fast' deployment rate of SA in
> various distributions. The next allocation blocks released are about
> to happen at a steady rate, so the rule is just waiting for its next
> round of false positives.

+1 to restricted to private address space only

Created attachment 4828 [details]
Remove newly allocated and to-be-allocated IANA address ranges from the rule

As it turns out, even now the rule triggers on several newly-allocated
address ranges, causing false positives.

The patch strips off all already allocated and to-be-allocated
address ranges from the rule, leaving only the:
- 0.0.0.0/8 self-identification network
- multicast addresses
- all three 'test' networks

(In reply to comment #10)
> Created an attachment (id=4828) [details]
> Remove newly allocated and to-be-allocated IANA address ranges from the rule
> 
> As it turns out, even now the rule triggers on several newly-allocated
> address ranges, causing false positives.
> 
> The patch strips off all already allocated and to-be-allocated
> address ranges from the rule, leaving only the:
> - 0.0.0.0/8 self-identification network
> - multicast addresses
> - all three 'test' networks

AXB's comment was +1 for private use networks which doesn't make sense to me since theoretically those should always be trusted, yes?

Since your rule doesn't hit 192.168.X or 10.X, I think you covered what makes sense to me.

+1 KAM

trunk:
  Bug 6460: RCVD_ILLEGAL_IP false positives
Sending rules/20_head_tests.cf
Committed revision 1040826.


I keep forgetting: is the rules directory now supposed to be
common between branches? If so, I wonder why Günther's previous
fix to this rule is only seen in trunk.

> AXB's comment was +1 for private use networks which doesn't make sense to me
> since theoretically those should always be trusted, yes?

Yes, sorry, my fault, my misleading statement. The rule has nothing to do
with private networks. It covers self, multicast and test networks, and
previously also covered unallocated IANA networks, which are going away
in the following months.

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=4828) [details] [details]
> > Remove newly allocated and to-be-allocated IANA address ranges from the rule
> > 
> > As it turns out, even now the rule triggers on several newly-allocated
> > address ranges, causing false positives.
> > 
> > The patch strips off all already allocated and to-be-allocated
> > address ranges from the rule, leaving only the:
> > - 0.0.0.0/8 self-identification network
> > - multicast addresses
> > - all three 'test' networks
> 
> AXB's comment was +1 for private use networks which doesn't make sense to me
> since theoretically those should always be trusted, yes?
> 
> Since your rule doesn't hit 192.168.X or 10.X, I think you covered what makes
> sense to me.
> 
> +1 KAM

you're right, should have specified:
+1 not including non allocated IANA space in the rule.

(In reply to comment #12)
> trunk:
>   Bug 6460: RCVD_ILLEGAL_IP false positives
> Sending rules/20_head_tests.cf
> Committed revision 1040826.
> 
> I keep forgetting: is the rules directory now supposed to be
> common between branches? If so, I wonder why Günther's previous
> fix to this rule is only seen in trunk.

Apparently the 3.3 rules are now decoupled from trunk rules !?

Ending this frustration with the RCVD_ILLEGAL_IP rule misfiring
by manually backporting this rule change to 3.3:

3.3:
  Bug 6460: RCVD_ILLEGAL_IP false positives
Sending rules/20_head_tests.cf
Committed revision 1042265.

If this change ends up in 3.3 sa-updates, this bug report can be closed,
otherwise it should be investigated as soon as possible.
Bumping up priority.

What will it take to make this go into 3.3 sa-updates?

> What will it take to make this go into 3.3 sa-updates?

As far as I can tell, the fix is already in 3.3 and 3.4 updates.
Closing.  Please reopen if someone thinks otherwise.