Fix fuzzing cfg rtt bug for uncompressed keys#322
Fix fuzzing cfg rtt bug for uncompressed keys#322sanket1729 wants to merge 1 commit intorust-bitcoin:masterfrom
Conversation
sanket1729
force-pushed
the
cfg_fuzzing_fix
branch
from
67578cf to
e077830
Compare
| 0 | ||
| } else { | ||
| ptr::copy(input.offset(1), (*pk).0.as_mut_ptr(), 64); | ||
| test_cleanup_pk(pk); |
There was a problem hiding this comment.
Can we instead just drop this line and keep the test_pk_validate and have it work?
There was a problem hiding this comment.
I had to change how keys are stored and the function test_cleanup_pk was basically an empty function. See the comment on the top module for exact implementation,
sanket1729
force-pushed
the
cfg_fuzzing_fix
branch
from
e077830 to
05f4278
Compare
sanket1729
force-pushed
the
cfg_fuzzing_fix
branch
from
2d9d2c7 to
4b06f20
Compare
sanket1729
force-pushed
the
cfg_fuzzing_fix
branch
from
4b06f20 to
a8d48a1
Compare
sanket1729
changed the title
| /// If pk is created from from slice: | ||
| /// - 0x02||pk_bytes -> pk_bytes||[0xaa;32] | ||
| /// - 0x03||pk_bytes -> pk_bytes||[0xbb;32] | ||
| /// - 0x04||pk_bytes -> pk_bytes |
There was a problem hiding this comment.
These types of patterns are exactly the kind of things fuzzers are good at finding. Why not just store an extra byte in memory?
There was a problem hiding this comment.
Is exceeding the 64 bytes and using space beyond the 64 bytes a good idea?
Because we need all of 64 bytes to support full roundtrip of uncompressed keys.
There was a problem hiding this comment.
I had another related question, why do you need to serialize/parse functions for fuzzing anyway? Can we just not use regular logic for it.
For forging signatures, the keys are anyways created by secp256k1_ec_pubkey_create
There was a problem hiding this comment.
No, we cannot switch to requiring public keys be valid according to standard rules. Not only does it break the RL fuzzers (see #271) but its also really slow, which can make fuzzers untenable.
| /// - 0x02||pk_bytes -> pk_bytes||[0xaa;32] | ||
| /// - 0x03||pk_bytes -> pk_bytes||[0xbb;32] | ||
| /// - 0x04||pk_bytes -> pk_bytes | ||
| /// This leaves the room for collision between compressed and uncompressed pks, |
There was a problem hiding this comment.
Wait, maybe I'm confused by this. What is the collision here and why would it ever be an issue?
There was a problem hiding this comment.
Never mind, this is not an issue. But what I meant by collision is the following
let pk = PublicKey::from_str("0x04<32bytes><0xaa;32>"); // Should be considered uncompressed, but is considered as compressed.
There was a problem hiding this comment.
Public keys don't, themselves, have any concept of "compressed" or not? That's purely a serialization-time difference, no?
2 participants
I did not investigate too much into trying to figure out how the
encoding for simple crypto works. I made some simple changes(basically
disabled the checks for uncompressed keys) and at least it works for
uncompressed keys roundtrip.
#282 (comment)
This broke the rust-miniscript fuzzing downstream.