fix: harden Paillier validation, fix buf_t aliasing, and add bn256_t …

[hsiuhsiu]

…constant-time arithmetic

• Fix critical use-after-free in buf_t::operator=(mem_t) and operator+=(mem_t) where a mem_t view aliasing the destination buffer would read zeroed/freed memory after the buffer was destroyed during reassignment.
• Harden Paillier create_pub to validate modulus (odd, >1, within bit_size) and return error_t instead of void, preventing acceptance of malicious moduli from counterparties.
• Enforce upper-bound check on Paillier modulus size in ECDSA 2PC keygen and refresh to prevent potential buffer overflows in constant-time decryption.
• Fix out-of-bounds write in HD key derivation by resizing derived_keys inside derive_keys() rather than relying on caller pre-allocation.
• Add bn256_t: fixed-width 256-bit modular arithmetic with constant-time Barrett reduction, Montgomery multiplication, and x86_64 MULX/ADX assembly. Replaces the old uint256_t from extended_uint.cpp.
• Fix const-correctness in secp256k1 point conversion to avoid mutating input Jacobian coordinates through cast-away-const UB.
• Add vartime_scope_t to paillier_t::verify_cipher for correct side-channel annotation on public ciphertext validation.
• Add bits_t::ref_t copy assignment operator for correct value semantics.
• Fix static initialization order for test channel fuzzing DRBG.

[cb-heimdall]

✅ Heimdall Review Status

Requirement	Status	More Info
Reviews	✅ 2/2	Denominator calculation Show calculation 1 if user is bot 0 1 if user is external 0 2 if repo is sensitive 0 From .codeflow.yml 2 Additional review requirements Show calculation Max 0 0 From CODEOWNERS 1 Global minimum 0 Max 2 2 1 if commit is unverified 0 Sum 2
CODEOWNERS	✅ None for this change

[valery-osheter-cb]

Approved

In general, to fix a “Rule of Two” issue, you either (a) remove the custom copy operation if it’s unnecessary and let the compiler generate both copy constructor and assignment, or (b) explicitly provide the missing corresponding operation (either = default or = delete) to make the copy semantics consistent and clear.

For ref_t, the simplest and most accurate fix is to explicitly declare a copy constructor and default it. This matches the existing semantics (a trivial bitwise copy of data and offset is desired) and does not change behavior. We should add ref_t(const ref_t&) noexcept = default; to the public section of ref_t alongside the existing assignment operators. No other methods, imports, or definitions are required. The rest of the class (get, set, private constructor, etc.) can remain unchanged.

Concretely, in include/cbmpc/core/buf.h, within class bits_t::ref_t, we will insert the defaulted copy constructor declaration just before or after the existing assignment operators (e.g., before ref_t& operator=(bool value) noexcept). This ensures ref_t now has both copy constructor and copy assignment explicitly defined, resolving the CodeQL warning without modifying existing functionality.

In general, the fix is to add clear, high-level documentation comments to the large function so that its purpose, inputs/outputs, and algorithm are understandable without reverse-engineering the assembly. For an inline assembly cryptographic routine, the best approach is to add a descriptive Doxygen-style comment block immediately above the function declaration and, optionally, a few key inline comments inside the assembly string to mark algorithm phases (e.g., computing q̂, multiplying by modulus, conditional subtraction). This preserves existing behavior while greatly improving readability.

Concretely, in src/cbmpc/crypto/base_bn256.cpp, above the definition of barrett_reduce256_intel_mulx, we should add a multi-line comment describing: (1) that it performs Barrett reduction of a 512-bit integer x modulo 256-bit modulus m using a 320-bit precomputed constant mu; (2) the expected layout of the input/output arrays; (3) that it uses x86_64 MULX/ADCX instructions for constant-time big-integer arithmetic; and (4) any important invariants (e.g., x < m^2 if that applies). Inside the function body, we already have one comment about register roles; keeping that and adding a few short comments marking the major algorithm steps is sufficient to raise the comment ratio and clarify structure, without touching the actual assembly instructions. No new imports, types, or other definitions are needed.

In general, to fix commented-out code you either (1) remove it entirely if it is not needed, or (2) reinstate it as active code if it actually should be executed, or (3) rewrite it so it is clearly documentation rather than code (e.g., by using different formatting or wording).

In this specific case, the commented-out lines:

// static uint256_t RR = uint256_t::get_mont_rr(MOD::w);
// mont_mul(r, a, RR);

are an older/alternative implementation of to_mont(). The current implementation already obtains RR from mod->get_mont_ctx()->RR and uses r.mont_mul(*this, b, ...), so these comments are redundant and potentially confusing. The minimal fix that does not change behavior is simply to delete these two commented-out lines. No additional imports, methods, or definitions are required.

Concretely, in src/cbmpc/crypto/base_bn256.cpp, inside bn256_t::to_mont(), remove lines 1337–1338 entirely.

In general, to fix “no raw arrays in interfaces,” you should avoid function parameters like T arr[N] or T arr[], because they decay to T* and misrepresent the actual type. Instead, either use an explicit pointer T*/const T*, or a container (std::array<T,N>, std::vector<T>, etc.) when appropriate. In low-level crypto code where performance and layout are critical, using an explicit pointer is often the least disruptive change.

