Firewall: fix issues with antilockout firewall rule #7381

Curly060 · 2024-04-14T11:09:59Z

This commit fixes two issues with the antilock firewall rule:

the rule may randomly open ports on the WAN under certain circumstances
the rule may select inactive interfaces

The issue is fixed by giving the user a choice for which interface the rule should apply for, default being LAN. The user is also being prevented from deleting/unassigning/disabling the currently selected interface.

AdSchellevis · 2024-04-14T12:22:31Z

src/etc/inc/filter.lib.inc

@@ -90,13 +90,23 @@ function filter_core_get_antilockout()
 {
    global $config;

+    // Migration from old noantilockout option


Assumes the default (new) configuration contains a fixed choice, which circles back to a static lan in config.xml.sample, when not persisting lan, initial boot after install will likely always persist lan here anyway, which might then be altered by an interface mismatch later on.

When removing the side affect write_config();, a migration also will assume a static situation, which past experiences learn has its limitations.

It's also good to remember, you're only able to cause your issue by removing existing interfaces, which almost sound like if we didn't let you remove "WAN", you didn't have an issue either..... (probably not true, but similar to the changes proposed)

Just my two cents on the subject.

Yes, that's what I wrote in the related issue of this PR: The place and the way of the migration doesn't feel right, which is due to my lack of knowledge about the internal workings of the config system. A one-time migration is most likely the better choice, but I'd need guidance as how to do that.

And it is not the removal of the WAN interface that causes trouble, it is the removal of the LAN interface (more precisely: the interface that in the config is named "lan"). Once that is gone, the opt with the lowest number is chosen:

If that opt happens to act as a WAN => antilockout rule opens ports on that WAN
If that opt happens to be disabled => antilockout rule doesn't work at all

fichtner · 2024-04-16T12:17:03Z

src/etc/inc/console.inc

@@ -384,6 +384,7 @@ EOD;
        $config['interfaces']['lan']['if'] = $lanif;
        $config['interfaces']['lan']['enable'] = true;

+        $config['system']['webgui']['antilockout_interface'] = 'lan';


as said in the issue I'd prefer a per-interface field instead of a loose list maintained somewhere else.

User view: I only use WAN and/or OPTX and I still want anti-lockout. What do I do on the console? A single WAN will also lock itself out...

fichtner · 2024-04-16T12:19:03Z

src/etc/inc/filter.lib.inc

    if (isset($config['system']['webgui']['noantilockout'])) {
-        return [];
+      unset($config['system']['webgui']['noantilockout']);


there is no reason to interfere with the state of this setting

fichtner · 2024-04-16T12:20:01Z

src/www/system_advanced_firewall.php

-        if (!empty($pconfig['noantilockout'])) {
-            $config['system']['webgui']['noantilockout'] = true;
-        } elseif (isset($config['system']['webgui']['noantilockout'])) {
-            unset($config['system']['webgui']['noantilockout']);
-        }
+        $config['system']['webgui']['antilockout_interface'] = $pconfig['antilockout_interface'];


historically these constructs exist to avoid PHP warnings and notices (check development mode)

fichtner · 2024-04-16T12:20:42Z

src/www/system_advanced_firewall.php

@@ -732,19 +728,31 @@
                  </div>
                </td>
              </tr>
-              <tr>
-                <td><a id="help_for_noantilockout" href="#" class="showhelp"><i class="fa fa-info-circle"></i></a> <?=gettext("Disable anti-lockout"); ?></td>
+              <tr id="antilockout_interface">


To be frank adding a better way but removing the old way to make it "safe" is questionable at best.

fichtner

Ok there is the thing:

Redesigning the feature and implementing it in a complimentary way is probably easier, but I see potential for hours of work and going to the drawing board to actually come up with a better system that prevents a similar amount of lockout-protection that the current system is offering.

A flaw in the system doesn't make the whole system worthless and that's why removing it will just create friction with existing users that I'm not going to debate including.

Maybe complications are unavoidable, but we better know all of them. See my "user view" questions.

fichtner · 2024-04-16T12:25:43Z

src/etc/inc/filter.lib.inc

+      unset($config['system']['webgui']['noantilockout']);
+    }
+    if (!isset($config['system']['webgui']['antilockout_interface'])) {
+      $lockout_if = get_primary_interface_from_list(array_keys(legacy_config_get_interfaces(['virtual' => false, 'enabled' => true])));


User view: Why are we adding compatibility but don't provide a way to turn this off anymore? I just want the old setting to remain and work as before.

fichtner · 2024-04-16T12:26:11Z

src/etc/inc/filter.lib.inc

+    if (!isset($config['system']['webgui']['antilockout_interface'])) {
+      $lockout_if = get_primary_interface_from_list(array_keys(legacy_config_get_interfaces(['virtual' => false, 'enabled' => true])));
+      if (empty($lockout_if)) {
+        $config['system']['webgui']['antilockout_interface'] = 'None';


Storing arbitrary values like "None" is a bad idea.

fichtner · 2024-04-16T12:27:26Z

src/opnsense/scripts/shell/setaddr.php

-    if (isset($config['system']['webgui']['noantilockout'])) {
-        unset($config['system']['webgui']['noantilockout']);
+    if ($config['system']['webgui']['antilockout_interface'] ?? 'None' != $interface) {
+        $config['system']['webgui']['antilockout_interface'] = $interface;


User view: huh, why does it reset the interface to WAN if I edit the address?

src/www/interfaces.php

fichtner · 2024-04-16T12:29:15Z

src/www/interfaces_assign.php

@@ -51,6 +51,13 @@ function link_interface_to_group($int)
    return $result;
 }

+function link_interface_to_antilockout($int)


Can we please avoid legacy glue constructs like this?

fichtner · 2024-04-16T12:30:16Z

src/www/interfaces_assign.php

@@ -155,6 +162,8 @@ interface_sync_wireless_clones($config['interfaces'][$newifname], false);
            $input_errors[] = gettext("The interface is part of a gre tunnel. Please delete the tunnel to continue");
        } elseif (link_interface_to_gif($id)) {
            $input_errors[] = gettext("The interface is part of a gif tunnel. Please delete the tunnel to continue");
+        } elseif (link_interface_to_antilockout($id)) {
+            $input_errors[] = gettext("The interface is used by the anti lockout firewall rule. Please select a different one to continue");


This is a silly constraint.

User view: ok now I need to go to some other page to set a new anti-lockout interface to be able to delete this and then I'll just forget to set anti-lockout I just want to create a new interface...

Curly060 · 2024-04-18T22:58:59Z

Ok there is the thing:

Redesigning the feature and implementing it in a complimentary way is probably easier, but I see potential for hours of work and going to the drawing board to actually come up with a better system that prevents a similar amount of lockout-protection that the current system is offering.

A flaw in the system doesn't make the whole system worthless and that's why removing it will just create friction with existing users that I'm not going to debate including.

Maybe complications are unavoidable, but we better know all of them. See my "user view" questions.

Yes, I do like this idea much better than the old way. Also, it was much easier to implement this than the old way. So I have updated (force-pushed) this PR implementing the way you proposed, see commit 5beb223.

Screenshots:
The setting:

Prevent deletion (popup is only blurry in this screenshot, not in reality):

Resetting to defaults will enable the anti lockout on the LAN interface.

One thing is missing, because I couldn't find out how to do it:
The migration of the configuration from the old logic to the new one. https://docs.opnsense.org/development/frontend/models_migrations.html do not seem to apply here. The interfaces do not have a model (or do they and I was blind?).

The old antilockout rule logic had two problems: * the rule may randomly open ports on the WAN under certain circumstances * the rule may select inactive interfaces The new logic has an antilockout setting per interface. It also prevents the deletion of interface assignments if: * the interface is locked * the anti lockout rule is active on this interface

fichtner · 2024-05-08T05:33:05Z

src/www/system_advanced_firewall.php

@@ -43,7 +43,6 @@
    $pconfig['maximumfrags'] = isset($config['system']['maximumfrags']) ? $config['system']['maximumfrags'] : null;
    $pconfig['adaptivestart'] = isset($config['system']['adaptivestart']) ? $config['system']['adaptivestart'] : null;
    $pconfig['adaptiveend'] = isset($config['system']['adaptiveend']) ? $config['system']['adaptiveend'] : null;
-    $pconfig['noantilockout'] = isset($config['system']['webgui']['noantilockout']);


Not sure if I said this earlier, but it's not a good idea to aim for a replacement at this point without a migration strategy.

Curly060 mentioned this pull request Apr 14, 2024

Anitlockout rule opens ports on WAN interface #7372

Open

2 tasks

fichtner self-assigned this Apr 14, 2024

AdSchellevis reviewed Apr 14, 2024

View reviewed changes

fichtner reviewed Apr 16, 2024

View reviewed changes

fichtner requested changes Apr 16, 2024

View reviewed changes

fichtner reviewed Apr 16, 2024

View reviewed changes

src/www/interfaces.php Show resolved Hide resolved

fichtner reviewed Apr 16, 2024

View reviewed changes

Curly060 force-pushed the fix-antilockout-rule-issues branch from 8917923 to 5beb223 Compare April 18, 2024 22:44

AdSchellevis force-pushed the master branch from 2aa15e4 to d1962f8 Compare April 30, 2024 13:02

Curly060 force-pushed the fix-antilockout-rule-issues branch from 5beb223 to 5549a52 Compare May 1, 2024 17:45

fichtner reviewed May 8, 2024

View reviewed changes

AdSchellevis force-pushed the master branch 3 times, most recently from 78845fc to 8ba454a Compare May 22, 2024 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Firewall: fix issues with antilockout firewall rule #7381

Firewall: fix issues with antilockout firewall rule #7381

Curly060 commented Apr 14, 2024

AdSchellevis Apr 14, 2024

Curly060 Apr 16, 2024

fichtner Apr 16, 2024

fichtner Apr 16, 2024

fichtner Apr 16, 2024

fichtner Apr 16, 2024

fichtner Apr 16, 2024

fichtner left a comment

fichtner Apr 16, 2024

fichtner Apr 16, 2024

fichtner Apr 16, 2024

fichtner Apr 16, 2024

fichtner Apr 16, 2024

fichtner Apr 16, 2024

Curly060 commented Apr 18, 2024

fichtner May 8, 2024

Firewall: fix issues with antilockout firewall rule #7381

Are you sure you want to change the base?

Firewall: fix issues with antilockout firewall rule #7381

Conversation

Curly060 commented Apr 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fichtner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Curly060 commented Apr 18, 2024

Choose a reason for hiding this comment