Skip to content

fix: constant-time hardening and input validation across crypto primitives#88

Merged
hsiuhsiu merged 1 commit intomasterfrom
various-fix
Jan 20, 2026
Merged

fix: constant-time hardening and input validation across crypto primitives#88
hsiuhsiu merged 1 commit intomasterfrom
various-fix

Conversation

@hsiuhsiu
Copy link
Copy Markdown
Contributor

Security improvements for side-channel resistance:

  • Fix Edwards curve identity detection to use (X=0 && Y==Z), avoiding confusion with torsion points like (0,-1)
  • Upgrade secp256k1 point addition to fully constant-time using affine conversion + cmov pattern, removing conditional CT fallback
  • Add constant-time conditional copy (cnd_copy_point) for secp256k1 and Ed25519
  • Fix bn_cmp_ct to properly handle comparison of negative numbers
  • Add constant_time_select_bytes/ct_select_buf utilities
  • Enforce CT-capable curves for ElGamal operations; reduce scalars mod q

Input validation hardening:

  • Add range checks for Paillier encrypt/decrypt inputs
  • Change paillier_t::rand_N_star to return error on gcd(r,N)!=1 by default
  • Convert commitment::verify asserts to proper error returns
  • Add ro_t::get_buf_checked with output length validation
  • Validate share indices and threshold in secret sharing
  • Add input validation across ZK proofs (Paillier, ElGamal, Pedersen)
  • Add protocol-level validation in ECDSA 2P/MP, OT, PVE, Schnorr, DKG

Other fixes:

  • Fix memory leak and uninitialized cmems_t in Go FFI transport
  • Add verify_out_of_band for MPC session transport verification
  • Remove unused pbkdf2, ct_add_support_e::Conditional, ZK batch functions

…tives

Security improvements for side-channel resistance:
- Fix Edwards curve identity detection to use (X=0 && Y==Z), avoiding
  confusion with torsion points like (0,-1)
- Upgrade secp256k1 point addition to fully constant-time using affine
  conversion + cmov pattern, removing conditional CT fallback
- Add constant-time conditional copy (cnd_copy_point) for secp256k1 and Ed25519
- Fix bn_cmp_ct to properly handle comparison of negative numbers
- Add constant_time_select_bytes/ct_select_buf utilities
- Enforce CT-capable curves for ElGamal operations; reduce scalars mod q

Input validation hardening:
- Add range checks for Paillier encrypt/decrypt inputs
- Change paillier_t::rand_N_star to return error on gcd(r,N)!=1 by default
- Convert commitment::verify asserts to proper error returns
- Add ro_t::get_buf_checked with output length validation
- Validate share indices and threshold in secret sharing
- Add input validation across ZK proofs (Paillier, ElGamal, Pedersen)
- Add protocol-level validation in ECDSA 2P/MP, OT, PVE, Schnorr, DKG

Other fixes:
- Fix memory leak and uninitialized cmems_t in Go FFI transport
- Add verify_out_of_band for MPC session transport verification
- Remove unused pbkdf2, ct_add_support_e::Conditional, ZK batch functions
@cb-heimdall
Copy link
Copy Markdown

cb-heimdall commented Jan 20, 2026

✅ 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

@hsiuhsiu hsiuhsiu marked this pull request as ready for review January 20, 2026 05:01
// single-branch computation, and assume this timing difference is not practically exploitable
// in our threat model for this code path.
mem_t Vbi = b[i] ? mem_t(V1[i]) : mem_t(V0[i]);
const ecc_point_t& Ubi = b[i] ? U1[i] : U0[i];

Check notice

Code scanning / CodeQL

Declaration hides variable Note

Variable Ubi hides another variable of the same name (on
line 93
).

Copilot Autofix

AI 3 months ago

In general, to fix a “declaration hides variable” issue, rename one of the conflicting variables so that each scope has a distinct identifier. That way, there is no shadowing and it is always unambiguous which variable is referenced at each use.

Here, the simplest and safest change is to keep the original ecc_point_t Ubi (line 94) as-is for the constant-time branch, and rename the const ecc_point_t& Ubi in the else branch (line 105) to a different, descriptive name such as U_branch (or similar). Then update the single usage on line 106 to use the new name. This does not affect logic: the if branch will continue using the mutable Ubi, and the else branch will use the new reference name while still referring to b[i] ? U1[i] : U0[i].

Concretely, in src/cbmpc/protocol/ot.cpp, inside base_ot_protocol_pvw_ctx_t::output_R, in the else block around lines 99–107, change:

  • const ecc_point_t& Ubi = b[i] ? U1[i] : U0[i];
  • x[i] = crypto::ro::hash_string(r[i] * Ubi).bitlen(l) ^ Vbi;

to:

  • const ecc_point_t& U_branch = b[i] ? U1[i] : U0[i];
  • x[i] = crypto::ro::hash_string(r[i] * U_branch).bitlen(l) ^ Vbi;

No new methods, definitions, or imports are needed.

Suggested changeset 1
src/cbmpc/protocol/ot.cpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/cbmpc/protocol/ot.cpp b/src/cbmpc/protocol/ot.cpp
--- a/src/cbmpc/protocol/ot.cpp
+++ b/src/cbmpc/protocol/ot.cpp
@@ -102,8 +102,8 @@
       // single-branch computation, and assume this timing difference is not practically exploitable
       // in our threat model for this code path.
       mem_t Vbi = b[i] ? mem_t(V1[i]) : mem_t(V0[i]);
