From 500f583551e32a76ea9aa20b4e60e720c93d33fd Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 8 Oct 2026 10:15:23 +0000 Subject: [PATCH 1/3] Validate a network ACL protocol name against the list, not the string validateProtocol() checked a protocol name with "tcp,udp,icmp,all".contains(protocol), a substring test, so "cp", "t", "dp,ic", "," or "tcp,udp" were accepted. Such a rule is stored and listed but never applied: the VR side can't parse it and drops it with only a warning. Compare against the entries of the list instead. Fixes: #14358 Signed-off-by: Brad House --- .../cloud/network/vpc/NetworkACLServiceImpl.java | 4 +++- .../network/vpc/NetworkACLServiceImplTest.java | 13 +++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java index 94e79ec6ea75..cf5ee36c5f89 100644 --- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java @@ -17,6 +17,7 @@ package com.cloud.network.vpc; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Comparator; import java.util.List; @@ -619,7 +620,8 @@ protected void validateProtocol(NetworkACLItemVO networkACLItemVO) { throw new InvalidParameterValueException("Invalid protocol number: " + protoNumber); } } else { - if (!supportedProtocolsForAclRules.contains(protocol.toLowerCase())) { + // one of the names, not just part of the string: "cp" or "p,ic" must not pass + if (!Arrays.asList(supportedProtocolsForAclRules.split(",")).contains(protocol.toLowerCase(Locale.ROOT))) { throw new InvalidParameterValueException(String.format("Invalid protocol [%s]. Expected one of: [%s]", protocol, supportedProtocolsForAclRules)); } } diff --git a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java index b24136972ad8..5997e949e440 100644 --- a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java +++ b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java @@ -612,6 +612,19 @@ public void validateProtocolTestProtocolIsStringInvalid() { networkAclServiceImpl.validateProtocol(networkAclItemVoMock); } + @Test + public void validateProtocolTestProtocolIsPartOfTheSupportedList() { + for (String protocol : new String[] {"cp", "t", "dp,ic", ",", "tcp,udp"}) { + Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn(protocol); + try { + networkAclServiceImpl.validateProtocol(networkAclItemVoMock); + Assert.fail("protocol [" + protocol + "] should have been rejected"); + } catch (InvalidParameterValueException e) { + // expected + } + } + } + @Test public void validateProtocolTestProtocolIsStringValid() { Mockito.doNothing().when(networkAclServiceImpl).validateIcmpTypeAndCode(networkAclItemVoMock); From c9c49d883617f00e8028f891fff1fa84e08c092d Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 8 Oct 2026 10:56:43 +0000 Subject: [PATCH 2/3] Cover protocol names in any case, and match the comment to the test Signed-off-by: Brad House --- .../cloud/network/vpc/NetworkACLServiceImpl.java | 2 +- .../network/vpc/NetworkACLServiceImplTest.java | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java index cf5ee36c5f89..4ed784be1997 100644 --- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java @@ -620,7 +620,7 @@ protected void validateProtocol(NetworkACLItemVO networkACLItemVO) { throw new InvalidParameterValueException("Invalid protocol number: " + protoNumber); } } else { - // one of the names, not just part of the string: "cp" or "p,ic" must not pass + // one of the names, not just part of the string: "cp" or "dp,ic" must not pass if (!Arrays.asList(supportedProtocolsForAclRules.split(",")).contains(protocol.toLowerCase(Locale.ROOT))) { throw new InvalidParameterValueException(String.format("Invalid protocol [%s]. Expected one of: [%s]", protocol, supportedProtocolsForAclRules)); } diff --git a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java index 5997e949e440..9ac76db95708 100644 --- a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java +++ b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java @@ -625,6 +625,19 @@ public void validateProtocolTestProtocolIsPartOfTheSupportedList() { } } + @Test + public void validateProtocolTestProtocolNameInAnyCase() { + Mockito.doNothing().when(networkAclServiceImpl).validateIcmpTypeAndCode(networkAclItemVoMock); + Mockito.when(networkAclItemVoMock.getIcmpCode()).thenReturn(null); + Mockito.when(networkAclItemVoMock.getIcmpType()).thenReturn(null); + Mockito.when(networkAclItemVoMock.getSourcePortStart()).thenReturn(null); + Mockito.when(networkAclItemVoMock.getSourcePortEnd()).thenReturn(null); + for (String protocol : new String[] {"TCP", "Udp", "ICMP", "All"}) { + Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn(protocol); + networkAclServiceImpl.validateProtocol(networkAclItemVoMock); + } + } + @Test public void validateProtocolTestProtocolIsStringValid() { Mockito.doNothing().when(networkAclServiceImpl).validateIcmpTypeAndCode(networkAclItemVoMock); From fa1632dbee8a30dcab3b12311f352e4fa0ba73a7 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 8 Oct 2026 12:20:40 +0000 Subject: [PATCH 3/3] Validate firewall and routed rule protocols against the list too The firewall (port forwarding, static NAT, firewall), IPv6 firewall and routing firewall rule checks test the protocol against the provider's supported protocols with the same substring test, so "cp" or "dp,ic" passed them as well. Add NetUtils.isProtocolInList(), which matches a whole entry of the comma-separated list in any case, trimming the entries since some providers list them with a space after the comma ("tcp,udp, icmp, all"), and use it for these checks and the network ACL one. Signed-off-by: Brad House --- .../com/cloud/network/Ipv6ServiceImpl.java | 2 +- .../network/firewall/FirewallManagerImpl.java | 2 +- .../network/vpc/NetworkACLServiceImpl.java | 3 +-- .../network/RoutedIpv4ManagerImpl.java | 2 +- .../java/com/cloud/utils/net/NetUtils.java | 19 +++++++++++++++++++ .../com/cloud/utils/net/NetUtilsTest.java | 15 +++++++++++++++ 6 files changed, 38 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/com/cloud/network/Ipv6ServiceImpl.java b/server/src/main/java/com/cloud/network/Ipv6ServiceImpl.java index 464f8c90ebb6..410b252fd8e4 100644 --- a/server/src/main/java/com/cloud/network/Ipv6ServiceImpl.java +++ b/server/src/main/java/com/cloud/network/Ipv6ServiceImpl.java @@ -583,7 +583,7 @@ public FirewallRule createIpv6FirewallRule(CreateIpv6FirewallRuleCmd cmd) throws supportedProtocols = caps.get(Network.Capability.SupportedProtocols).toLowerCase(); } - if (!supportedProtocols.contains(protocol.toLowerCase())) { + if (!NetUtils.isProtocolInList(supportedProtocols, protocol)) { throw new InvalidParameterValueException(String.format("Protocol %s is not supported in zone", protocol)); } else if (!supportedTrafficTypes.contains(trafficType.toString().toLowerCase())) { throw new InvalidParameterValueException(String.format("Traffic Type %s is currently supported by Firewall in network %s", trafficType, network)); diff --git a/server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java b/server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java index 4aee5fef48a6..65837f308ae2 100644 --- a/server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java +++ b/server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java @@ -603,7 +603,7 @@ public void validateFirewallRule(Account caller, IPAddressVO ipAddress, Integer supportedProtocols = caps.get(Capability.SupportedProtocols).toLowerCase(); } - if (!supportedProtocols.contains(proto.toLowerCase())) { + if (!NetUtils.isProtocolInList(supportedProtocols, proto)) { throw new InvalidParameterValueException("Protocol " + proto + " is not supported in zone " + network.getDataCenterId()); } else if (proto.equalsIgnoreCase(NetUtils.ICMP_PROTO) && purpose != Purpose.Firewall) { throw new InvalidParameterValueException("Protocol " + proto + " is currently supported only for rules with purpose " + Purpose.Firewall); diff --git a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java index 4ed784be1997..b6baf1fcf85f 100644 --- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java @@ -17,7 +17,6 @@ package com.cloud.network.vpc; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.Comparator; import java.util.List; @@ -621,7 +620,7 @@ protected void validateProtocol(NetworkACLItemVO networkACLItemVO) { } } else { // one of the names, not just part of the string: "cp" or "dp,ic" must not pass - if (!Arrays.asList(supportedProtocolsForAclRules.split(",")).contains(protocol.toLowerCase(Locale.ROOT))) { + if (!NetUtils.isProtocolInList(supportedProtocolsForAclRules, protocol)) { throw new InvalidParameterValueException(String.format("Invalid protocol [%s]. Expected one of: [%s]", protocol, supportedProtocolsForAclRules)); } } diff --git a/server/src/main/java/org/apache/cloudstack/network/RoutedIpv4ManagerImpl.java b/server/src/main/java/org/apache/cloudstack/network/RoutedIpv4ManagerImpl.java index ee888a61adc5..0dede6b0471c 100644 --- a/server/src/main/java/org/apache/cloudstack/network/RoutedIpv4ManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/network/RoutedIpv4ManagerImpl.java @@ -893,7 +893,7 @@ public FirewallRule createRoutingFirewallRule(CreateRoutingFirewallRuleCmd creat supportedProtocols = caps.get(Network.Capability.SupportedProtocols).toLowerCase(); } - if (!supportedProtocols.contains(protocol.toLowerCase())) { + if (!NetUtils.isProtocolInList(supportedProtocols, protocol)) { throw new InvalidParameterValueException(String.format("Protocol %s is not supported in zone", protocol)); } else if (!supportedTrafficTypes.contains(trafficType.toString().toLowerCase())) { throw new InvalidParameterValueException("Traffic Type " + trafficType + " is currently supported by Firewall in network " + networkId); diff --git a/utils/src/main/java/com/cloud/utils/net/NetUtils.java b/utils/src/main/java/com/cloud/utils/net/NetUtils.java index d89d9fa2d93c..33f9f0e362ed 100644 --- a/utils/src/main/java/com/cloud/utils/net/NetUtils.java +++ b/utils/src/main/java/com/cloud/utils/net/NetUtils.java @@ -38,6 +38,7 @@ import java.util.Formatter; import java.util.Iterator; import java.util.List; +import java.util.Locale; import java.util.Random; import java.util.Set; import java.util.SortedSet; @@ -991,6 +992,24 @@ public static long getCidrSize(final String netmask) { return MAX_CIDR - count; } + /** + * Whether the protocol is one of the comma-separated protocols, as a whole entry and in any case. + * The lists come from provider capabilities, some of which have spaces after their commas; the + * protocol itself is taken as given. + */ + public static boolean isProtocolInList(final String protocols, final String protocol) { + if (protocols == null || protocol == null) { + return false; + } + final String p = protocol.toLowerCase(Locale.ROOT); + for (final String entry : protocols.split(",")) { + if (entry.trim().toLowerCase(Locale.ROOT).equals(p)) { + return true; + } + } + return false; + } + public static boolean isValidPort(final int p) { return !(p > PORT_RANGE_MAX || p < PORT_RANGE_MIN); } diff --git a/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java b/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java index 5c9d41f90a25..6843960fa7a3 100644 --- a/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java +++ b/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java @@ -955,4 +955,19 @@ public void testVpnIpRange() { String cidr = NetUtils.transformCidr(startIp + "/" + cidrSize); Assert.assertEquals("10.1.2.0/28", cidr); } + + @Test + public void testIsProtocolInList() { + assertTrue(NetUtils.isProtocolInList("tcp,udp,icmp,all", "tcp")); + assertTrue(NetUtils.isProtocolInList("tcp,udp,icmp,all", "ICMP")); + // provider capabilities with a space after the comma + assertTrue(NetUtils.isProtocolInList("tcp,udp,icmp, all", "all")); + assertTrue(NetUtils.isProtocolInList("tcp, udp", "udp")); + // part of an entry, or of the list, is not an entry + for (String protocol : new String[] {"cp", "t", "dp,ic", ",", "tcp,udp", "", "proxy", " all"}) { + assertFalse(protocol, NetUtils.isProtocolInList("tcp,udp,icmp,all,tcp-proxy", protocol)); + } + assertFalse(NetUtils.isProtocolInList(null, "tcp")); + assertFalse(NetUtils.isProtocolInList("tcp", null)); + } }