silentpayments: reject label tweaks that overflow the group order #1974

pull theStack wants to merge 1 commits into bitcoin-core:master from theStack:check-label_cache-tweak-overflow changing 3 files +17 −4

theStack commented at 3:05 AM on October 22, 2026: contributor

Label tweaks returned by the label_lookup callback that overflow the group order were silently reduced. recipient_label_create never produces such tweaks, so an overflowing one means the caller's label cache is broken. As suggested in #1929 (comment), this PR treats it as API misuse using ARG_CHECK (illegal callback, return 0), documents the requirement. It also adds a test and a changelog entry.

silentpayments: reject label tweaks that overflow the group order

Previously, a label tweak returned by the `label_lookup` callback that
is not smaller than the group order was silently reduced modulo the
group order. Such a tweak can never be produced by
`secp256k1_silentpayments_recipient_label_create`, so it indicates a
corrupted or improperly constructed label cache, i.e., a bug on the
caller side rather than invalid data from an untrusted source. Treat
this as an API misuse (similar to the magic byte checks on the
`prevouts_summary` argument), document the requirement, and add a
test.

The assignment of the found output is moved after the label tweak
check, so that the found_outputs[k] entry is left untouched if the
scan is aborted due to an invalid label tweak.

b5fb2adf09

real-or-random commented at 9:20 PM on October 22, 2026: contributor

Concept ACK

I tend to thinkl the Changelog entry really adds more confusion then it solves. Any caller who hits this is clearly violates the API contract (in a very weird way).
theStack force-pushed on Oct 23, 2026
theStack commented at 1:47 AM on October 23, 2026: contributor

I tend to thinkl the Changelog entry really adds more confusion then it solves. Any caller who hits this is clearly violates the API contract (in a very weird way).

Makes sense, dropped the changelog commit.
fametrano commented at 4:10 AM on October 23, 2026: contributor

tACK b5fb2adf090d49b034db86b60a740d6058743ff7

I built the head with CMake on macOS arm64 and ran ctest: all 231 tests pass. I also wrote a small program against the public API that scans a one-output labeled transaction with several tweaks in the label cache, and ran it against the head and the merge base. With the tweak from recipient_label_create, both find the output and return a tweak that spends it. With the group order or 2^256-1, the merge base returns 1 and a tweak that does not spend the output. The head calls the illegal callback, returns 0, reports no found outputs and leaves the found-output entry untouched. I can share the program if useful.
fametrano commented at 6:07 AM on October 23, 2026: contributor

Under valgrind's constant-time check, the new if (tweak_overflow) branches on a secret value when the label tweak is marked secret.

I added to ctime_tests a scan that finds an output sent to a labeled address, with the scan key and the label tweak (the value label_lookup returns) marked secret, and ran it under valgrind. Valgrind reports "Conditional jump or move depends on uninitialised value(s)" at if (tweak_overflow) { in secp256k1_silentpayments_recipient_scan_outputs. The same test gives 0 errors on the base commit. With the one-line secp256k1_declassify suggested inline, the run is clean and the output is still found.

I can post the test here, or open a follow-up PR that adds it to src/ctime_tests.c.

in src/modules/silentpayments/main_impl.h:759 in b5fb2adf09

 755 |               * break indistinguishability of the transaction. */
 756 |              secp256k1_scalar_clear(&t_k_scalar);
 757 |              if (label_tweak != NULL) {
 758 | +                int tweak_overflow;
 759 |                  secp256k1_scalar label_tweak_scalar;
 760 | +                secp256k1_scalar_set_b32(&label_tweak_scalar, label_tweak, &tweak_overflow);

fametrano commented at 6:07 AM on October 23, 2026:

Declassify tweak_overflow before it is branched on, as is done for valid_scan_key.

                secp256k1_scalar_set_b32(&label_tweak_scalar, label_tweak, &tweak_overflow);
                secp256k1_declassify(ctx, &tweak_overflow, sizeof(tweak_overflow));

theStack commented at 9:42 AM on October 23, 2026:

Would prefer to do this in a follow-up PR, where we can discuss whether the label-related values need to be treated as secret in the context of the scanning function in the first place (I'd lean towards no, but curious about other opinions).

theStack added the label assurance on Oct 23, 2026
theStack added the label tweak/refactor on Oct 23, 2026

Contributors

theStack

real-or-random

fametrano

Labels

assurance tweak/refactor

Linked (view graph)

#1929 silentpayments: perform tweak addition with actual scalars