Skip to content

Fix buffer overflows when parsing seed and node_ip from a config file - #133

Merged
fnordspace merged 1 commit into
qubic:mainfrom
parthod0x:fix/config-parse-buffer-overflows
Oct 15, 2026
Merged

fnordspace merged 1 commit into
qubic:mainfrom
parthod0x:fix/config-parse-buffer-overflows

Conversation

@parthod0x

Copy link
Copy Markdown
Contributor

The -conf config-file parser in argparser.h copies fixed-length data into
heap buffers without room for a null terminator or a bound on the copy length.
Three memory-safety bugs are reachable from a config file, all confirmed with
AddressSanitizer in isolated reproductions:

  1. seed over-read (relates to getNodeIpList crashes #13). malloc(55); memset(0,55); memcpy(…,55)
    fills all 55 bytes with no terminator. sanityCheckSeed() then calls
    strlen(g_seed) (sanity_check.h) which reads past the allocation —
    heap-buffer-overflow, READ of size 56.
  2. seed source over-read. The same memcpy(…, 55) reads 55 bytes from the
    config value even when it is shorter, over-reading the source string.
  3. node_ip write overflow. memcpy(g_nodeIp, v[1].c_str(), v[1].size())
    into a 64-byte buffer with no bound — a node_ip value longer than 64 chars
    overflows it (heap-buffer-overflow, WRITE).

Fix

  • seed: allocate 56 bytes (55-char seed + terminator) and copy at most the
    value's own length (capped at 55). strlen() now stops in-bounds and short
    values are not over-read.
  • node_ip: cap the copy at 63 bytes so the 64th stays a terminator, and
    guard the trailing-newline check against an empty value.

The command-line -seed / -nodeip paths were already safe (they point at
null-terminated argv), and the msvault vault-name parser already bounds its
copy with if (len > 32) len = 32; — this brings the config path in line.

Verification

  • The three overflows reproduce under ASAN on current main; the patched logic
    eliminates all three (seed strlen in-bounds for 10/55/60-char values;
    node_ip capped for 100-char values; no source over-read).
  • g++ -std=c++17 -fsyntax-only argparser.h passes.

Related to #13 (the seed null-termination report there is still present in the
config path; #13's other two reports — the printSystemInfoFromNode hex and the
getNodeIpList peer loop — are already resolved on current main).

The `-conf` config parser copied fixed-length data into heap buffers without
leaving room for a null terminator or bounding the copy, so three memory-safety
bugs were reachable from a config file (all confirmed with AddressSanitizer):

1. seed: `malloc(55); memset(0,55); memcpy(.., 55)` filled all 55 bytes with no
   terminator, so `sanityCheckSeed()`'s `strlen(g_seed)` read past the
   allocation (heap-buffer-overflow, READ of size 56). Relates to qubic#13.
2. seed: the same `memcpy(.., 55)` read 55 bytes from the config value even when
   it was shorter, over-reading the source string.
3. node_ip: `memcpy(g_nodeIp, v[1].c_str(), v[1].size())` into a 64-byte buffer
   with no bound, so a value longer than 64 chars overflowed it
   (heap-buffer-overflow, WRITE).

Fixes:
- seed: allocate 56 bytes (55 + terminator) and copy at most the value's own
  length (capped at 55), so strlen() stops in-bounds and short values are not
  over-read.
- node_ip: cap the copy at 63 bytes so the 64th stays a terminator, and guard
  the trailing-newline check against an empty value.

The existing `-seed`/`-nodeip` command-line paths were already safe (they point
g_seed/g_nodeIp at null-terminated argv), as is the msvault vault-name parser,
which already bounds its copy; this brings the config path in line.

@fnordspace fnordspace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you for submitting the fix.

@fnordspace
fnordspace merged commit 0cf8541 into qubic:main Oct 15, 2026
3 checks passed
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.

2 participants