RDKB:66583 -[Field]Multiple firewall rules error causing rules not getting applied.

[rirfha948]

Reason for Change:

• Implement general code improvements to enhance reliability, maintainability, and overall software quality.
• Address identified issues and optimize existing implementation.

Test Procedure:

• Verify the updated logic and functionality.
• Validate that existing features continue to work as expected.
• Perform regression testing to ensure no unintended impact on related functionality.
• Review logs and system behavior for any anomalies after the update
  RDKB-66853 - Unit testLogs.txt

[github-actions]

📋 PR Format Reminder

• Title: RDKB:66583 -[Field]Multiple firewall rules error causing rules not getting applied. — expected TICKET-123 : description
  (Multiple tickets OK: RDKCOM-5492 RDKBDEV-3336 : ... | Include US ticket + subtask for user-stories)
• Description missing:
  • Risks (Low / Medium / High)
  • Priority (P0 / P1 / P2)

Expected:

TICKET-123 : brief description

Reason for change: why
Test Procedure: how to verify
Risks: Low / Medium / High
Priority: P0 / P1 / P2

[Copilot]

🟡 Changes recommended

Unresolved moderate issues remain in rule ordering and port validation across both firewall backends.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR improves firewall rule generation with stricter validation and corrected managed-site rule insertion ordering.

Changes:

• Adds strict managed-site port validation.
• Validates IoT IPv4 addresses.
• Adjusts iptables and nftables rule insertion indexes.

File summaries

File	Review summary
source/firewall/firewall.c	Moderate findings: insertion offset can place rules after the broad GWMETA rule (line 9529, 1 vote); truncated ports can pass validation and emit incorrect rules (lines 9497 and 13027, 2 votes).
source/firewall/firewall_utils.c	Moderate finding: stricter validation is not shared with nftables, allowing invalid ports to break ruleset reloads (line 47, 1 vote).
source/firewall_nft/firewall_nft.c	Moderate findings: insertion index is off by one (line 8548, 2 votes), and managed-site ports lack validation, allowing invalid nft statements (line 8548, 2 votes).

Review details

Suppressed comments (3)

source/firewall/firewall.c:9529

• On builds where the conditional above emits the append rule, ruleIndex already includes its return value of 1. Since iptables -I positions are 1-based, the added + 1 inserts these rules after the broad --dports 80,443,8080 ... GWMETA rule instead of before it; matching traffic can therefore be handled by GWMETA before reaching the managed-site rule. Keep the extra offset only for configurations where the append rule is not emitted, or calculate the insertion point from the actual chain contents.

                    do_parcon_mgmt_lan2wan_pc_site_insertrule(fp, ruleIndex + 1, nstdPort);

source/firewall/firewall.c:13027

• This validation only protects the iptables generator. The equivalent nft generator still formats iot_ipaddr directly into add rule ip filter INPUT ... ip daddr %s/24 (source/firewall_nft/firewall_nft.c:11855), so an empty or malformed value can still make the generated nft ruleset invalid on nft builds. Apply the same inet_pton guard there or share the validation path.

      if (1 == inet_pton(AF_INET, iot_primaryAddress, &iot_ipv4addr))

source/firewall/firewall_utils.c:48

• This stricter validator is not used by the nft target: source/firewall_nft/Makefile.am:25 does not build firewall_utils.c, firewall_nft.c retains a private atoi-based validator for managed services, and its managed-site URL path performs no port validation. A value such as 80abc can therefore be emitted into the nft script and make the whole nft -f reload fail. Apply the same strict validation to both nft paths or move the helper into shared code.

    port = strtoul(port_num, NULL, 10);
    if (port <= 0 || port > MAX_PORT)

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

[Copilot]

🟡 Changes recommended

NFT paths still lack the corresponding fixes, and invalid IoT addresses can reach additional rule generation.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

source/firewall/firewall.c:9561

• The equivalent nft backend still emits do_parcon_mgmt_lan2wan_pc_site_insertrule(fp, ruleIndex, nstdPort) (source/firewall_nft/firewall_nft.c:8548), while this hunk changes only the iptables generator to ruleIndex + 1. Since the project builds a separate nft generator under FIREWALL_NFT, nft deployments retain the old insertion position and can still misorder the GWMETA rule when multiple explicit-port managed sites are configured. Apply the same index correction to the nft path or share the index calculation.

                    do_parcon_mgmt_lan2wan_pc_site_insertrule(fp, ruleIndex + 1, nstdPort);

