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

Refactor of fuel-crypto for clarity around safety #346

Merged
merged 6 commits into from
Feb 22, 2023
Merged

Refactor of fuel-crypto for clarity around safety #346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c387305
Refactor of `fuel-crypto` for clarity around safety
mitchmindtree Feb 10, 2023
c1ec2c8
Re-add but deprecate Signature/Message from_bytes_unchecked for best-…
mitchmindtree Feb 10, 2023
0db0fa8
Update secp256k1 dep from 0.24 to 0.26
mitchmindtree Feb 13, 2023
a5b9ac9
Merge branch 'master' into mitchmindtree/fuel-crypto-safety
xgreenx Feb 15, 2023
bc8c732
Merge branch 'master' into mitchmindtree/fuel-crypto-safety
ControlCplusControlV Feb 20, 2023
f6e04de
Merge branch 'master' into mitchmindtree/fuel-crypto-safety
freesig Feb 22, 2023
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
2 changes: 1 addition & 1 deletion fuel-crypto/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ coins-bip39 = { version = "0.7", default-features = false, optional = true }
fuel-types = { workspace = true, default-features = false }
lazy_static = { version = "1.4", optional = true }
rand = { version = "0.8", default-features = false, optional = true }
secp256k1 = { version = "0.24", default-features = false, features = ["recovery"], optional = true }
secp256k1 = { version = "0.26", default-features = false, features = ["recovery"], optional = true }
serde = { version = "1.0", default-features = false, features = ["derive"], optional = true }
sha2 = { version = "0.10", default-features = false }
zeroize = { version = "1.5", features = ["derive"] }
Expand Down
3 changes: 2 additions & 1 deletion fuel-crypto/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ pub use error::Error;
pub use hasher::Hasher;
pub use keystore::Keystore;
pub use message::Message;
pub use mnemonic::FuelMnemonic;
#[cfg(all(feature = "std", feature = "random"))]
pub use mnemonic::generate_mnemonic_phrase;
pub use public::PublicKey;
pub use secret::SecretKey;
pub use signature::Signature;
Expand Down
63 changes: 26 additions & 37 deletions fuel-crypto/src/message.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
use crate::Hasher;

pub use fuel_types::Bytes32;

use core::fmt;
use core::ops::Deref;
pub use fuel_types::Bytes32;

/// Normalized signature message
#[derive(Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
Expand All @@ -12,54 +10,39 @@ use core::ops::Deref;
pub struct Message(Bytes32);

impl Message {
/// Memory length of the type
/// Memory length of the type in bytes.
pub const LEN: usize = Bytes32::LEN;

/// Normalize a message for signature
/// Normalize the given message by cryptographically hashing its content in
/// preparation for signing.
pub fn new<M>(message: M) -> Self
where
M: AsRef<[u8]>,
{
Self(Hasher::hash(message))
}

/// Add a conversion from arbitrary slices into owned
/// Construct a `Message` directly from its bytes.
///
/// # Safety
///
/// There is no guarantee the provided bytes will be the product of a cryptographically secure
/// hash. Using insecure messages might compromise the security of the signature.
pub unsafe fn from_bytes_unchecked(bytes: [u8; Self::LEN]) -> Self {
/// This constructor expects the given bytes to be a valid,
/// cryptographically hashed message. No hashing is performed.
pub fn from_bytes(bytes: [u8; Self::LEN]) -> Self {
Self(bytes.into())
}

/// Add a conversion from arbitrary slices into owned
///
/// # Safety
///
/// This function will not panic if the length of the slice is smaller than
/// `Self::LEN`. Instead, it will cause undefined behavior and read random
/// disowned bytes.
/// Construct a `Message` reference directly from a reference to its bytes.
///
/// This function extends the unsafety of [`Self::from_bytes_unchecked`].
pub unsafe fn from_slice_unchecked(bytes: &[u8]) -> Self {
Self(Bytes32::from_slice_unchecked(bytes))
/// This constructor expects the given bytes to be a valid,
/// cryptographically hashed message. No hashing is performed.
pub fn from_bytes_ref(bytes: &[u8; Self::LEN]) -> &Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this actually uphold the lifetime. If the bytes array is moved after this call and &Self is still being used, do we get a compile error? If not this should still be an unsafe function right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this actually uphold the lifetime.

Yes, in this case the lifetime of the reference in &Self is bound by the only input lifetime, i.e. the lifetime of bytes: &[u8; Self::LEN]. In this case, Rust elides the lifetime because there is only one possible valid lifetime for the output. Here is what the un-ellided signature would look like:

    pub fn from_bytes_ref<'a>(bytes: &'a [u8; Self::LEN]) -> &'a Self {

// TODO: Wrap this unsafe conversion safely in `fuel_types::Bytes32`.
unsafe { &*(bytes.as_ptr() as *const Self) }
}

/// Copy-free reference cast
///
/// # Safety
///
/// Inputs smaller than `Self::LEN` will cause undefined behavior.
///
/// This function extends the unsafety of [`Self::from_bytes_unchecked`].
pub unsafe fn as_ref_unchecked(bytes: &[u8]) -> &Self {
// The interpreter will frequently make references to keys and values using
// logically checked slices.
//
// This function will avoid unnecessary copy to owned slices for the interpreter
// access
&*(bytes.as_ptr() as *const Self)
/// Kept temporarily for backwards compatibility.
#[deprecated = "Use `Message::from_bytes` instead"]
pub fn from_bytes_unchecked(bytes: [u8; Self::LEN]) -> Self {
Self::from_bytes(bytes)
}
}

Expand All @@ -83,17 +66,23 @@ impl From<Message> for [u8; Message::LEN] {
}
}

