Make ML-DSA signature decoding follow the spec #895
Conversation
|
Heh, and apparently I have some clippy clean-up to do as well! |
ml-dsa/src/lib.rs
Outdated
| fn many_round_trip_test<P>() | ||
| where | ||
| P: MlDsaParams, | ||
| { | ||
| use rand::Rng; | ||
|
|
||
| const ITERATIONS: usize = 1000; | ||
|
|
||
| let mut rng = rand::thread_rng(); | ||
| let mut seed = B32::default(); | ||
|
|
||
| for _i in 0..ITERATIONS { | ||
| let seed_data: &mut [u8] = seed.as_mut(); | ||
| rng.fill(seed_data); | ||
|
|
||
| let kp = P::key_gen_internal(&seed); | ||
| let sk = kp.signing_key; | ||
| let vk = kp.verifying_key; | ||
|
|
||
| let M = b"Hello world"; | ||
| let rnd = Array([0u8; 32]); | ||
| let sig = sk.sign_internal(&[M], &rnd); | ||
|
|
||
| let sig_enc = sig.encode(); | ||
| let sig_dec = Signature::<P>::decode(&sig_enc).unwrap(); | ||
|
|
||
| assert_eq!(sig_dec, sig); | ||
| assert!(vk.verify_internal(&[M], &sig)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Perhaps consider proptest for these sorts of tests.
You can look at https://github.com/RustCrypto/crypto-bigint/tree/master/tests for some examples.
(Or I can try to convert them to proptests as a followup)
There was a problem hiding this comment.
Note: tried doing this myself and it's a tad tricky due to the use of generics
There was a problem hiding this comment.
Okay, found an okay enough solution and opened a PR to add proptests: #896
It also required adding a Debug impl for Signature as this PR does also.
It was able to find some failures right away, though I did see many_round_trip_test find failures with this PR's current state that the proptests did not (but perhaps they just need to be run more).
|
Sorry, didn't realize this was ready to test already. Still having problems, but with a new error: thread 'ownedchain::tests::test_ownedchainstore' panicked at /home/jderose/.cargo/git/checkouts/signatures-7e4d4cb41d25b54f/92d2a82/ml-dsa/src/hint.rs:30:9: |
|
Yep, that looks like the same error I'm seeing locally. Still working on that one. |
|
@json420 Oddly, when I went to fix that remaining |
|
@bifurcation Huh, I hit what I pasted right away when I first ran my tests against your branch... but now I'm not getting them anymore. I'll test further. |
|
Ok, I can still hit the error (sometimes) in my tests: thread 'ownedchain::tests::test_ocs_secret_to_public' panicked at /home/jderose/.cargo/git/checkouts/signatures-7e4d4cb41d25b54f/f1e4e30/ml-dsa/src/hint.rs:30:9: I'll work on narrowing it down to known inputs. |
|
@bifurcation I still don't have reproduce-able values, but I have a theory: "internal error: entered unreachable code" is only being hit on legitimately invalid signature values. I've ran through millions of valid signatures and haven't hit this, but one difference in my zebrachain tests is that I'm testing bit-flip permutations, which in some cases should produce a signature that shouldn't decode (right?). |
|
Perhaps you can change the |
|
@json420 - Bit flips could certainly cause the decoding to fail, but that @tarcieri - Should we consider this |
|
Actually @tarcieri please hold, it started failing for me again. |
|
This test case reliably fails: #[test]
fn bad_seed() {
const BAD_SEED: &[u8] = &[
223, 116, 86, 28, 89, 196, 31, 159, 108, 178, 58, 1, 14, 198, 242, 131, 243, 77, 188,
21, 25, 108, 151, 27, 232, 208, 147, 229, 245, 113, 178, 114,
];
let seed = B32::try_from(BAD_SEED).unwrap();
let kp = MlDsa87::key_gen_internal(&seed);
let sk = kp.signing_key;
let vk = kp.verifying_key;
let M = b"Hello world";
let rnd = Array([0u8; 32]);
let sig = sk.sign_internal(&[M], &rnd);
let sig_enc = sig.encode();
let sig_dec = Signature::<MlDsa87>::decode(&sig_enc).unwrap();
let _ver = vk.verify_internal(&[M], &sig); // <-- this will panic
} |
|
@bifurcation yup, that one fails for me too. Also fails against git master.. |
|
OK, I believe the latest commit fixes this secondary issue. As with the first issue, we were being too strict in our assumptions about what ranges of numbers would appear in a given setting. The Once again, the actual fix is one character, changing @json420 - Could you confirm that this fix looks good to you? |
|
@bifurcation I haven't been able to get any failures since your last change. Thanks! |
|
OK @tarcieri I think this is ready to merge. |
Fixes #894
This PR also adds a test that runs many random signatures. I confirmed locally that this produced a failure rate of 0.4-1.1% for ML-DSA-65 and ML-DSA-87, all due to signature decoding, and all for the reasons discussed below. I never saw any failures for ML-DSA-44, but that seems plausible: The failure condition arises when the high-order bits of the$\gamma_2 / 2$ is roughly a factor of 2.75 lower for ML-DSA-44 than the other variants.
rvector don't change, and the thresholdIt appears that the root of the problem is a one-character fix, allowing repeated values in the monotonicity check in
hint.rs. This makes it so that the tests usually pass. Marking this PR as draft because that same test fails intermittently, reportinginternal error: entered unreachable codeathint.rs:30. I'll take a look at that tomorrow.