Skip to content

fix(#716): item 5 — bound >255-byte hostname in tcp_address_to_name#720

Merged
jsavin merged 2 commits into
developfrom
worktree-716-5-tcpverbs-hostname-bound
Jun 6, 2026
Merged

fix(#716): item 5 — bound >255-byte hostname in tcp_address_to_name#720
jsavin merged 2 commits into
developfrom
worktree-716-5-tcpverbs-hostname-bound

Conversation

@jsavin

@jsavin jsavin commented Jun 6, 2026

Copy link
Copy Markdown
Owner

Summary

Closes item 5 of #716 — the only remaining open item. (Items 1, 3 already closed via PRs #718, #719; items 2, 4, 6 closed as documentation-only per audit reconciliation.)

The bug. tcp_address_to_name uses getnameinfo() to do a reverse DNS lookup, writing into a char hostname[NI_MAXHOST] buffer. NI_MAXHOST is 1025 on macOS/Linux to accommodate non-DNS resolution sources (NIS, LDAP, /etc/hosts). The resolved hostname is then handed to copyctopstring(hostname, name_out) where name_out is a 256-byte UserTalk bigstring. Per PR #707, copyctopstring clamps the payload to 255 bytes and returns false on truncation — but the existing code at tcpverbs.c:1273 discarded that return value, silently writing a truncated hostname.

A long PTR record — or an attacker-controlled reverse lookup result like evil.com.<padding>...<more padding>.victim.com — could leave the prefix evil.com visible to UserTalk-level identity checks while the actual lookup intent was concealed past the 255-byte cutoff.

The fix. Check copyctopstring's boolean return. On truncation, emit a log_warn and fall back to the dotted-decimal IP string — the same fallback path used when getnameinfo() itself fails. Pattern matches frontier-cli/window_registry.c:116 and PR #714's post-#707 callsites.

Testability refactor. Extracted the post-getnameinfo hostname-to-bigstring decision into a new helper tcp_address_to_name_pack so the truncation branch is exercisable without mocking getnameinfo. The helper takes the resolved C string and the original address, writing the bigstring + fallback.

Other tcpverbs.c callsites audited

Verified bounded by construction; no other fixes needed:

Site Source Bound
tcp_set_error (line 387) char[256] from snprintf 255 (matches bigstring cap)
tcp_address_decode (line 666) INET_ADDRSTRLEN (16) 15 + NUL
9 status-string sites String literals <= 10 bytes

WinSockNetEvents.c is Windows-only — not linked into headless build.

Test plan

  • make build — clean
  • ./tools/run_headless_tests.sh591/591 pass (was 589; +2 new in tcp_phase1a_unit_tests)
  • Direct ./tests/tcp_phase1a_unit_tests — 33/33 (was 31; new tests at 3.8 and 3.9 exercise the helper directly)
  • cd tests && make test-integration — in progress at PR open; will confirm matches 21-baseline before merge

Behavioral tests added

TEST(address_to_name_pack_long_hostname_falls_back_to_ip)
TEST(address_to_name_pack_short_hostname_preserved)

Long-hostname test: constructs a 300-byte synthetic C string, calls tcp_address_to_name_pack, asserts:

  1. Helper returns false (signaling fallback used)
  2. name_out contains "192.168.1.1" (the dotted-decimal IP), NOT a 255-byte truncation of the synthetic hostname

Short-hostname test: passes "example.com", asserts helper returns true and name_out contains "example.com" verbatim.

Scope

This PR closes the last code-touching item of #716. The audit-tail issue can be closed after this merges.

Discovery process

Original audit framing: "DNS-spec'd 253 bytes for hostnames, which is safely under 255". This turned out to be only half the story — DNS labels are bounded, but getnameinfo returns into NI_MAXHOST (1025) which can hold longer names from non-DNS sources. The actual truncation surface only appears when you trace from the copyctopstring call site upward through the buffer that feeds it, not just the spec for the most-common input source.

🤖 Generated with Claude Code

NI_MAXHOST is 1025 (macOS, Linux) to accommodate non-DNS resolution
sources (NIS, LDAP, /etc/hosts), but UserTalk's bigstring max payload is
255. A long PTR record -- or an attacker-controlled reverse lookup that
returns a crafted hostname like "evil.com.<padding>...<more
padding>.victim.com" -- would silently truncate via copyctopstring's
post-#707 clamp, potentially leaving the prefix "evil.com" visible to
the UserTalk-level identity check while concealing the actual
intent of the lookup.

The fix surfaces the truncation as an explicit log_warn and falls
back to the dotted-decimal IP string -- the same fallback path used
when getnameinfo() itself fails. Pattern matches
frontier-cli/window_registry.c:116 and PR #714's post-#707 callsites.

For testability, the hostname-to-bigstring decision was extracted from
tcp_address_to_name into a new helper tcp_address_to_name_pack. The
helper takes the resolved C string and the original address, so the
truncation branch is testable without mocking getnameinfo. Two new
behavioral unit tests in tests/tcp_phase1a_unit_tests.c:

- address_to_name_pack_long_hostname_falls_back_to_ip: 300-byte
  synthetic hostname -> helper returns false, name_out is "192.168.1.1"
- address_to_name_pack_short_hostname_preserved: "example.com" ->
  helper returns true, name_out is "example.com" verbatim

Other tcpverbs.c copyctopstring callsites audited and verified
bounded by construction:
- tcp_set_error (line 387): char[256] bounded by snprintf
- tcp_address_decode (line 666): INET_ADDRSTRLEN (16 bytes)
- 9 string-literal sites (INACTIVE/OPEN/DATA/STOPPED/LISTENING/etc.)
  with payloads under 10 bytes

WinSockNetEvents.c is Windows-only, not linked into headless build.

Verified:
- make build: clean
- ./tools/run_headless_tests.sh: 591/591 pass (was 589, +2 new)
- cd tests && make test-integration: 21 baseline failures (matches
  PR #717 baseline; no tcp_address_to_name failures)

Closes item 5 of #716. Items 1, 3, 6 already closed; items 2, 4
closed as documentation-only.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…721

Security reviewer P2: add inline rationale to tcp_address_to_name_pack
explaining (a) why dotted-decimal IP is the right fallback rather than
empty / langerrormessage (UserTalk-caller identity-check safety,
contract preservation), and (b) why the log_warn intentionally omits
the truncated hostname (attacker-controlled, log-injection risk).

Security reviewer P2: memset(name_out, 0, sizeof(name_out)) before
each test call so a future regression that early-returns without
writing surfaces as a deterministic empty string rather than reading
stack garbage.

Bar-raiser P2: WinSockNetEvents.c:1419 has the same bug pattern in
the Windows path (not in headless build). Filed as #721 to track
when/if the Windows path is revived.

Bar-raiser P2 (comment placement) declined: the doc comment for
tcp_address_to_name still reads correctly as "see the helper above
for the truncation contract" -- moving it would create a different
clarity tradeoff.

Tests still 33/33.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jsavin jsavin merged commit 4660e0a into develop Jun 6, 2026
@jsavin jsavin deleted the worktree-716-5-tcpverbs-hostname-bound branch June 6, 2026 09:46
@jsavin

jsavin commented Jun 6, 2026

Copy link
Copy Markdown
Owner Author

/auto Summary

What went well: This was the chain's payoff — a real bug with a clear attack model (attacker-controlled reverse DNS truncating past a UserTalk identity-check substring). Extracting tcp_address_to_name_pack for testability was the right move because mocking getnameinfo is heavy and fragile; the helper seam lets the test drive a synthetic 300-byte hostname directly. Pattern consistency across PR #714 / #718 / #719 / #720 made the reviewer fatigue low.

What could improve: The chain's first three items (3, 4, 2) were doc-only because the audit targets pointed at legacy files not in the headless build. The audit script (the Plan-agent grep over Common/source/) didn't filter for live reachability through generated/kernel_verbs_init.c. Future audit-tail issues should include a "is this in the live build" reachability check as part of the initial scoping, not as a discovery during fix-time. This would have shortened the chain significantly. The lesson is now recorded in #716's item-3 closure comment for the next audit tail.

Recommendations:

  1. Bake a "live-reachability check" step into the Plan agent's audit workflow: cross-reference each callsite against generated/kernel_verbs_init.c (for verb-routed code) before flagging as a defect.
  2. WinSockNetEvents.c:1419 follow-up tracked as WinSockNetEvents.c:1419 mirror of #716 item 5 (hostname truncation on Windows path) #721 — should be addressed if/when the Windows path is revived.

🤖 Generated with Claude Code /auto

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