Skip to content

txin_check.c: txin_dgst_save_and_reset() unconditionally memcpy's 130 bytes from addr_str -- one real call site's buffer is only 50 bytes (80-byte OOB stack read) #580

Description

@BitHighlander

Summary

`txin_dgst_save_and_reset(const char* amt_str, const char* addr_str)` takes no length parameter and unconditionally does `memcpy(last_addr_str, addr_str, ADDR_STR_LEN)` (`ADDR_STR_LEN` = 130), trusting every caller's buffer to be at least that large. `transaction.c`'s internal-transfer confirmation path (an ordinary account-to-account send, or a THORChain/Maya swap's transfer leg) declares `char node_str[NODE_STRING_LENGTH]` (50 bytes) and passes it to this function.

Impact (high)

Every such confirmed output triggers an 80-byte out-of-bounds stack read (undefined behavior), and the garbage bytes get copied into the global `last_addr_str` used by the duplicate-transaction anti-malware detector, polluting its comparison state. The sibling call site (`transaction.c`'s generic `needs_confirm` path) correctly passes a real 130-byte protobuf field (`TxOutputType.address`), confirming the function's implicit 130-byte contract is real -- just silently violated by the `node_str` call site.

Fix direction

Either add a length parameter to `txin_dgst_save_and_reset()`/`txin_dgst_compare()` and pad/bound accordingly, or enlarge `node_str` to `ADDR_STR_LEN` at the call site (matching the function's actual contract).

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions