Skip to content

Commit

Permalink
Refactor of fuel-crypto for clarity around safety
Browse files Browse the repository at this point in the history 
This removes many of the unsafe constructors exposed in the `fuel-
crypto` API, some due to the implementations not actually being unsafe,
and some due to a revised implementation that safely encapsulates the
unsafe behaviour.

The `Message` and `Signature` `from_bytes` and `from_bytes_ref`
constructors have been documented to clarify expectations that they
expect signature or cryptographically hashed data respectively.

The `PublicKey` and `SecretKey` `from_bytes_unchecked` constructors have
been made private in order to ensure holding an instance of one of these
types guarantees their inner byte arrays at least satisfies the curve.

The original, custom curve-checking functions have been removed in
favour of using the secp256k1 library constructors directly which share
the same implementation but with an added range check that is likely to
get optimised out due to the use of const-sized arrays anyway.

Adds some missing `#[repr(transparent)]` decorators to `fuel-types`.

The `FuelMnemonic` type provided unnecessary indirection between the
user and the free function, and has been removed in favour of exposing
the function directly from the crate root.
  • Loading branch information
@mitchmindtree
mitchmindtree committed Feb 10, 2023
1 parent bc789b1 commit c387305
Show file tree
Hide file tree
Showing 11 changed files with 98 additions and 284 deletions.
3 changes: 2 additions & 1 deletion fuel-crypto/src/lib.rs
View file
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
61 changes: 22 additions & 39 deletions fuel-crypto/src/message.rs
View file
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,33 @@ 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
/// Construct a `Message` reference directly from a reference to its bytes.
///
/// 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.
///
/// 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))
}

/// 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)
/// 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 {
// TODO: Wrap this unsafe conversion safely in `fuel_types::Bytes32`.
unsafe { &*(bytes.as_ptr() as *const Self) }
}
}

Expand All @@ -83,17 +60,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
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
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)
}
}
Loading

0 comments on commit c387305

Please sign in to comment.