Bug 6803 - Add input validation to responses from DNSBL queries
Summary: Add input validation to responses from DNSBL queries
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: 3.3.2
Hardware: All All
: P2 major
Target Milestone: 3.4.0
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-08 07:34 UTC by Jeff Chan
Modified: 2013-01-17 01:22 UTC (History)
3 users (show)



Attachment Type Modified Status Actions Submitter/CLA Status
proposed change patch None Mark Martinec [HasCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff Chan 2012-06-08 07:34:44 UTC
Preliminary note: I have not researched whether this issue is already addressed in another ticket.  If it is, then please merge or close this one.

Increasingly it's a problem for many systems that their DNSBL responses are corrupted by (their providers') DNS servers which transform NXDOMAIN results into some other IP address.  Many do this to try to monetize typos or protect users from visiting harmful web sites.  However it breaks many applications' usage of DNSBLs where a common answer is NXDOMAIN for a 'not-blacklisted' result.  Many SpamAssassin systems seem to be encountering this problem, and it seems to be getting worse.

If it's not already being done, it could be highly useful for SA to validate DNSBL responses to be in CIDR 128/8.  I don't know for a fact that 128/8 would be too broad or narrow, but it could be a useful basis for research into real world behavior.  Validating DNSBL responses could prevent much of the confusion repeatedly expressed on the SA Users list, improve the correctness of SA operating behavior, and reduce the counterproductive and wasteful workload of the SA and DNSBL communities in responding to this problem.

Here is a reference.

http://www.surbl.org/faqs#dnsproxy
Comment 1 Jeff Chan 2012-06-08 07:38:15 UTC
Whoops, that should be 127/8.
Comment 2 Kevin A. McGrail 2012-06-08 12:53:47 UTC
Jeff,

This is really a duplicate of Bug 6728.  As you'll see that bug gets messy because there is an RFC for RBLs that hasn't gained any traction IMO.

However, see http://wiki.apache.org/spamassassin/DnsBlocklists and see http://wiki.apache.org/spamassassin/DnsBlocklistsInclusionPolicy.

And as noted, we have a BLOCKED response code available that even points to a page and mentions using your own DNS, etc.

Perhaps SURBL can implement the BLOCKED response code as well and join the other two RBLs for SA that have that implemented?

Regards,
KAM

*** This bug has been marked as a duplicate of bug 6728 ***
Comment 3 Jeff Chan 2012-06-08 14:55:37 UTC
SURBL is not doing blocking the way the other lists are, therefore that issue is not affecting users of SURBL data, and this issue is different from 6728.  

