From 455518faa0d2a05c1994b9c5d6f2397eb9d1db84 Mon Sep 17 00:00:00 2001
From: Ad Schellevis <ad@opnsense.org>
Date: Tue, 3 Mar 2020 17:15:57 +0100
Subject: [PATCH] firewall: add protocol validations and switch to
 NetworkAliasField type for https://github.com/opnsense/plugins/issues/1720

---
 .../app/models/OPNsense/Firewall/Filter.php   | 51 +++++++++++++++++++
 .../app/models/OPNsense/Firewall/Filter.xml   | 16 ++----
 2 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/net/firewall/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.php b/net/firewall/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.php
index ebe4b7394..51390347c 100644
--- a/net/firewall/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.php
+++ b/net/firewall/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.php
@@ -28,8 +28,59 @@
 
 namespace OPNsense\Firewall;
 
+use Phalcon\Validation\Message;
 use OPNsense\Base\BaseModel;
+use OPNsense\Firewall\Util;
 
 class Filter extends BaseModel
 {
+    /**
+     * @inheritDoc
+     */
+    public function performValidation($validateFullModel = false)
+    {
+        // standard model validations
+        $messages = parent::performValidation($validateFullModel);
+        foreach ($this->rules->rule->iterateItems() as $rule) {
+            // validate changed rules
+            $rule_changed = false;
+            foreach($rule->iterateItems() as $field) {
+                $rule_changed = $rule_changed ? $rule_changed : $field->isFieldChanged();
+            }
+            if ($validateFullModel || $rule_changed) {
+                // port / protocol validation
+                if (!empty((string)$rule->source_port) && !in_array( $rule->protocol, ['TCP', 'UDP'])) {
+                    $messages->appendMessage(new Message(
+                        gettext("Source ports are only valid for tcp or udp type rules."),
+                        $rule->source_port->__reference
+                    ));
+                }
+                if (!empty((string)$rule->destination_port) && !in_array( $rule->protocol, ['TCP', 'UDP'])) {
+                    $messages->appendMessage(new Message(
+                        gettext("Destination ports are only valid for tcp or udp type rules."),
+                        $rule->destination_port->__reference
+                    ));
+                }
+                // validate protocol family
+                $dest_is_addr = Util::isSubnet($rule->destination_net) || Util::isIpAddress($rule->destination_net);
+                $dest_proto = strpos($rule->destination_net, ':') === false ? "inet" : "inet6";
+                if ($dest_is_addr && $dest_proto != $rule->ipprotocol) {
+                    $messages->appendMessage(new Message(
+                        gettext("Destination address type should match selected TCP/IP protocol version."),
+                        $rule->destination_net->__reference
+                    ));
+                }
+                $src_is_addr = Util::isSubnet($rule->source_net) || Util::isIpAddress($rule->source_net);
+                $src_proto = strpos($rule->source_net, ':') === false ? "inet" : "inet6";
+                if ($src_is_addr && $src_proto != $rule->ipprotocol) {
+                    $messages->appendMessage(new Message(
+                        gettext("Source address type should match selected TCP/IP protocol version."),
+                        $rule->source_net->__reference
+                    ));
+                }
+
+            }
+        }
+        return $messages;
+    }
 }
\ No newline at end of file
diff --git a/net/firewall/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.xml b/net/firewall/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.xml
index ef6ff5c46..0404af5dd 100644
--- a/net/firewall/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.xml
+++ b/net/firewall/src/opnsense/mvc/app/models/OPNsense/Firewall/Filter.xml
@@ -55,11 +55,8 @@
                     <Required>Y</Required>
                     <default>any</default>
                 </protocol>
-                <!--
-                        XXX: needs a new field type to validate aliases as well
-                             should map internally to  'source' => array('network' => $source_net, "not" => true|false),
-                -->
-                <source_net type="NetworkField">
+                <!-- XXX: should map internally to  'source' => array('network' => $source_net, "not" => true|false) -->
+                <source_net type="NetworkAliasField">
                     <default>any</default>
                     <Required>Y</Required>
                 </source_net>
@@ -72,13 +69,9 @@
                     <Required>N</Required>
                     <EnableWellKnown>Y</EnableWellKnown>
                     <EnableRanges>Y</EnableRanges>
-                    <ValidationMessage>Please specify a valid port number, range or known service name</ValidationMessage>
                 </source_port>
-                <!--
-                        XXX: needs a new field type to validate aliases as well
-                             should map internally to  'source' => array('destination' => destination_net, "not" => true|false),
-                -->
-                <destination_net type="NetworkField">
+                <!-- XXX: should map internally to  'source' => array('destination' => destination_net, "not" => true|false) -->
+                <destination_net type="NetworkAliasField">
                     <default>any</default>
                     <Required>Y</Required>
                 </destination_net>
@@ -91,7 +84,6 @@
                     <Required>N</Required>
                     <EnableWellKnown>Y</EnableWellKnown>
                     <EnableRanges>Y</EnableRanges>
-                    <ValidationMessage>Please specify a valid port number, range or known service name</ValidationMessage>
                 </destination_port>
                 <gateway type="JsonKeyValueStoreField">
                     <Required>N</Required>