Skip to content

copyctopstring: callsites should check the boolean return to surface truncation (defense-in-depth) #712

Description

@jsavin

Rationale

PR #708 (#707) closed a stack buffer overflow in copyctopstring by clamping the payload to 255 bytes before the memmove. The overflow class is gone. However, the security review of #708 flagged a defense-in-depth gap: several db_format.c callsites pass user-supplied paths (from --migrate <db>.root, runtime compactdatabase verbs, programmatic ODB compaction) to copyctopstring and discard the new boolean return.

Post-#708 a >255-byte path no longer overflows — but it silently truncates. The truncated path is then passed to pathtofilespecopenfile / opennewfile_exclusive. A truncated path could resolve to a different file than the user intended (path-confusion → wrong-file open / wrong-file write). The existing pathtofilespec failure path catches most cases (the truncated path probably doesn't exist), but it's not a guarantee — if the truncation point happens to land on a real path, the operation succeeds at the wrong location.

The fix shape matches the frontier-cli/window_registry.c:116 pattern PR #708 already established: check the boolean return, log+fail on truncation.

Current State

Common/source/db_format.c callsites at:

  • Line 2151 (copyctopstring(dst_path, bsdst))
  • Line 2378 (copyctopstring(db_path, bspath))
  • Line 2493 (and possibly other migration paths in the same function — audit the surrounding migrate_* helpers)

discard the boolean return from the post-#707 copyctopstring. A >255-byte input still results in the truncated 255-byte prefix being used as if it were the full path.

Desired Outcome

Each affected callsite checks the return value and either:

if (!copyctopstring(dst_path, bsdst)) {
    fail_step = "dst_path > 255 bytes (truncated)";
    goto cleanup;
}

or surfaces the truncation through whatever error-reporting channel the surrounding function uses. Mirror frontier-cli/window_registry.c:116's pattern.

Exit Criteria

  • Audit every copyctopstring callsite that accepts user-supplied or filesystem-derived paths (the 338 calls grep, filtered to non-literal first args)
  • For each: either check the return and fail loud, OR add a comment explaining why truncation is safe at that callsite (e.g. "argument is hardcoded literal", "upstream validates length")
  • At minimum, the db_format.c migration / compaction callsites get the explicit check
  • Add an integration test that supplies a >255-byte path to --migrate and confirms the truncation is surfaced as an error rather than silently using the prefix

Context

Metadata

Metadata

Assignees

No one assigned

    Labels

    priority/p2Medium priority - nice to havescope/medium1-2 weeks, requires design workworkstream/build-toolingBuild system, dependencies, CMake, and vendoring

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions