Advanced Options - Multiple State / Connection Controls Not Working

ky41083 · Jul 21, 2013, 2:47 AM

Ok, so just for the record, you're on 2.0.3-RELEASE? and it's working (you're getting something anyways).

I'm on 2.1-RC0 (i386) built on Thu Jul 18 21:59:09 EDT 2013 (pfSense-2.1-RC0-2g-i386-nanobsd_vga-upgrade-20130718-2159.img.gz)

@phil.davis:

filter.inc
	$noadvoptions = false;
	if(isset($rule['statetype']) && $rule['statetype'] <> "") {
		switch($rule['statetype']) {
			case "none":
				$noadvoptions = true;
				$aline['flags'] .= " no state ";
				break;
If the state type (another advanced rule option) is set to "none" then the "Advanced Options" settings on the rule do not apply. That is the only obvious way I see that the code can bypass putting max-src-states and similar into the pf rule.

The state type on all of my rules in question (and the rest to be honest) are all set to the default value of "keep state", so by that function all of the advanced options I have set should be applied on the backend, and they are not :P

So, bug submittal time ;)

phil.davis · Jul 21, 2013, 3:35 AM

I looked further up the code block:

	if (($rule['protocol'] == "tcp") && ($type == "pass")) {
... do advanced sate-related settings

So this is only actioned for rules that are for protocol TCP.
If protocol is "all" it won't put the advanced settings in.
I guess that is reasonable, states are only TCP things, but for protocol "all" it could first write a TCP rule with the advanced settings then another generic rule for everything else (UDP, ICMP…). Or at the GUI could give an error message when saving, telling you that these advanced settings are only for protocol TCP.

ky41083 · Jul 21, 2013, 3:39 AM

Submitted to Redmine, can follow up at:

https://redmine.pfsense.org/issues/3098

ky41083 · Jul 21, 2013, 3:49 AM

@phil.davis:

I looked further up the code block:
	if (($rule['protocol'] == "tcp") && ($type == "pass")) {
... do advanced sate-related settings
So this is only actioned for rules that are for protocol TCP.
If protocol is "all" it won't put the advanced settings in.
I guess that is reasonable, states are only TCP things, but for protocol "all" it could first write a TCP rule with the advanced settings then another generic rule for everything else (UDP, ICMP…). Or at the GUI could give an error message when saving, telling you that these advanced settings are only for protocol TCP.

Ahh that's starting to make sense ;D Sweet, and I agree, it should at LEAST be notated that they will ONLY apply to TCP rules. Going to test now…

Update: These options should be applied on the backend for UDP also at the very least, and honestly for any protocol that can make a state table entry. Outgoing UDP can cripple a router, in the session table sense, in a matter of seconds…. we need more control here, and I believe pf is more than capable of this.

ky41083 · Jul 21, 2013, 4:23 AM

Yeup, that's the kicker. My outgoing rule is protocol "any", and my BitTorrent rule was TCP/UDP. Separated the BitTorrent rule out into single TCP and UDP rules, and this is what's coming back in rules.debug:

pass in quick on $WAN reply-to ( xl0 1.2.3.6 ) inet proto tcp from any to $BitTorrent_Host_IP port $BitTorrent_Port flags S/SA keep state ( max 64 max-src-nodes 64 max-src-conn 64 max-src-states 64 max-src-conn-rate 64 /5, overload <virusprot>flush global ) label "USER_RULE: NAT BitTorrent Client - TCP"

pass in quick on $WAN reply-to ( xl0 1.2.3.6 ) inet proto udp from any to $BitTorrent_Host_IP port $BitTorrent_Port label "USER_RULE: NAT BitTorrent Client - UDP"

Both rules have exactly the same settings under advanced options in the GUI, and only the TCP rule is getting any of them applied too it.</virusprot>

phil.davis · Jul 21, 2013, 4:23 AM

From http://www.freebsd.org/cgi/man.cgi?query=pf.conf&sektion=5

pf(4) will also create state for other protocols which are effectively
stateless by nature. UDP packets are matched to states using only host
addresses and ports, and other protocols are matched to states using only
the host addresses.

So at least some of the state limiting parameters can be used for UDP/ICMP also. But these ones only apply to TCP:

For stateful TCP connections, limits on established connections (connec-
tions which have completed the TCP 3-way handshake) can also be enforced
per source IP.

max-src-conn <number>Limits the maximum number of simultaneous TCP connections which
have completed the 3-way handshake that a single host can make.
max-src-conn-rate <number>/ <seconds>Limit the rate of new connections over a time interval. The con-
nection rate is an approximation calculated as a moving average.</seconds></number></number>

So it needs a bit of extra explanation and validation in the GUI to allow only the valid combinations of parameters to be entered.

ky41083 · Jul 21, 2013, 4:27 AM

@phil.davis:

From http://www.freebsd.org/cgi/man.cgi?query=pf.conf&sektion=5

pf(4) will also create state for other protocols which are effectively
stateless by nature. UDP packets are matched to states using only host
addresses and ports, and other protocols are matched to states using only
the host addresses.

So at least some of the state limiting parameters can be used for UDP/ICMP also. But these ones only apply to TCP:

For stateful TCP connections, limits on established connections (connec-
tions which have completed the TCP 3-way handshake) can also be enforced
per source IP.

max-src-conn <number>Limits the maximum number of simultaneous TCP connections which
have completed the 3-way handshake that a single host can make.
max-src-conn-rate <number>/ <seconds>Limit the rate of new connections over a time interval. The con-
nection rate is an approximation calculated as a moving average.</seconds></number></number>

So it needs a bit of extra explanation and validation in the GUI to allow only the valid combinations of parameters to be entered.

Sweet, will update bug report! So yes, we need notations and code additions because right now absolutely none of the options, even valid ones, are applied to anything but TCP rules.

ky41083 · Jul 23, 2013, 8:27 PM

I was looking more for something like this…..... (patch attached). However, with your changes on the rule edit page and these changes to filter.inc (as long as they pass the bug test and check out) I think we might have something ;D

filter.inc.patch.txt

ky41083 · Jul 24, 2013, 4:14 AM

Edit: Corrected commit request for 2.1 branch: https://github.com/ky41083/pfsense/commit/a3ff93398a5cbec0e18960be36fd2fdc24cd5aa9

Note: Yes, it's better (cleaner) than the previously attached patch, but they both do exactly the same thing in the end.

This: https://github.com/pfsense/pfsense/pull/721 - needs to be reverted or preferably just fixed (again) to match the newly supported protocols. Currently it will not let you apply advanced options to rules that now support them, except TCP only rules.

phil.davis · Jul 24, 2013, 9:00 AM

I have made a single pull request that has the code for the GUI input validation and the pf rule generation: https://github.com/pfsense/pfsense/pull/729
Review, test and comment…

ky41083 · Aug 7, 2013, 3:42 AM

Commit https://github.com/pfsense/pfsense/commit/08597fcc811eaa8299610b1e797b16abe3c7235d Line 485, "if (!empty($_POST['max']))" is overriding a preexisting function below on line 494. From (http://www.freebsd.org/cgi/man.cgi?query=pf.conf&apropos=0&sektion=5&manpath=FreeBSD+8.3-RELEASE&arch=default&format=html#STATEFUL_TRACKING_OPTIONS) that we referred too, there is no traffic prerequisite for using any stateful tracking options except "max-src-conn" and "max-src-conn-rate" which are TCP only, as you have them currently. With the newer change to the filter.inc function we were working on (https://github.com/pfsense/pfsense/commit/dde3cae3dcbd7b64757c66acc4b56f1183831ede) that brings light to this fact, shouldn't some of these validation rules also be changed to reflect this? And while we're here, remove the double occurrence of "if (!empty($_POST['max'])". The function that states the rule must simply be a pass type rule to be accepted being more correct.

I think the basis for rule validation, rather than protocol, should be state type, as described at the very top of the linked section above: "A number of options related to stateful tracking can be applied on a per-rule basis. keep state, modulate state and synproxy state support these options, and keep state must be specified explicitly to apply options to a rule." So any protocol of any state type (basically any pass rule) can use all state options, while keep state must be specified to use the two TCP only "max-src-conn" and "max-src-conn-rate" state options.

Am I making sense here?