Bug #11688
closedDisabling all interfaces associated with a floating rule causes the firewall to generate an incorrect pf rule
0%
Description
TL;DR¶
If a floating rule is associated with interfaces, but none of them are enabled, the generated rule incorrectly applies to all interfaces.
To Reproduce¶
Setup¶
- Set up a basic pfSense system with WAN and LAN interfaces
- Add FW rule on WAN to allow port 80/443 for webcfg
- Add Floating rule:
- Action: Block
- Interface: LAN
- Direction: out (shouldn't matter)
- Address Family: IPv4
- Protocol: Any
- Source: any
- Destination: any
- Description: FLOATER
Test 1: Interface exists (normal behavior)¶
- Apply rules
- Observe correct rule in
/tmp/rules.debug
:block out on { vtnet1 } inet from any to any tracker 1615945995 label "USER_RULE: FLOATER"
- From console, confirm that rule works as expected:
- Ping some WAN IP address: OK
- Ping some LAN IP address:
permission denied
Test 2: Disabled interface (buggy behavior)¶
- Disable LAN interface via webcfg
- Browse to
/interfaces.php?if=lan
- Uncheck
Enable interface
- Save
- Apply Changes
- Browse to
- Observe bogus rule in
/tmp/rules.debug
:block out inet from any to any tacker 1615945995 label "USER_RULE: FLOATER"
Note that the
on { vtnet1 }
part is completely missing, causing this rule to apply to all interfaces.
- From console, observe that the rule is mistakenly being applied to WAN also:
- Ping some WAN IP address:
permission denied
- Ping some LAN IP address:
permission denied
- Ping some WAN IP address:
Cause¶
The problem is with this line: https://github.com/pfsense/pfsense/blob/v2.4.5_1/src/etc/inc/filter.inc#L2802
/* Check to see if the interface is in our list */
if (isset($rule['floating'])) {
if (isset($rule['interface']) && $rule['interface'] <> "") {
$interfaces = explode(",", $rule['interface']);
$ifliste = "";
foreach ($interfaces as $iface) {
if (array_key_exists($iface, $FilterIflist)) {
if (isset($FilterIflist[$iface]['if'])) {
$ifliste .= " " . $FilterIflist[$iface]['if'] . " ";
} else if (isset($FilterIflist[$iface][0]['if'])) {
$ifliste .= " " . $FilterIflist[$iface][0]['if'] . " ";
}
}
}
if ($ifliste <> "") {
$aline['interface'] = " on { {$ifliste} } ";
} else {
$aline['interface'] = ""; // <<<<<<<<<<<<<<<<<<<<<<<<<<<<
}
} else {
$aline['interface'] = "";
}
} else if
If none of the interfaces in $rule['interface']
are found in $FilterIflist
, then $ifliste
will be empty. Then the aforementioned line will set $aline['interface']
to an empty string.
Then at the end of the function, the rule line will be generated with the on { <interface-list> }
part missing:
https://github.com/pfsense/pfsense/blob/v2.4.5_1/src/etc/inc/filter.inc#L3220
Solution¶
The solution is simple: If all interfaces to which a floating rule is associated cannot be found, then the rule should be simply thrown away; it has no interfaces to which to apply.
diff --git a/src/etc/inc/filter.inc b/src/etc/inc/filter.inc
index cd3e26efcd..54644269bb 100644
--- a/src/etc/inc/filter.inc
+++ b/src/etc/inc/filter.inc
@@ -2799,7 +2799,12 @@ function filter_generate_user_rule($rule) {
if ($ifliste <> "") {
$aline['interface'] = " on { {$ifliste} } ";
} else {
- $aline['interface'] = "";
+ /* The rule specifies interface(s), but not one could be found.
+ * Ignore this rule rather than generating one w/o an
+ * 'on (interface)' specification, since that would incorrectly
+ * apply to all interfaces!
+ */
+ return "# rule " . $rule['descr'] . " has no matching interfaces";
}
} else {
$aline['interface'] = "";
Test 3: Disabled interface (with patch)¶
- Apply the patch to
/etc/inc/filter.inc
- Restart PHP-FPM (from pfSense console option 16) (not sure if necessary)
- Reload filter rules
- Browse to
/status_filter_reload.php
- Click
Reload Filter
- Browse to
- Observe bypassed rule in
/tmp/rules.debug
:# rule FLOATER has no matching interfaces! label "USER_RULE: FLOATER"
- Note that the
label ...
part is still concatenated, so the comment can not end with a newline!
- Note that the
- From console, confirm that rule works as expected:
- Ping some WAN IP address: OK
Notes¶
- This was discovered and tested against pfSense 2.4.5_1, but the issue still exists in latest
master
: https://github.com/pfsense/pfsense/blob/5effaab2cf6cc5485c17eb41407d1a85fd7a17d3/src/etc/inc/filter.inc#L2977
Updated by Jonathon Reinhart over 3 years ago
I opened a GitHub pull request: https://github.com/pfsense/pfsense/pull/4509
Updated by Jim Pingle over 3 years ago
- Status changed from New to Pull Request Review
- Target version set to 2.6.0
Updated by Anonymous over 3 years ago
- Status changed from Pull Request Review to Feedback
Updated by Jim Pingle over 3 years ago
- Subject changed from Disabling all interfaces associated with a floating rule causes pfSense to generate bad pf rules to Disabling all interfaces associated with a floating rule causes the firewall to generate an invalid pf rule
Updating subject for release notes.
Updated by Jonathon Reinhart over 3 years ago
Jim Pingle wrote:
causes the firewall to generate an invalid pf rule
I would argue that the generated rule is not "invalid". It is syntactically "valid", it is just not what the user intended, which is why I called it "bad".
How about this:
causes the firewall to generate an incorrect pf rule
Updated by Jim Pingle over 3 years ago
- Subject changed from Disabling all interfaces associated with a floating rule causes the firewall to generate an invalid pf rule to Disabling all interfaces associated with a floating rule causes the firewall to generate an incorrect pf rule
Either way is fine
Updated by Jim Pingle over 3 years ago
- Status changed from Feedback to Closed
This is working as expected on current builds.
Updated by Jim Pingle over 3 years ago
- Target version changed from 2.6.0 to 2.5.2