impl From<Message> for Bytes32 {
fn from(s: Message) -> Self {
s.0
}
}

impl From<&Hasher> for Message {
fn from(hasher: &Hasher) -> Self {
// Safety: `Hasher` is a cryptographic hash
unsafe { Self::from_bytes_unchecked(*hasher.digest()) }
Self::from_bytes(*hasher.digest())
}
}

impl From<Hasher> for Message {
fn from(hasher: Hasher) -> Self {
// Safety: `Hasher` is a cryptographic hash
unsafe { Self::from_bytes_unchecked(*hasher.finalize()) }
Self::from_bytes(*hasher.finalize())
}
}

Expand Down
26 changes: 8 additions & 18 deletions fuel-crypto/src/mnemonic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,11 @@
/// FuelMnemonic is a simple mnemonic phrase generator.
pub struct FuelMnemonic;
#![cfg(all(feature = "std", feature = "random"))]

#[cfg(all(feature = "std", feature = "random"))]
mod use_std {
use super::FuelMnemonic;
use crate::Error;
use coins_bip39::{English, Mnemonic};
use crate::Error;
use coins_bip39::{English, Mnemonic};
use rand::Rng;

pub type W = English;

use rand::Rng;

impl FuelMnemonic {
/// Generates a random mnemonic phrase given a random number generator and
/// the number of words to generate, `count`.
pub fn generate_mnemonic_phrase<R: Rng>(rng: &mut R, count: usize) -> Result<String, Error> {
Ok(Mnemonic::<W>::new_with_count(rng, count)?.to_phrase()?)
}
}
/// Generates a random mnemonic phrase given a random number generator and
/// the number of words to generate, `count`.
pub fn generate_mnemonic_phrase<R: Rng>(rng: &mut R, count: usize) -> Result<String, Error> {
Ok(Mnemonic::<English>::new_with_count(rng, count)?.to_phrase()?)
}
108 changes: 23 additions & 85 deletions fuel-crypto/src/public.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
use crate::Hasher;

use fuel_types::{Bytes32, Bytes64};

use crate::hasher::Hasher;
use core::fmt;
use core::ops::Deref;
use fuel_types::{Bytes32, Bytes64};

/// Asymmetric public key
#[derive(Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
Expand All @@ -12,51 +10,18 @@ use core::ops::Deref;
pub struct PublicKey(Bytes64);

impl PublicKey {
/// Memory length of the type
/// Memory length of the type in bytes.
pub const LEN: usize = Bytes64::LEN;

/// Copy-free reference cast
/// Construct a `PublicKey` directly from its bytes.
///
/// # Safety
///
/// This function will not panic if the length of the slice is smaller than
/// `Self::LEN`. Instead, it will cause undefined behavior and read random
/// disowned bytes.
///
/// There is no guarantee the provided bytes will fit the curve.
pub unsafe fn as_ref_unchecked(bytes: &[u8]) -> &Self {
// The interpreter will frequently make references to keys and values using
// logically checked slices.
//
// This function will save unnecessary copy to owned slices for the interpreter
// access
&*(bytes.as_ptr() as *const Self)
}

/// Add a conversion from arbitrary slices into owned
///
/// # Safety
///
/// There is no guarantee the provided bytes will fit the curve. The curve
/// security can be checked with [`PublicKey::is_in_curve`].
pub unsafe fn from_bytes_unchecked(bytes: [u8; Self::LEN]) -> Self {
/// This constructor expects the given bytes to be a valid public key, and
/// does not check whether the public key is within the curve.
fn from_bytes_unchecked(bytes: [u8; Self::LEN]) -> Self {
Self(bytes.into())
}

/// Add a conversion from arbitrary slices into owned
///
/// # Safety
///
/// This function will not panic if the length of the slice is smaller than
/// `Self::LEN`. Instead, it will cause undefined behavior and read random
/// disowned bytes.
///
/// There is no guarantee the provided bytes will fit the curve.
pub unsafe fn from_slice_unchecked(bytes: &[u8]) -> Self {
Self(Bytes64::from_slice_unchecked(bytes))
}

/// Hash of the public key
/// Cryptographic hash of the public key.
pub fn hash(&self) -> Bytes32 {
Hasher::hash(self.as_ref())
}
Expand Down Expand Up @@ -117,52 +82,19 @@ mod use_std {
use super::*;
use crate::{Error, SecretKey};

use secp256k1::{Error as Secp256k1Error, PublicKey as Secp256k1PublicKey, Secp256k1};
use secp256k1::{
constants::UNCOMPRESSED_PUBLIC_KEY_SIZE, Error as Secp256k1Error, PublicKey as Secp256k1PublicKey, Secp256k1,
};

use core::borrow::Borrow;
use core::str;

const UNCOMPRESSED_PUBLIC_KEY_SIZE: usize = 65;

// Internal secp256k1 identifier for uncompressed point
//
// https://github.com/rust-bitcoin/rust-secp256k1/blob/ecb62612b57bf3aa8d8017d611d571f86bfdb5dd/secp256k1-sys/depend/secp256k1/include/secp256k1.h#L196
const SECP_UNCOMPRESSED_FLAG: u8 = 4;

impl PublicKey {
/// Check if the provided slice represents a public key that is in the
/// curve.
///
/// # Safety
///
/// This function extends the unsafety of
/// [`PublicKey::as_ref_unchecked`].
pub unsafe fn is_slice_in_curve_unchecked(slice: &[u8]) -> bool {
use secp256k1::ffi::{self, CPtr};

let public = Self::as_ref_unchecked(slice);

let mut public_with_flag = [0u8; UNCOMPRESSED_PUBLIC_KEY_SIZE];

public_with_flag[1..].copy_from_slice(public.as_ref());

// Safety: FFI call
let curve = ffi::secp256k1_ec_pubkey_parse(
ffi::secp256k1_context_no_precomp,
&mut ffi::PublicKey::new(),
public_with_flag.as_c_ptr(),
UNCOMPRESSED_PUBLIC_KEY_SIZE,
);

curve == 1
}

/// Check if the secret key representation is in the curve.
pub fn is_in_curve(&self) -> bool {
// Safety: struct is guaranteed to reference itself with correct len
unsafe { Self::is_slice_in_curve_unchecked(self.as_ref()) }
}

pub(crate) fn from_secp(pk: &Secp256k1PublicKey) -> PublicKey {
debug_assert_eq!(
UNCOMPRESSED_PUBLIC_KEY_SIZE,
Expand All @@ -174,10 +106,9 @@ mod use_std {
debug_assert_eq!(SECP_UNCOMPRESSED_FLAG, pk[0]);

// Ignore the first byte of the compression flag
let pk = &pk[1..];
let bytes = <[u8; Self::LEN]>::try_from(&pk[1..]).expect("compile-time bounds-checks");

// Safety: compile-time assertion of size correctness
unsafe { Self::from_slice_unchecked(pk) }
Self::from_bytes_unchecked(bytes)
}

pub(crate) fn _to_secp(&self) -> Result<Secp256k1PublicKey, Error> {
Expand All @@ -197,9 +128,7 @@ mod use_std {
type Error = Error;

fn try_from(b: Bytes64) -> Result<Self, Self::Error> {
let public = PublicKey(b);

public.is_in_curve().then_some(public).ok_or(Error::InvalidPublicKey)
public_key_bytes_valid(&b).map(|_| Self(b))
}
}

Expand Down Expand Up @@ -236,4 +165,13 @@ mod use_std {
.and_then(PublicKey::try_from)
}
}

/// Check if the public key byte representation is in the curve.
fn public_key_bytes_valid(bytes: &[u8; PublicKey::LEN]) -> Result<(), Error> {
let mut public_with_flag = [0u8; UNCOMPRESSED_PUBLIC_KEY_SIZE];
public_with_flag[1..].copy_from_slice(bytes);
secp256k1::PublicKey::from_slice(&public_with_flag)
.map(|_| ())
.map_err(|_| Error::InvalidPublicKey)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: The original is_slice_in_curve_unchecked function would also leave the first byte zeroed before invoking the secp256k1 pubkey parser like this, so I've carried over the behaviour in this function. However, it looks like we should possibly be setting the first byte to SECP_UNCOMPRESSED_FLAG? Should investigate or open an issue before landing.

}
}
Loading