source/firewall/firewall_utils.c:35

• The NFT backend is built separately (source/firewall_nft/Makefile.am) and does not use this helper: it defines its own validate_port() at source/firewall_nft/firewall_nft.c:8225, and its managed-site URL path still copies/emits nstdPort without this validation. On NFT builds, malformed or oversized managed-site ports can therefore still generate invalid rules and reproduce the reported failure. Share this helper or apply the same checks to the NFT implementation before treating port validation as complete.

    const char *digit;

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Apply equivalent strict port validation in the NFT backend before approval.

Review details

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

source/firewall/firewall.c:9534

• The new port validation is not used by the nft generator. In nft mode, the matching URL path still copies the configured port and emits it into nft commands without rejecting non-numeric or truncated values (source/firewall_nft/firewall_nft.c:8517-8531), so malformed managed-site URLs can still produce invalid rulesets. Share this validation with the nft path or add the equivalent check there.

This issue also appears in the following locations of the same file:

• line 9561
• line 13066
  source/firewall/firewall_utils.c:48

• The stricter port validation is only used by the classic backend; the separately built NFT backend retains its local atoi-based validator and does not validate managed-site URL ports before emitting rules. Thus malformed or truncated ports can still generate invalid NFT rules. Please mirror this validation in the NFT implementation or explicitly limit the behavior change to iptables.

source/firewall/firewall.c:9561

• This change only updates the iptables generator. When nft_enable=1, firewall_log_handle.sh runs firewall_nft, whose corresponding managed-site code still calls do_parcon_mgmt_lan2wan_pc_site_insertrule(fp, ruleIndex, nstdPort) at source/firewall_nft/firewall_nft.c:8548; multiple non-standard-port rules therefore retain the old insertion behavior and the reported failure remains for nft deployments. Apply the same index fix to the nft generator as well.

                    do_parcon_mgmt_lan2wan_pc_site_insertrule(fp, ruleIndex + 1, nstdPort);

source/firewall/firewall.c:13066

• This IPv4 check is only present in the iptables generator. With nft_enable=1, the runtime uses source/firewall_nft/firewall_nft.c, which still unconditionally writes iot_ipaddr into ip daddr %s/24 at line 11855; an invalid configured address can therefore still make the nft ruleset invalid. Mirror this validation in the nft implementation.

      if (1 == inet_pton(AF_INET, iot_primaryAddress, &iot_ipv4addr))

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate validation and rule-indexing findings remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

source/firewall/firewall.c:9606

• nstdPort is truncated to seven characters before it is validated. For example, 00000010 becomes 0000001, passes validate_port, and emits a rule for port 1 instead of port 10; other long values can likewise be changed silently. Validate the complete substring before copying it, or use a buffer that cannot truncate the source.

                    strncpy(nstdPort, urlType == IPv6_URL ? pch+2 : pch+1, sizeof(nstdPort)-1);
		    nstdPort[sizeof(nstdPort)-1] = '\0';
                  if ('\0' == nstdPort[0] || 0 != validate_port(nstdPort))

source/firewall/firewall.c:13147

• These checks only protect the iptables generator. With FIREWALL_NFT, the separately built source/firewall_nft generator still interpolates iot_ipaddr and iot_ifName directly in its corresponding IOT block (lines 11848-11858), so the same malformed configuration can still generate invalid nft rules. Mirror the IP/interface validation in that backend.

      valid_iot_ipaddr = (1 == inet_pton(AF_INET, iot_primaryAddress, &iot_ipv4addr));
      valid_iot_ifname = IsValidInterfaceName(iot_ifName);

source/firewall_nft/firewall_nft.c:8548

• ruleIndex already includes the rule emitted by do_parcon_mgmt_lan2wan_pc_site_appendrule() at line 8369. For the nft backend, adding another one makes the insertion point past the existing chain boundary (for example, with no trusted rules the chain has one rule but this emits position 2); nft positions are zero-based. Use the existing ruleIndex here and keep the + 1 adjustment only for the iptables -I form.

                    do_parcon_mgmt_lan2wan_pc_site_insertrule(fp, ruleIndex + 1, nstdPort);

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite