pfSense

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.

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 # }"

This has addressed the comment line removals.

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

Thank you.

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.

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.

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

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.

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?

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.

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.

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.

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

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?

2.3.1_5 appears to have fixed it. Thank you.

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.