Project

General

Profile

Actions

Bug #11688

closed

Disabling all interfaces associated with a floating rule causes the firewall to generate an incorrect pf rule

Added by Jonathon Reinhart over 3 years ago. Updated over 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
Rules / NAT
Target version:
Start date:
03/16/2021
Due date:
% Done:

0%

Estimated time:
Plus Target Version:
21.05
Release Notes:
Default
Affected Version:
All
Affected Architecture:
All

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
  • 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

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
  • 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!
  • From console, confirm that rule works as expected:
    • Ping some WAN IP address: OK

Notes

Actions #1

Updated by Jonathon Reinhart over 3 years ago

I opened a GitHub pull request: https://github.com/pfsense/pfsense/pull/4509

Actions #2

Updated by Jim Pingle over 3 years ago

  • Status changed from New to Pull Request Review
  • Target version set to 2.6.0
Actions #3

Updated by Jim Pingle over 3 years ago

  • Plus Target Version set to 21.05
Actions #4

Updated by Anonymous over 3 years ago

  • Status changed from Pull Request Review to Feedback
Actions #5

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.

Actions #6

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

Actions #7

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

Actions #8

Updated by Jim Pingle over 3 years ago

  • Status changed from Feedback to Closed

This is working as expected on current builds.

Actions #9

Updated by Jim Pingle over 3 years ago

  • Target version changed from 2.6.0 to 2.5.2
Actions #10

Updated by Renato Botelho over 3 years ago

  • Assignee set to Anonymous
Actions

Also available in: Atom PDF