Skip to content

Bound asset-name copies to the field size (fixes stack overflow; addresses #58) - #137

Merged
fnordspace merged 1 commit into
qubic:mainfrom
parthod0x:fix/asset-name-copy-overflow
Oct 20, 2026
Merged

fnordspace merged 1 commit into
qubic:mainfrom
parthod0x:fix/asset-name-copy-overflow

Conversation

@parthod0x

Copy link
Copy Markdown
Contributor

Addresses the memory-safety half of #58.

Problem

qx.cpp, qswap.cpp and nostromo.cpp copy a user-supplied asset name into an 8-byte field with no bound — 19 sites of the form:

char assetNameS1[8] = {0};
memcpy(assetNameS1, assetName, strlen(assetName));   // unbounded

(plus the assetNameU1[8], &v.asset.assetName and &input.assetName variants). A name longer than the field overflows it. AddressSanitizer on an 11-char name:

ERROR: AddressSanitizer: stack-buffer-overflow
WRITE of size 11 ...

Fix

Add a small copyAssetNameBounded() helper in asset_utils.h that caps the copy at the field size (asset names are at most 7 chars), mirroring the bounded copy asset_utils.cpp already uses:

inline void copyAssetNameBounded(void* dst, const char* assetName, size_t capacity = 7)
{
    size_t n = assetName ? strlen(assetName) : 0;
    if (n > capacity) n = capacity;
    memcpy(dst, assetName, n);
}

All 19 sites now route through it, so an over-long name can no longer corrupt memory even if it slips past input validation.

Verification (AddressSanitizer)

Driving the real helper from asset_utils.h: the original unbounded pattern reports stack-buffer-overflow, WRITE of size 11 on an 8-byte buffer with an 11-char name; copyAssetNameBounded bounds the copy to 7 and is clean.

This is the defense-in-depth complement to the up-front validation #58 asks for (reject names > 7 chars before any fee is paid), and the same bug class as the node-IP / proposal-URL copies in #134.

@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.

I think instead of adding a new function it is enough to check the input with `sanityCheckValidAssetName``` for the two missing SC:

 case NOSTROMO_TRANSFER_SHARE_MANAGEMENT_RIGHTS:
      sanityCheckNode(g_nodeIp, g_nodePort);
      sanityCheckSeed(g_seed);
      sanityCheckValidAssetName(g_nost_tokenName);
      ...
  case NOSTROMO_CHECK_TOKEN_CREATABILITY:
      sanityCheckNode(g_nodeIp, g_nodePort);
      sanityCheckValidAssetName(g_nost_tokenName);
      ...

Two NOSTROMO subcommands dispatched g_nost_tokenName without first calling
sanityCheckValidAssetName, so an over-long name reached the request builder
and overran its 8-byte asset-name field. Every other asset command already
validates the name up front; add the same check to these two (addresses qubic#58).
@parthod0x
parthod0x force-pushed the fix/asset-name-copy-overflow branch from 2d0b8f9 to e1aca1c Compare October 15, 2026 18:18
@parthod0x

Copy link
Copy Markdown
Contributor Author

Reworked per your suggestion: removed the helper and all the copy-site edits, and just added sanityCheckValidAssetName(g_nost_tokenName) to the two cases that were missing it — NOSTROMO_TRANSFER_SHARE_MANAGEMENT_RIGHTS and NOSTROMO_CHECK_TOKEN_CREATABILITY. Every other asset command already validates up front. Rebased onto main; compiles clean under /WX. Thanks!

@fnordspace
fnordspace merged commit cd194f0 into qubic:main Oct 20, 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