From b83cc529eb0e94f6bf39287e2d1d866f2a221615 Mon Sep 17 00:00:00 2001
From: Ad Schellevis <ad@opnsense.org>
Date: Tue, 22 Apr 2025 21:17:52 +0200
Subject: [PATCH] Firewall: multiselect for icmptype for both legacy and icmp.
 closes https://github.com/opnsense/core/issues/8513

---
 .../Firewall/forms/dialogFilterRule.xml       | 10 +++++++++
 .../app/models/OPNsense/Firewall/Filter.php   |  7 +++++++
 .../app/models/OPNsense/Firewall/Filter.xml   | 21 +++++++++++++++++++
 src/www/firewall_rules_edit.php               |  9 ++++----
 4 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/src/opnsense/mvc/app/controllers/OPNsense/Firewall/forms/dialogFilterRule.xml b/src/opnsense/mvc/app/controllers/OPNsense/Firewall/forms/dialogFilterRule.xml
index 838f88e4b..73bf226ae 100644
--- a/src/opnsense/mvc/app/controllers/OPNsense/Firewall/forms/dialogFilterRule.xml
+++ b/src/opnsense/mvc/app/controllers/OPNsense/Firewall/forms/dialogFilterRule.xml
@@ -168,6 +168,16 @@
             <sortable>false</sortable>
         </grid_view>
     </field>
+    <field>
+        <id>rule.icmptype</id>
+        <label>ICMP type</label>
+        <type>select_multiple</type>
+        <hint>Any</hint>
+        <advanced>true</advanced>
+        <grid_view>
+            <ignore>true</ignore>
+        </grid_view>
+    </field>
     <field>
         <id>rule.source_not</id>
         <label>Invert Source</label>
diff --git a/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.php b/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.php
index d7b48feb8..f2ad3c5ad 100644
--- a/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.php
+++ b/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.php
@@ -92,6 +92,13 @@ class Filter extends BaseModel
                         }
                     }
 
+                    if (!$rule->icmptype->isEmpty() && !in_array($rule->protocol, ['ICMP'])) {
+                        $messages->appendMessage(new Message(
+                            gettext("Option only applies to ICMP packets"),
+                            $rule->icmptype->__reference
+                        ));
+                    }
+
                     if (strpos($rule->source_net, ',') !== false && $rule->source_not == '1') {
                         $messages->appendMessage(new Message(
                             gettext("Inverting sources is only allowed for single targets to avoid mis-interpretations"),
diff --git a/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.xml b/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.xml
index 2d4a252ae..a30e242d7 100644
--- a/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.xml
+++ b/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.xml
@@ -85,6 +85,27 @@
                         <opt1 value='TCP/UDP'>TCP/UDP</opt1>
                     </AddOptions>
                 </protocol>
+                <icmptype type="OptionField">
+                    <Multiple>Y</Multiple>
+                    <OptionValues>
+                        <echoreq>Echo Request</echoreq>
+                        <echorep>Echo Reply</echorep>
+                        <unreach>Destination Unreachable</unreach>
+                        <squench>Source Quench (Deprecated)</squench>
+                        <redir>Redirect</redir>
+                        <althost>Alternate Host Address (Deprecated)</althost>
+                        <routeradv>Router Advertisement</routeradv>
+                        <routersol>Router Solicitation</routersol>
+                        <timex>Time Exceeded</timex>
+                        <paramprob>Parameter Problem</paramprob>
+                        <timereq>Timestamp</timereq>
+                        <timerep>Timestamp Reply</timerep>
+                        <inforeq>Information Request (Deprecated)</inforeq>
+                        <inforep>Information Reply (Deprecated)</inforep>
+                        <maskreq>Address Mask Request (Deprecated)</maskreq>
+                        <maskrep>Address Mask Reply (Deprecated)</maskrep>
+                    </OptionValues>
+                </icmptype>
                 <!-- XXX: should map internally to  'source' => array('network' => $source_net, "not" => true|false) -->
                 <source_net type="NetworkAliasField">
                     <Default>any</Default>
diff --git a/src/www/firewall_rules_edit.php b/src/www/firewall_rules_edit.php
index 79337b705..584757ea9 100644
--- a/src/www/firewall_rules_edit.php
+++ b/src/www/firewall_rules_edit.php
@@ -154,6 +154,7 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') {
             }
         }
         $pconfig['category'] = !empty($pconfig['category']) ? explode(",", $pconfig['category']) : [];
+        $pconfig['icmptype'] = !empty($pconfig['icmptype']) ? explode(",", $pconfig['icmptype']) : [];
 
         // process fields with some kind of logic
         address_to_pconfig(
@@ -192,6 +193,7 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') {
         }
         $pconfig['src'] = "any";
         $pconfig['dst'] = "any";
+        $pconfig['icmptype'] = [];
     }
 
     // initialize empty fields
@@ -608,7 +610,7 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') {
         }
 
         if ($pconfig['protocol'] == "icmp" && !empty($pconfig['icmptype'])) {
-            $filterent['icmptype'] = $pconfig['icmptype'];
+            $filterent['icmptype'] = implode(',', $pconfig['icmptype']);
         } elseif ($pconfig['protocol'] == 'ipv6-icmp' && !empty($pconfig['icmp6-type'])) {
             $filterent['icmp6-type'] = $pconfig['icmp6-type'];
         }
@@ -1053,10 +1055,9 @@ include("head.inc");
                   <tr id="icmpbox">
                     <td><a id="help_for_icmptype" href="#" class="showhelp"><i class="fa fa-info-circle"></i></a> <?=gettext("ICMP type");?></td>
                     <td>
-                      <select <?=!empty($pconfig['associated-rule-id']) ? "disabled" : "";?> name="icmptype" class="selectpicker" data-live-search="true" data-size="5" >
+                      <select <?=!empty($pconfig['associated-rule-id']) ? "disabled" : "";?> name="icmptype[]" class="selectpicker" title="<?=gettext("Any");?>" data-live-search="true" data-size="5" multiple="multiple">
 <?php
                       $icmptypes = array(
-                      "" => gettext("any"),
                       "echoreq" => gettext("Echo Request"),
                       "echorep" => gettext("Echo Reply"),
                       "unreach" => gettext("Destination Unreachable"),
@@ -1076,7 +1077,7 @@ include("head.inc");
                       );
 
                       foreach ($icmptypes as $icmptype => $descr): ?>
-                        <option value="<?=$icmptype;?>" <?= $icmptype == $pconfig['icmptype'] ? "selected=\"selected\"" : ""; ?>>
+                        <option value="<?=$icmptype;?>" <?= in_array($icmptype, $pconfig['icmptype']) ? "selected=\"selected\"" : ""; ?>>
                           <?=$descr;?>
                         </option>
 <?php