Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fuzzing cfg rtt bug for uncompressed keys #322

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix fuzzing cfg rtt bug for uncompressed keys #322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 18 additions & 26 deletions secp256k1-sys/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,6 +680,15 @@ impl<T> CPtr for [T] {

#[cfg(fuzzing)]
mod fuzz_dummy {
/// Serialization logic:
/// If pk is created from sk keypair:
/// - It is serialized with prefix 0x02: sk || [0xaa;32]
/// 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
Copy link
Member

Choose a reason for hiding this comment

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

These types of patterns are exactly the kind of things fuzzers are good at finding. Why not just store an extra byte in memory?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

/// This leaves the room for collision between compressed and uncompressed pks,
Copy link
Member

Choose a reason for hiding this comment

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

Wait, maybe I'm confused by this. What is the collision here and why would it ever be an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Public keys don't, themselves, have any concept of "compressed" or not? That's purely a serialization-time difference, no?

/// but as such collisions should be improbable. 2/2^128
use super::*;
use core::sync::atomic::{AtomicUsize, Ordering};

Expand Down Expand Up @@ -770,26 +779,16 @@ mod fuzz_dummy {
assert_eq!(cx_flags & required_flags, required_flags);
}

/// Checks that pk != 0xffff...ffff and pk[1..32] == pk[33..64]
/// Checks that pk is valid
unsafe fn test_pk_validate(cx: *const Context,
pk: *const PublicKey) -> c_int {
check_context_flags(cx, 0);
if (*pk).0[1..32] != (*pk).0[33..64] ||
((*pk).0[32] != 0 && (*pk).0[32] != 0xff) ||
secp256k1_ec_seckey_verify(cx, (*pk).0[0..32].as_ptr()) == 0 {
if secp256k1_ec_seckey_verify(cx, (*pk).0[0..32].as_ptr()) == 0 {
0
} else {
1
}
}
unsafe fn test_cleanup_pk(pk: *mut PublicKey) {
(*pk).0[32..].copy_from_slice(&(*pk).0[..32]);
if (*pk).0[32] <= 0x7f {
(*pk).0[32] = 0;
} else {
(*pk).0[32] = 0xff;
}
}

// Pubkeys
pub unsafe fn secp256k1_ec_pubkey_parse(cx: *const Context, pk: *mut PublicKey,
Expand All @@ -802,11 +801,10 @@ mod fuzz_dummy {
0
} else {
ptr::copy(input.offset(1), (*pk).0[0..32].as_mut_ptr(), 32);
ptr::copy(input.offset(2), (*pk).0[33..64].as_mut_ptr(), 31);
if *input == 3 {
(*pk).0[32] = 0xff;
ptr::write_bytes((*pk).0[32..64].as_mut_ptr(), 0xbb, 32);
} else {
(*pk).0[32] = 0;
ptr::write_bytes((*pk).0[32..64].as_mut_ptr(), 0xaa, 32);
}
test_pk_validate(cx, pk)
}
Expand All @@ -816,7 +814,6 @@ mod fuzz_dummy {
0
} else {
ptr::copy(input.offset(1), (*pk).0.as_mut_ptr(), 64);
test_cleanup_pk(pk);
Copy link
Member

Choose a reason for hiding this comment

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

Can we instead just drop this line and keep the test_pk_validate and have it work?

Copy link
Member Author

Choose a reason for hiding this comment

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

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,

test_pk_validate(cx, pk)
}
},
Expand All @@ -833,10 +830,10 @@ mod fuzz_dummy {
assert_eq!(test_pk_validate(cx, pk), 1);
if compressed == SECP256K1_SER_COMPRESSED {
assert_eq!(*out_len, 33);
if (*pk).0[32] <= 0x7f {
*output = 2;
} else {
if &(*pk).0[32..64] == &[0xbb; 32] {
*output = 3;
} else {
*output = 2;
}
ptr::copy((*pk).0.as_ptr(), output.offset(1), 32);
} else if compressed == SECP256K1_SER_UNCOMPRESSED {
Expand All @@ -856,7 +853,7 @@ mod fuzz_dummy {
check_context_flags(cx, SECP256K1_START_SIGN);
if secp256k1_ec_seckey_verify(cx, sk) != 1 { return 0; }
ptr::copy(sk, (*pk).0[0..32].as_mut_ptr(), 32);
test_cleanup_pk(pk);
ptr::write_bytes((*pk).0[32..64].as_mut_ptr(), 0xaa, 32);
assert_eq!(test_pk_validate(cx, pk), 1);
1
}
Expand All @@ -866,7 +863,6 @@ mod fuzz_dummy {
check_context_flags(cx, 0);
assert_eq!(test_pk_validate(cx, pk), 1);
if secp256k1_ec_seckey_negate(cx, (*pk).0[..32].as_mut_ptr()) != 1 { return 0; }
test_cleanup_pk(pk);
assert_eq!(test_pk_validate(cx, pk), 1);
1
}
Expand All @@ -879,7 +875,6 @@ mod fuzz_dummy {
check_context_flags(cx, SECP256K1_START_VERIFY);
assert_eq!(test_pk_validate(cx, pk), 1);
if secp256k1_ec_seckey_tweak_add(cx, (*pk).0[..32].as_mut_ptr(), tweak) != 1 { return 0; }
test_cleanup_pk(pk);
assert_eq!(test_pk_validate(cx, pk), 1);
1
}
Expand All @@ -892,7 +887,6 @@ mod fuzz_dummy {
check_context_flags(cx, 0);
assert_eq!(test_pk_validate(cx, pk), 1);
if secp256k1_ec_seckey_tweak_mul(cx, (*pk).0[..32].as_mut_ptr(), tweak) != 1 { return 0; }
test_cleanup_pk(pk);
assert_eq!(test_pk_validate(cx, pk), 1);
1
}
Expand All @@ -911,7 +905,6 @@ mod fuzz_dummy {
return 0;
}
}
test_cleanup_pk(out);
assert_eq!(test_pk_validate(cx, out), 1);
1
}
Expand Down Expand Up @@ -1059,8 +1052,7 @@ mod fuzz_dummy {
check_context_flags(cx, 0);
let inslice = slice::from_raw_parts(input32, 32);
(*pubkey).0[..32].copy_from_slice(inslice);
(*pubkey).0[32..].copy_from_slice(inslice);
test_cleanup_pk(pubkey as *mut PublicKey);
ptr::write_bytes((*pubkey).0[32..64].as_mut_ptr(), 0xaa, 32);
test_pk_validate(cx, pubkey as *mut PublicKey)
}

Expand Down
11 changes: 10 additions & 1 deletion src/key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,10 +714,19 @@ mod test {
PublicKey::from_str("0218845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166").unwrap(),
pk
);
#[cfg(fuzzing)]
assert_eq!(
PublicKey::from_str("04\
18845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166\
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
).unwrap(),
pk
);
#[cfg(not(fuzzing))]
assert_eq!(
PublicKey::from_str("04\
18845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166\
84B84DB303A340CD7D6823EE88174747D12A67D2F8F2F9BA40846EE5EE7A44F6"
84b84db303a340cd7d6823ee88174747d12a67d2f8f2f9ba40846ee5ee7a44f6"
).unwrap(),
pk
);
Expand Down