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

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.

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 outdated

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

Yudis-bit commented at 10:19 AM on November 2, 2026:

Hm, but the comment right below line 760 explicitly says:

/* Leaking these values would break indistinguishability of the transaction, so clear them. */

and clears label_tweak_scalar. If leaking it breaks indistinguishability, treating it as public seems inconsistent with the cleanup in this very PR.

Also secp256k1_declassify is a no-op outside valgrind/msan builds anyway, so adding it here avoids leaving a known ctime test failure in the tree.

theStack added the label assurance on Oct 23, 2026

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

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), 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.

95552e1654

in include/secp256k1_silentpayments.h:1 in b5fb2adf09 outdated

real-or-random commented at 7:59 AM on November 2, 2026:

Do we need the docs changes at all? If I understand correctly, the way the API is designed is that label tweaks are opaque to the caller, and they should use secp256k1_silentpayments_recipient_label_create to create one. Talking about scalars will only confuse the user.

theStack commented at 10:19 AM on November 2, 2026:

Good point, dropped the docs changes, adapted the commit message and PR description accordingly. Mentioning low-level details like scalars and group order for user types in the API docs seems counter-productive indeed.

theStack force-pushed on Nov 2, 2026

fametrano commented at 4:58 PM on November 2, 2026: contributor

tACK 95552e1654bfd3d34facfcf64e7ca3b0a52594c0

Since my test of b5fb2adf, git range-diff shows only the header doc changes dropped, from the commit and its message; the code and the test are unchanged. On Linux aarch64 (gcc 14.2, valgrind 3.24, no asm), make check passes and ctime_tests gives 0 errors under valgrind, on the head and on the head merged with master a3a5cd4 (patch, commands and logs).

The inline thread mentions a ctime failure left in the tree. The only one I know of is the one I reported in that thread, from a scan I added, and that scan is not in the tree. ctime_tests gives 0 errors with or without the secp256k1_declassify line, because its scan passes no label lookup, which its comment says is sufficient, and never reaches if (tweak_overflow). With my scan, which finds a labeled output, valgrind reports 0 errors with only the scan key marked secret. With the label tweak marked secret too, it reports 1 error at if (tweak_overflow). The declassify line removes that error, and the merge base gives 0 errors on the same test. So this only matters if the label tweak counts as secret, and even then the branch reveals only that a caller passed an invalid tweak, which recipient_label_create never returns. It does not block the PR.

That question, including Yudis-bit's point about clearing the tweak, stays open for the follow-up theStack proposed. If the answer is that the tweak is secret, the fix is my one-line suggestion, and my scan can go into ctime_tests with it.

real-or-random approved

real-or-random commented at 9:36 AM on November 3, 2026: contributor

utACK 95552e1654bfd3d34facfcf64e7ca3b0a52594c0

Yes, we can talk about whether to declassify or not in a follow-up

real-or-random merged this on Nov 3, 2026

real-or-random closed this on Nov 3, 2026

theStack deleted the branch on Nov 3, 2026