-      const ecc_point_t& Ubi = b[i] ? U1[i] : U0[i];
-      x[i] = crypto::ro::hash_string(r[i] * Ubi).bitlen(l) ^ Vbi;
+      const ecc_point_t& U_branch = b[i] ? U1[i] : U0[i];
+      x[i] = crypto::ro::hash_string(r[i] * U_branch).bitlen(l) ^ Vbi;
     }
   }
 
EOF
@@ -102,8 +102,8 @@
// single-branch computation, and assume this timing difference is not practically exploitable
// in our threat model for this code path.
mem_t Vbi = b[i] ? mem_t(V1[i]) : mem_t(V0[i]);
const ecc_point_t& Ubi = b[i] ? U1[i] : U0[i];
x[i] = crypto::ro::hash_string(r[i] * Ubi).bitlen(l) ^ Vbi;
const ecc_point_t& U_branch = b[i] ? U1[i] : U0[i];
x[i] = crypto::ro::hash_string(r[i] * U_branch).bitlen(l) ^ Vbi;
}
}

Copilot is powered by AI and may make mistakes. Always verify output.
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 have the same question. On line 94, ecc_point_t Ubi = U0[i]; is defined and used in the if branch. But in the else branch Ubi is shadowed. Ubi is not used outside of the if/else branch. So maybe the solution is to move line 94 to inside the if branch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Given that this doesn't change the code behavior, I will include that in the next PR.

Copy link
Copy Markdown

@valery-osheter-cb valery-osheter-cb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@hsiuhsiu hsiuhsiu merged commit 82e2691 into master Jan 20, 2026
8 checks passed
@hsiuhsiu hsiuhsiu deleted the various-fix branch January 20, 2026 15:34
tankbottoms added a commit to tankbottoms/cb-mpc that referenced this pull request Mar 2, 2026
…tives (coinbase#88)

Security improvements for side-channel resistance:
- Fix Edwards curve identity detection to use (X=0 && Y==Z), avoiding confusion with torsion points like (0,-1)
- Upgrade secp256k1 point addition to fully constant-time using affine conversion + cmov pattern, removing conditional CT fallback
- Add constant-time conditional copy (cnd_copy_point) for secp256k1 and Ed25519
- Fix bn_cmp_ct to properly handle comparison of negative numbers
- Add constant_time_select_bytes/ct_select_buf utilities
- Enforce CT-capable curves for ElGamal operations; reduce scalars mod q

Input validation hardening:
- Add range checks for Paillier encrypt/decrypt inputs
- Change paillier_t::rand_N_star to return error on gcd(r,N)!=1 by default
- Convert commitment::verify asserts to proper error returns
- Add ro_t::get_buf_checked with output length validation
- Validate share indices and threshold in secret sharing
- Add input validation across ZK proofs (Paillier, ElGamal, Pedersen)
- Add protocol-level validation in ECDSA 2P/MP, OT, PVE, Schnorr, DKG

Other fixes:
- Fix memory leak and uninitialized cmems_t in Go FFI transport
- Add verify_out_of_band for MPC session transport verification
- Remove unused pbkdf2, ct_add_support_e::Conditional, ZK batch functions
tankbottoms pushed a commit to tankbottoms/cb-mpc that referenced this pull request Mar 15, 2026
…tives (coinbase#88)

Security improvements for side-channel resistance:
- Fix Edwards curve identity detection to use (X=0 && Y==Z), avoiding confusion with torsion points like (0,-1)
- Upgrade secp256k1 point addition to fully constant-time using affine conversion + cmov pattern, removing conditional CT fallback
- Add constant-time conditional copy (cnd_copy_point) for secp256k1 and Ed25519
- Fix bn_cmp_ct to properly handle comparison of negative numbers
- Add constant_time_select_bytes/ct_select_buf utilities
- Enforce CT-capable curves for ElGamal operations; reduce scalars mod q

Input validation hardening:
- Add range checks for Paillier encrypt/decrypt inputs
- Change paillier_t::rand_N_star to return error on gcd(r,N)!=1 by default
- Convert commitment::verify asserts to proper error returns
- Add ro_t::get_buf_checked with output length validation
- Validate share indices and threshold in secret sharing
- Add input validation across ZK proofs (Paillier, ElGamal, Pedersen)
- Add protocol-level validation in ECDSA 2P/MP, OT, PVE, Schnorr, DKG

Other fixes:
- Fix memory leak and uninitialized cmems_t in Go FFI transport
- Add verify_out_of_band for MPC session transport verification
- Remove unused pbkdf2, ct_add_support_e::Conditional, ZK batch functions
dafisher2000 added a commit to IntegraLedger/cb-mpc that referenced this pull request Mar 23, 2026
Merge coinbase/cb-mpc master (cb60753) into Integra fork:
- PR coinbase#88: constant-time hardening, input validation, prove() error returns
- PR coinbase#84: ZK proof hardening, Fischlin parameter validation
- PR coinbase#75: FFI module extraction (mem_t → ffi::view)
- PR coinbase#71-76: benchmarks, refactoring, CI improvements

Integra customizations preserved:
- derive_child_key() asymmetric HD derivation
- MarshalBinary/UnmarshalECDSA2PCKey/DeriveChild Go API
- P2 server-first protocol support
- WASM build support

Post-merge adaptations:
- Custom CGO bindings updated to use ffi::view() pattern (2 call sites)
- WASM rebuilt with Emscripten (includes Node.js environment support)
- Dockerfile.wasm-build added for reproducible WASM builds

Validated: 19/19 C++ tests, 11/11 Go tests, interactive keygen+sign+derive
(Go P1 ↔ Go P2, WASM P2 ↔ Go P1)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants