Skip to content

nstun: fail closed on invalid policy rule fields#289

Open
carrerasdarren-cell wants to merge 1 commit into
google:masterfrom
carrerasdarren-cell:fix/nstun-reject-invalid-rules
Open

nstun: fail closed on invalid policy rule fields#289
carrerasdarren-cell wants to merge 1 commit into
google:masterfrom
carrerasdarren-cell:fix/nstun-reject-invalid-rules

Conversation

@carrerasdarren-cell

Copy link
Copy Markdown

Summary

  • reject malformed IPv4/IPv6 rule selectors and CIDR prefix lengths
  • reject missing, reversed, zero, and out-of-range port ranges before narrowing to uint16_t
  • reject invalid guest, gateway, and redirect addresses instead of continuing with zeroed values
  • add focused parser and rule-validation regression tests

Security rationale

nstun_init_parent() previously logged malformed policy fields but continued with a zero-initialized rule. For HOST_TO_GUEST redirects, an invalid src_ip therefore became INADDR_ANY when the host listener was bound. For guest-to-host policy, an invalid selector on an ALLOW rule became a wildcard match, potentially bypassing a later deny rule.

Port fields are uint32 in the protobuf but uint16_t internally. Values such as 65536 silently became zero, which is the internal wildcard sentinel, while other values could select a different host port. Prefix lengths were not range-checked, and the IPv4 mask expression could shift by an invalid count.

Local proof

With stock master and a local HOST_TO_GUEST rule containing src_ip: "127.0.0.999" and dport: 18080:

  • nsjail logged the parse error but continued
  • /proc/net/tcp showed 00000000:46A0, a listener on 0.0.0.0:18080
  • the jailed command executed and nsjail exited successfully

With this patch, the same config aborts before command execution and leaves no listener. The proof used only disposable local containers and no external network service.

Validation

  • native ARM64 Linux full build with project -Werror flags
  • independent AMD64 Linux full build
  • make nstun_policy_test on both architectures
  • focused test under AddressSanitizer and UndefinedBehaviorSanitizer
  • stock-versus-patched privileged local integration proof
  • clang-format --dry-run --Werror
  • git diff --check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant