Project

General

Profile

Actions

Bug #6395

closed

Comments are not removed from URL Table (Ports) links

Added by Alex Vergilis over 5 years ago. Updated 3 days ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Rules / NAT
Target version:
Start date:
05/23/2016
Due date:
% Done:

100%

Estimated time:
Plus Target Version:
Release Notes:
Affected Version:
2.3.x
Affected Architecture:

Description

After upgrading from 2.2.6 on the 2.3.1 release, errors appear while loading rules, if the rule contains a "URL Table (Ports)" and the linked document contains comments.

Sample error:

There were error(s) loading the rules: /tmp/rules.debug:158: syntax error - The line in question reads [158]: pass in quick on $WAN reply-to ( igb1 192.168.1.1 ) inet proto tcp from $UrlTableHosts to any port $UrlTablePortsTcp tracker 1450580033 flags S/SA keep state label "USER_RULE" @ 2016-05-21 00:00:00

Sample URL Table (Ports) file hosted on a server:

#
# Confidential and secret
#
# Web Ports
#
80
443

Actions #1

Updated by Phillip Davis over 5 years ago

Pull request https://github.com/pfsense/pfsense/pull/2979
Can you try this?
The old editing in 2.2.* was editing out any comment lines. The new editing in 2.3.* leaves comments in the file.
Since comments do not seem to work for port tables, I have put back the old 2.2.* editing for port tables, leaving the new editing for non-port URL tables.

Actions #2

Updated by NOYB NOYB over 5 years ago

Appears alias type url table (ports) is not being loaded as a table. It doesn't show up in diag>tables and the rules.debug entry is not as a table.

Alias Type URL (Ports)
rules.debug:
zPorts_Test = "{ 80 443 }"

Alias Type URL Table (Ports)
rules.debug:
zPorts_Test = "{ # # Confidential and secret # # Web Ports # }"

Actions #3

Updated by Alex Vergilis over 5 years ago

This has addressed the comment line removals.

FYI - the "URL Table (IPs)" did not have the comment removal issue.

Thank you.

Actions #4

Updated by NOYB NOYB over 5 years ago

Phillip Davis wrote:

Pull request https://github.com/pfsense/pfsense/pull/2979
Can you try this?
The old editing in 2.2.* was editing out any comment lines. The new editing in 2.3.* leaves comments in the file.
Since comments do not seem to work for port tables, I have put back the old 2.2.* editing for port tables, leaving the new editing for non-port URL tables.

Full line comments are fine in url table files. The issue I think is that it is not being treated as a url alias table.

Actions #5

Updated by NOYB NOYB over 5 years ago

Workaround: Use alias type URL (Ports) instead of URL Table (Ports).

A better way of removing the comments than reverting the code. Is to set the kflc flag to false on line 2181:
$ports = parse_aliases_file($tmp_urltable_filename, "url_ports", "-1", true);
$ports = parse_aliases_file($tmp_urltable_filename, "url_ports", "-1", false);

But the real fix should be to treat it correctly as a table.

Actions #6

Updated by Phillip Davis over 5 years ago

Yes, the ports "table" is actually implemented as a list contained in a file, which is inserted inline into the ruleset. That is fine as a design decision - as long as everyone and the code understands what format is valid to put inline like that.

I will happily leave it to the reviewing devs to decide the best solution to this. The cause and possible remedies are all in the discussion above. NOYB's suggestion about parse_aliases_file() looks good - wish I'd noticed that :)

Actions #7

Updated by NOYB NOYB over 5 years ago

I think there may be a bigger issue here that just it works this way and not that way.
Essentially it is being handled the same way as URL (Ports). Which is basically an alias containing a list of discrete port numbers. Fine for small lists. And if that is the way it is going to be then there may as well not be a URL Table (Ports) option.

The advantage of the table is same as it is for addresses. It is better for large lists. Maybe not as much of an advantage for ports numbers but why have both options if they are both treated the same. I say either treat the URL Table (Ports) as a table like it is supposed to be or eliminate it.

Actions #8

Updated by Phillip Davis over 5 years ago

I might show my ignorance here. pf supports the table directive like:
table <spammers> persist file "/etc/spammers"
e.g. as described by http://www.openbsd.org/faq/pf/tables.html

But in all the examples, the file (and resulting table) containing IP addresses/CIDR.

Does pf support having a table that is a list of ports from a file, then using that table after the "port" keyword in a rule?

Actions #9

Updated by NOYB NOYB over 5 years ago

Well I guess we can both lay claim to ignorance.

Looking over the FreeBSD pfctl man page it seems to me that "address" is inclusive of port. Otherwise I see no means of specifying a port.

So given that, and the way rules.debug appears to substitute a variable for the table containing the addresses. My guess, and it is just a guess, is that is does.
Seems like it is just a variable pointing to a list of entries to be used. Doesn't seem like it would know the difference.

But then again... your guess is good as mine.

If it doesn't, then I don't see any point in having a URL Table (Ports) alias option.

Update:
Found this is filter.inc
case "urltable_ports":
// TODO: Change it when pf supports tables with ports
$urlfn = alias_expand_urltable($aliased['name']);
if ($urlfn) {
$aliases .= "{$aliased['name']} = \"{ " . preg_replace("/\n/", " ", file_get_contents($urlfn)) . " }\"\n";
}
break;

Modified it to use as table and it appears to load the rule. Though that doesn't necessarily mean the rule is working.
// $aliases .= "{$aliased['name']} = \"{ " . preg_replace("/\n/", " ", file_get_contents($urlfn)) . " }\"\n";
$aliases .= "table <{$aliased['name']}> persist file \"{$urlfn}\"\n";
$aliases .= "{$aliased['name']} = \"<{$aliased['name']}>\"\n";

A little hasty on that. It just took it awhile to spit out the error.

Notification:
There were error(s) loading the rules: /tmp/rules.debug:294: unknown port zPorts_Test - The line in question reads [294]: pass in quick on $WAN reply-to ( em0 192.168.2.1 ) inet proto tcp from $DNS_Verizon to any port $zPorts_Test tracker 1464056092 flags S/SA keep state label "USER_RULE: zPorts Test" 2016-05-23 22:53:20
@

rules.debug:
@table <zPorts_Test> persist file "/var/db/aliastables/zPorts_Test.txt"
zPorts_Test = "<zPorts_Test>"

pass in quick on $WAN reply-to ( em0 192.168.2.1 ) inet proto tcp from $DNS_Verizon to any port $zPorts_Test tracker 1464056092 flags S/SA keep state label "USER_RULE: zPorts Test"
@

Seems maybe we just reduced our ignorance a little bit.

Actions #10

Updated by NOYB NOYB over 5 years ago

Proposed fix.
https://github.com/pfsense/pfsense/pull/2980

Keeps full line comments of downloaded url table file, but strips them from pf load.
When/If pf supports tables with ports only this one location needs changed to take advantage while retaining the full line comments.

Actions #11

Updated by Renato Botelho over 5 years ago

  • Status changed from New to Feedback
  • % Done changed from 0 to 100

Merged. Thanks!

Actions #12

Updated by Chris Buechler over 5 years ago

  • Affected Version changed from 2.3.1 to 2.3.x
Actions #13

Updated by Alex Vergilis over 5 years ago

Phillip/Chris/Renato,

I've noticed that I had to save the URL several times before the issue went away. It seemed to behave similar to bug #5845 which was related to parsing out \r\n instead of just \n.

Just want to make sure that #5845 does not come back either.

Actions #14

Updated by NOYB NOYB over 5 years ago

That is a different issue. It has to do with the url table download and parsing. It occurs irrespective of this fix. If you look at the url table file /var/db/aliastables/ it will not be populated. Initial appearance is that the alias type (url_ports) is not being picked up on the first pass. Since it defaults to "url" it is not issue for addresses.

Looking into it.
See #6399

Actions #15

Updated by Renato Botelho over 5 years ago

Alex Vergilis wrote:

Phillip/Chris/Renato,

I've noticed that I had to save the URL several times before the issue went away. It seemed to behave similar to bug #5845 which was related to parsing out \r\n instead of just \n.

Just want to make sure that #5845 does not come back either.

Is original issue reported in this ticket fixed?

Actions #16

Updated by Alex Vergilis over 5 years ago

2.3.1_5 appears to have fixed it. Thank you.

Actions #17

Updated by Alex Vergilis over 5 years ago

FYI - I need this update to be rolled up into 2.3.2 as I cannot upgrade to 2.3.1_5 directly without breaking firewall rules and making the firewall unusable until _X patch is applied.

Actions #18

Updated by Renato Botelho over 5 years ago

  • Status changed from Feedback to Resolved
Actions

Also available in: Atom PDF