fix: harden protocol and crypto validation against malformed peer input - #135
Conversation
hsiuhsiu
commented
Aug 27, 2026
hsiuhsiu
commented
- Propagate unpack errors from multi-set group messages instead of discarding them, so a malformed inner broadcast payload fails group_message rather than being silently accepted.
- Validate ECDSA-MP round-3 signing broadcasts before hashing them into the transcript view, so a malformed proof is rejected before it can influence the transcript.
- Verify in TDH2 combine/combine_additive that the supplied public shares actually reconstruct to the claimed public key, and reject duplicate partial-decryption role IDs, closing a path to combine shares and decryptions from mismatched key contexts.
- Reject malformed message lengths in the base PVW OT protocol instead of processing undersized/oversized inputs and outputs.
- Propagate unpack errors from multi-set group messages instead of discarding them, so a malformed inner broadcast payload fails group_message rather than being silently accepted. - Validate ECDSA-MP round-3 signing broadcasts before hashing them into the transcript view, so a malformed proof is rejected before it can influence the transcript. - Verify in TDH2 combine/combine_additive that the supplied public shares actually reconstruct to the claimed public key, and reject duplicate partial-decryption role IDs, closing a path to combine shares and decryptions from mismatched key contexts. - Reject malformed message lengths in the base PVW OT protocol instead of processing undersized/oversized inputs and outputs.
✅ Heimdall Review Status
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| TEST(MPCJob, MultiSetGroupMessageRejectsMalformedInnerBroadcast) { | ||
| constexpr int n = 2; | ||
| constexpr int malicious = 1; |
Rob1Ham
left a comment
Rob1Ham
left a comment
There was a problem hiding this comment.
I reviewed the changes at 310805a807e3735ba53c21752d98961fa65e674e and have no additional feedback on the implementation itself.
I left two non-blocking observations regarding Docker/OpenSSL path consistency and regression-test maintainability.
| no-quic no-rc2 no-rc4 no-rc5 no-rfc3779 no-scrypt no-sctp no-seed no-siphash no-sm2 no-sm3 no-sm4 no-sock no-srtp no-srp \ | ||
| no-ssl-trace no-ssl3 no-stdio no-tests no-tls no-ts no-unit-test no-uplink no-whirlpool no-zlib \ | ||
| --prefix="${CBMPC_OPENSSL_ROOT:-/usr/local/opt/openssl@3.6.3}" --libdir=lib64 | ||
| --prefix="${CBMPC_OPENSSL_ROOT:-/usr/local/opt/openssl@3.6.4}" --libdir=lib64 |
There was a problem hiding this comment.
The script now installs OpenSSL under openssl@3.6.4, while Dockerfile:40-42 still creates the /usr/local compatibility links against openssl@3.6.1. Is that divergence intentional?
The CMake paths use the new 3.6.4 prefix directly, but consumers relying on the conventional /usr/local include and library paths would still resolve through the older targets. If this is not intentional, could the Dockerfile links be updated as part of the version bump?
| constexpr uint8_t kSecp256k1CurveCodeHi = 0x02; | ||
| constexpr uint8_t kSecp256k1CurveCodeLo = 0xca; | ||
| for (int i = 0; i + 1 < msg.size(); ++i) { | ||
| if (msg[i] == kSecp256k1CurveCodeHi && msg[i + 1] == kSecp256k1CurveCodeLo) { |
There was a problem hiding this comment.
Would it be possible to target a decoded field directly rather than mutating the first raw 0x02ca occurrence? The current approach may be sensitive to unrelated serialization changes and could modify a different field if that byte sequence occurs earlier in the message. A structured mutation would make the regression test more deterministic.
|
Review Error for Rob1Ham @ 2026-08-27 14:57:36 UTC |
0ed5492
Addressed them. Thanks! |
6 participants
