[I] Network ACL protocol validation is a substring check: "cp", "dp,ic" etc. are accepted and the rule is silently not applied [cloudstack]

["bhouse-nexthop (via GitHub)" <gi...@apache.org>]

bhouse-nexthop opened a new issue, #14358:
URL: https://github.com/apache/cloudstack/issues/14358

   ### problem
   
   `createNetworkACL` / `updateNetworkACLItem` accept protocol names that are not valid. `NetworkACLServiceImpl.validateProtocol()` checks a non-numeric protocol with:
   
   ```java
   supportedProtocolsForAclRules.contains(protocol.toLowerCase())   // "tcp,udp,icmp,all"
   ```
   
   That is a substring test on the string `"tcp,udp,icmp,all"`, so `cp`, `t`, `dp,ic`, `,` and `tcp,udp` all pass.
   
   The rule is stored and listed, but is never applied. On the VR side, `SetNetworkAclConfigItem` doesn't recognise the protocol, tries `Integer.parseInt` on it, and drops the rule with only a warning in the agent log. An allow or a deny that the API accepted and the UI shows silently does nothing.
   
   ### versions
   
   Present on 4.22.
   
   ### The steps to reproduce the bug
   
   1. `createNetworkACL aclid=<id> protocol=cp startport=22 endport=22 cidrlist=0.0.0.0/0 action=deny traffictype=ingress`. It succeeds.
   2. `listNetworkACLs aclid=<id>` shows the rule with protocol `cp`.
   3. Nothing for it appears in the VR's `ACL_INBOUND_ethX` chain. The agent logs "Problem occurred when reading the entries in the ruleParts array".
   
   ### What to do about it?
   
   Compare against the entries of the list, not against the string.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Network ACL / firewall rule protocol validation is a substring check: "cp", "dp,ic" etc. are accepted [cloudstack]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] commented on issue #14358:
URL: https://github.com/apache/cloudstack/issues/14358#issuecomment-6062952055

   ## 🎯 Triage report
   
   Network ACL / firewall protocol validation (`NetworkACLServiceImpl.validateProtocol()` and equivalents in `FirewallManagerImpl`, `Ipv6ServiceImpl`, `RoutedIpv4ManagerImpl`) uses `String.contains()` against the concatenated capability string (`"tcp,udp,icmp,all"`) instead of checking membership in a parsed list. This lets malformed protocol values like `cp`, `t`, or `,` pass validation, get stored/listed via the API, but silently fail to be applied on the VR (agent logs a warning and drops the rule). The result is a rule that appears to exist but provides no actual firewalling — a silent security-relevant functional gap.
   
   ### 📊 Assessment
   
   | Dimension | Value | Reasoning |
   |---|---|---|
   | **Type** | type:bug | Validation logic incorrectly accepts invalid input, leading to a rule that is silently never enforced. |
   | **Component** | component:networking, component:virtual-router | Affects Network ACL/firewall rule validation and VR-side ACL application. |
   | **Severity** | Severity:Major | Not a crash, but silently fails to enforce a security rule the user believes is active (allow/deny goes unenforced without any visible error to the user). |
   | **Labels** | type:bug, component:networking, component:virtual-router, Severity:Major | See above. |
   | **Coding agent** | Suitable | Root cause, affected files/methods, and the fix approach (split the capability string and compare against trimmed list entries) are precisely identified by the reporter; fix is self-contained and localized. |
   
   No similar or duplicate issues were found in the repository.
   
   <details><summary>💡 Notes and suggestions</summary>
   
   - Fix should replace `supportedProtocolsForAclRules.contains(protocol.toLowerCase())` with a check against `Arrays.asList(supportedProtocolsForAclRules.split(","))` with each entry trimmed, applied consistently across `NetworkACLServiceImpl`, `FirewallManagerImpl`, `Ipv6ServiceImpl`, and `RoutedIpv4ManagerImpl`.
   - Worth adding/augmenting unit tests for `validateProtocol()` with inputs like `cp`, `dp,ic`, `,`, and `tcp,udp` to confirm they are now rejected.
   - Since this silently drops security rules without any API/UI error, consider whether existing stored rules with invalid protocols (created before the fix) should be flagged or cleaned up — out of scope for the code fix itself but worth a mention in the PR description.
   
   </details>
   
   
   
   > Generated by [Daily Issue Triage](https://github.com/apache/cloudstack/actions/runs/37785922422) · sonnet50 79.4K · [◷](https://github.com/search?q=repo%3Aapache%2Fcloudstack+%22gh-aw-workflow-call-id%3A+apache%2Fcloudstack%2Fdaily-issue-triage%22&type=issues)
   >
   <details>
   <summary>Add this agentic workflows to your repo</summary>
   
   To install this agentic workflow, run
   
   ```
   gh aw add githubnext/agentics/workflows/daily-issue-triage.md@d7c1dc4b72b00607a67caaffdcc216cb64379cf9
   ```
   </details>
   
   
   <!-- gh-aw-agentic-workflow: Daily Issue Triage, engine: copilot, version: 1.0.52, model: claude-sonnet-5, id: 37785922422, workflow_id: daily-issue-triage, run: https://github.com/apache/cloudstack/actions/runs/37785922422 -->
   <!-- gh-aw-workflow-call-id: apache/cloudstack/daily-issue-triage -->


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org