The biggest problem is deliberate DNS corruption mostly by provider's nameservers.  That potentially affects *all* DNSBLs.
Comment 4 Jeff Chan 2012-06-08 14:56:31 UTC
Not a duplicate.  The issue raised is entirely different.
Comment 5 Kevin A. McGrail 2012-06-08 15:01:11 UTC
(In reply to comment #4)
> Not a duplicate.  The issue raised is entirely different.

I think not.  See 6728 comment 7.  It has to do with validation outside of 127/8 which has been discussed before.

If you agree, we can reopen that ticket or at least cross-reference to this one. 

Regards,
KAM
Comment 6 John Hardin 2012-06-08 15:05:16 UTC
(In reply to comment #4)
> Not a duplicate.  The issue raised is entirely different.

Agreed.

If there is some concern about standardization of valid responses to 127/8, then allow configuration of a netblock per DNSBL site, and ignore responses outside of that site's configured valid netblock.
Comment 7 Darxus 2012-06-08 15:06:04 UTC
I think the current SURBL rules only match on the last octet:

25_uribl.cf:urirhssub       URIBL_SC_SURBL  multi.surbl.org.        A   2

This could be handled by just changing to rules to match the entire expected IP returned, similar to:

72_active.cf:header     RCVD_IN_DNSWL_HI        eval:check_rbl_sub('dnswl-firsttrusted', '^127\.0\.\d+\.3$')

Of course it's not the only option.  Throwing some alert (rule) on non-128/8 addresses would be nice.
Comment 8 AXB 2012-06-08 15:10:38 UTC
To me it makes more sense to educate ppl who have a problam than hide if from them.
If they haven't figured out the issue on their own, they'll hardly figure out the meaning of a warning - if they ever see it.
The only way that seems to get their attention is if they feel some pain.
Comment 9 Kevin A. McGrail 2012-06-08 15:12:07 UTC
(In reply to comment #8)
> To me it makes more sense to educate ppl who have a problam than hide if
> from them.
> If they haven't figured out the issue on their own, they'll hardly figure
> out the meaning of a warning - if they ever see it.
> The only way that seems to get their attention is if they feel some pain.

SA cannot promote a position where rules purposefully trigger causing inaccurate results to gain Admin attention.  I have and will fight that tooth and nail.
Comment 10 Jeff Chan 2012-06-08 15:15:01 UTC
Comment 7 of bug 6728 is arguably off topic for that bug.  The issue in this bug is different from the core purpose of the other bug which is to correctly interpret a signal of being deliberately blocked.

This bug is about unintentional DNS corruption by intermediary recursive nameservers.  The other bug is about intentional signalling of being blocked via a special response by authoritative nameservers.  These are not the same issues.
Comment 11 Jeff Chan 2012-06-08 15:16:54 UTC
(In reply to comment #7)
> I think the current SURBL rules only match on the last octet:
> 
> 25_uribl.cf:urirhssub       URIBL_SC_SURBL  multi.surbl.org.        A   2
> 
> This could be handled by just changing to rules to match the entire expected
> IP returned, similar to:
> 
> 72_active.cf:header     RCVD_IN_DNSWL_HI       
> eval:check_rbl_sub('dnswl-firsttrusted', '^127\.0\.\d+\.3$')
> 
> Of course it's not the only option.  Throwing some alert (rule) on non-128/8
> addresses would be nice.

SURBL may want to use other than the last octet.  Therefore 127/8 is better and more universal with respect to other DNSBLs some of which do use the other octets.
Comment 12 Kevin A. McGrail 2012-06-08 15:22:25 UTC
I can agree this ticket can stay open and leave the other resolved.  

I believe we need an alert rule that triggers for ANY rbl respond outside of 127/8.  Then if local admins don't like it SCORE 0 would disable.  The description of the rule will link to a page that discussing using your own DNS, etc.

I think this is the best course of action short of maintaining a list regarding every individual RBLs policy re what is a valid response.

I've ask Darxus if he can take a shot at this code.
Comment 13 Jeff Chan 2012-06-08 15:29:02 UTC
I am agnostic about what the best solution is, but I'm certain that input validation is a very good idea that is needed and will address a too common problem.
Comment 14 Mark Martinec 2012-06-27 16:02:59 UTC
Created attachment 5079 [details]
proposed change

The attached patch changes the interpretation of DNS/RBL subrules
where the subrule conditional is a single number (which is interpreted
as a bitmask). For these cases an additional check is made for the
resulting A record to fall within a 127.0.0.0/8 network range.

Affected modules: Plugin::URIDNSBL, Plugin::AskDNS, Mail::SpamAssassin::Dns

trunk (3.4):
  Bug 6803: Add input validation to responses from DNSBL queries
Sending lib/Mail/SpamAssassin/Dns.pm
Sending lib/Mail/SpamAssassin/Plugin/AskDNS.pm
Sending lib/Mail/SpamAssassin/Plugin/URIDNSBL.pm
Committed revision 1354598.
Comment 15 Kevin A. McGrail 2012-06-27 16:05:10 UTC
(In reply to comment #14)
> Created attachment 5079 [details]
> proposed change
> 
> The attached patch changes the interpretation of DNS/RBL subrules
> where the subrule conditional is a single number (which is interpreted
> as a bitmask). For these cases an additional check is made for the
> resulting A record to fall within a 127.0.0.0/8 network range.
> 
> Affected modules: Plugin::URIDNSBL, Plugin::AskDNS, Mail::SpamAssassin::Dns
> 
> trunk (3.4):
>   Bug 6803: Add input validation to responses from DNSBL queries
> Sending lib/Mail/SpamAssassin/Dns.pm
> Sending lib/Mail/SpamAssassin/Plugin/AskDNS.pm
> Sending lib/Mail/SpamAssassin/Plugin/URIDNSBL.pm
> Committed revision 1354598.

Can we trigger a default rule that links to the generic DNSBL issue page if we get an out of range answer perhaps?
Comment 16 Mark Martinec 2012-06-27 16:45:33 UTC
> Can we trigger a default rule that links to the generic DNSBL issue page
> if we get an out of range answer perhaps?

I think a rule would be needed for each zone queried,
unless we want to add some hack to the code.


The check_rbl_sub eval (DNSEval.pm) is rather simpleminded,
but does recognize a regexp, so something like '^(?!127\.)'
as a subtest could do the job.


The uridnsbl and urirhssub can take masks in various forms,
but cannot negate them, nor do they take a regexp, so it
seems a rule with an associated negated metarule would be
needed for each zone queried:

URIDNSBL.pm:
  C<subtest> is a sub-test to run against the returned data.  The sub-test may
  be in one of the following forms: m, n1-n2, or n/m, where n,n1,n2,m can be
  any of: decimal digits, 0x followed by up to 8 hexadecimal digits, or an IPv4
  address in quad-dot form. The 'A' records (IPv4 dotted address) as returned
  by DNSBLs lookups are converted into a numerical form (r) and checked against
  the specified sub-test as follows:
  for a range n1-n2 the following must be true: (r >= n1 && r <= n2);
  for a n/m form the following must be true: (r & m) == (n & m);
  for a single value in quad-dot form the following must be true: r == n;
  for a single decimal or hex form the following must be true:
    ((r & n) != 0) && ((r & 0xff000000) == 0x7f000000), i.e. within 127.0.0.0/8

so a n/m subtest could be used: 127.0.0.0/255.0.0.0
and then negated with a meta.

Similar applies to AskDNS plugin, it uses the same logic as URIDNSBL.
Comment 17 Mark Martinec 2012-06-27 18:16:35 UTC
Bug 6803: disable test X_URIBL_Y_FFD in t/dnsbl_subtests.t, no longer applies
  Sending t/dnsbl_subtests.t
Committed revision 1354654.
Comment 18 Mark Martinec 2013-01-17 01:22:59 UTC
Closing. The change brought by Comment 14 essentially avoids the problem
of modified/mangled DNS responses by additionally testing for 127.0.0.0/8.