For this specific issue, the best fix without changing functionality is to adjust the signature of bn256_t::reduce from taking uint64_t x[8] to taking uint64_t* x. Inside the function, x is only forwarded to barrett_reduce, so no internal changes are needed beyond the type change. This keeps the semantics identical (the caller was already effectively passing a uint64_t*) while aligning with the rule by removing the misleading array syntax from the interface.

Concretely, in src/cbmpc/crypto/base_bn256.cpp around line 1096, update:

bn256_t bn256_t::reduce(uint64_t x[8]) {

to:

bn256_t bn256_t::reduce(uint64_t* x) {

No additional methods, imports, or definitions are required inside this file snippet. The corresponding declaration in the class header (base_bn256.h) would also need to be updated to match, but per your constraints we do not edit that file here.

In general, to fix “no raw arrays in interfaces” for function parameters, you either (1) change the parameter type to a container (such as std::array<uint64_t, 8>), or (2) change the parameter to an explicit pointer (e.g., uint64_t* r) so the interface clearly indicates pointer semantics rather than an array type that will decay. Since we must avoid changing functionality and we don’t see the calling code, the least invasive and most accurate fix is to change the parameter declaration from uint64_t r[8] to uint64_t* r. This matches the actual ABI/type the compiler already uses and keeps all uses (r[0] … r[7]) valid, while eliminating the misleading array syntax.

Concretely, in src/cbmpc/crypto/base_bn256.cpp, locate the definition of void bn256_t::mul_add_no_reduce(uint64_t r[8], const bn256_t& a, const bn256_t& b) starting at line 1060. Replace the first parameter’s type with uint64_t* r. No other changes are needed in this function body because indexing into r works identically for a pointer parameter. We are not adding new methods or imports; we are just clarifying the parameter type to align with actual pointer semantics and the CodeQL rule’s recommendation.

To fix the problem, the commented‑out “code” line should either be deleted or rewritten as a descriptive comment that no longer looks like compilable C++. This avoids confusion and aligns with the guidance to not keep commented‑out code around. We should not change any functional code around it, because the existing explicit multiplication/addition sequence is clearly the intended implementation.

The single best minimal fix is to change // q2[8] = mul_add_regular_mu(q2+3, q1[3], mu); into a plain‑English explanation, e.g., indicating that the following manual operations are equivalent to a conceptual mul_add_regular_mu step. This preserves useful algorithmic context for readers without leaving behind what looks like disabled code.

Concretely, in src/cbmpc/crypto/base_bn256.cpp, locate the block around line 129 and replace that single comment line with a descriptive comment such as // Conceptually: q2[8] = mul_add_regular_mu(q2 + 3, q1[3], mu); or similar wording, ensuring it is clearly documentation rather than code. No new methods, imports, or definitions are needed.

To fix the problem, remove the commented-out code line so that only active, tested code remains. This avoids confusion and keeps the function’s implementation clear and maintainable.

Concretely, in src/cbmpc/crypto/base_bn256.cpp, within barrett_reduce256_noasm, delete the comment line // q2[7] = mul_add_regular_mu(q2+2, q1[2], mu); that appears just before the block starting with b = uint128_t(q1[2]);. For consistency and to avoid similar findings, you should also remove the analogous commented-out lines for q2[5] and q2[6], which are likewise directly followed by the fully expanded implementation.

No new methods, imports, or definitions are required: we are only deleting three comment lines and leaving the existing arithmetic sequence intact.

To fix the problem in general, either delete commented-out executable code or convert it into a more clearly non-executable explanation (for example, describing the operation in prose or using different formatting). If the commented-out statement represents the desired behavior and the surrounding code does not, then the alternative is to restore it by uncommenting and removing the redundant manual implementation.

In this specific case, the code that follows line 91 already implements the behavior that the commented-out function call would perform, so reinstating the call would be a functional change and likely incorrect. The minimal, non-functional fix is to remove the commented-out code line // q2[6] = mul_add_regular_mu(q2+1, q1[1], mu); from barrett_reduce256_noasm in src/cbmpc/crypto/base_bn256.cpp. No other code or imports are required.

To fix the problem, remove the commented-out line of code so that only active, compiled code remains. This preserves existing functionality because the implementation is already present in the subsequent lines; we are not changing any executed logic.

Concretely, in src/cbmpc/crypto/base_bn256.cpp, within barrett_reduce256_noasm, delete the line // q2[5] = mul_add_regular_mu(q2+0, q1[0], mu); at line 71. No additional imports, helper methods, or definitions are required, and no other parts of the file need to be adjusted.

To fix the problem, remove the commented-out code fragment so that only the actual declaration of q2 remains. This eliminates the distraction and the risk that future maintainers misinterpret the comment as active or intended code.

Concretely, in src/cbmpc/crypto/base_bn256.cpp, locate the declaration uint64_t q2[10]; // = {0}; inside barrett_reduce256_noasm. Replace that line with a declaration that omits the trailing commented-out initializer, leaving just uint64_t q2[10]; (retaining the same indentation). No additional imports, methods, or definitions are required, and this change does not alter current behavior, since the initializer was already commented out.