Secret/public key wrapper guidelines

[semenov-vladyslav]

SecretKey/PublicKey wrappers guidelines

This is a guideline to implementing wrappers for secret/public key types. It defines the following:

• constants
• methods, naming convention, serialization
• traits
• companion crypto operation related types
• security issues

Conversion/serialization methods naming/typing rules

• try_ -- prefix used with methods that can fail, usually try_from_xxx; must return Result<T>;
• to_/from_/as_ -- reflects operation; to_ returns non-ref type, as_ returns ref type; must not return Result<T>;
• bytes/ref/slice -- reflects operand type:
  • bytes methods must accept or return [u8; N] type;
  • ref methods must accept or return &[u8; N] type;
  • slice methods must accept or return &[u8] or T: AsRef<[u8]> type;
• &[u8; N] should be preferred to &[u8] as it retains accepted array length information.

Secret keys

Constants

• pub const SECRET_KEY_LEN: usize -- length in raw/serialized bytes.

Recommended traits

• Zeroize -- if internal/wrapped type does not implement it already;
• Drop -- perform zeroize on drop, can be derived with #[zeroize(drop)];
• Into/TryInto, From/TryFrom -- conversions to/from bytes:
  • impl From<[u8; SECRET_KEY_LEN]> for SecretKey (or impl From<&[u8; SECRET_KEY_LEN]> for SecretKey?) or TryFrom if secret key bytes need verification that can fail -- conversion from raw/serialized secret bytes;
  • impl TryFrom<&[u8]> for SecretKey -- conversion from a slice, can fail as slice can have bad length;
  • impl From<SecretKey> for [u8; SECRET_KEY_LEN] -- conversion to a serialized byte array;

Not-recommended traits

• Clone -- secret keys should really not be cloned/copied, a clone/copy can be reused/misused which might lead to security threat. Instead, new keys should be "derived" from master-key. In this case key system becomes non-trivial and should be documented. One example of such key system is slip10.
• Copy -- can't be implemented without implementing Clone trait.
• Debug -- makes little sense and only in test/debug builds;
• PartialEq/Eq/Ord -- secret keys may not have a reasonable equality, there's no reasonable use-case where secret key equality comparison is justified. Default PartialEq may not be safe (non-constant time, might lead to side-channel attacks), it's advised to use constant time equality comparison from subtle crate.
• Hash -- secret keys should not be used as keys in associative containers. Default Hash instance might leak information about secret key bits. If the Hash instance is justified it should be cryptographically secure: key derivation of MAC mechanism should be used.
• AsRef<[u8]> -- access to raw/serialized secret key bytes (not internal representation as scalar bytes) via as_ref is discouraged to minimize API;
• signature::Signer -- signing operation, discouraged to avoid additional dependency.

Secret key specific methods

• pub fn generate() -> Result<Self> -- generate a new random key with built-in rng;
• pub fn generate_with<R: RngCore + CryptoRng>(rng: &mut R) -> Self -- generate a new random key with provided rng;
• pub fn public_key(&self) -> PublicKey -- get public key;

Serialization/conversion methods

• pub fn to_bytes(&self) -> [u8; SECRET_KEY_LEN] discouraged;
• pub fn as_bytes(&self) -> &[u8; SECRET_KEY_LEN] or pub fn as_slice(&self) -> &[u8];
• pub fn from_bytes(bytes: [u8; SECRET_KEY_LEN]) -> Self or pub fn try_from_bytes(bytes: [u8; SECRET_KEY_LEN]) -> Result<Self>;
• pub fn try_from_slice(slice: &[u8]) -> Result<Self> or pub fn try_from_slice<T: AsRef<[u8]>>(slice: T) -> Result<Self>;

Security

Secret key bytes must not leak. To ensure that many implementations implement Zeroize trait. However, fFunction arguments, result values, and local arrays might leak secret key bytes:

• fn from_slice(slice: &[u8]) -> SecretKey does not guarantee that the caller will clear slice;
• fn from_bytes(bytes: [u8; SECRET_KEY_LEN]) -> SecretKey does not guarantee that the compiler will actually move (and will not copy bytes), or clear bytes after it's been used, or won't remove such cleanup;
• fn generate<R>(rng: &mut R) -> SecretKey implemented via let mut bytes = [0_u8; SECRET_KEY_LEN] does not guarantee that local bytes will be cleared;
• fn to_bytes(&self) -> [u8; SECRET_KEY_LEN] does not guarantee that the caller will clear result array;

Example 1:

#[derive(Zeroize)]
#[zeroize(drop)]
struct SecretKey([u8; 32]);

fn generate() -> SecretKey {
    let mut bytes = [0_u8; 32];
    use rand::RngCore;
    rand::thread_rng().fill_bytes(&mut bytes[..]);
    SecretKey(bytes)
}

This implementation leaks secret key bytes in release build (ie. compiler can't optimize away copying).

Example 2:

#[derive(Zeroize)]
#[zeroize(drop)]
struct SecretKey([u8; 32]);

fn generate() -> SecretKey {
    let mut sk = SecretKey([0_u8; 32]);
    use rand::RngCore;
    rand::thread_rng().fill_bytes(&mut sk.0[..]);
    sk
}

Here compiler does perform move.

Example 3: crypto::signatures::ed25519::SecretKey::generate() (built in release mode) leaves behind 4 additional copies of the secret key in stack besides the secret key itself. ed25519_zebra::SigningKey does not perform zeroize when dropping secret key.

Extent of the problem is not clear: whether it's possible or not to exploit such leaks. The common recommendation is to keep only one copy (including implicitly created by compiler) of a secret key.

Solution 0: Do nothing and claim it's not a potential vulnerability (not true, it is!).

Solution 1: Replace secret bytes type [u8; N] with #[derive(Zeroize)] pub struct SecretBytes<const N: usize>(pub [u8; N]) in function arguments, local functions, result types. Obviously, this is a breaking and inconvenient change.

Solution 2: Make array cleanup responsibility of the caller. Encourage use of .zeroize(), make code review/audit of potentially hasardous code. Not clear how to go about function arguments and return values:

fn from_bytes(bytes: [u8; SECRET_KEY_LEN]) -> SecretKey;
fn to_bytes(&self) -> [u8; SECRET_KEY_LEN];

Solution 3: Forbid use of [u8; N] type for secret bytes in function arguments and return values. Make array cleanup responsibility of the caller. Encourage use of .zeroize(), make code review/audit of potentially hasardous code:

fn from_bytes(bytes: &[u8; SECRET_KEY_LEN]) -> SecretKey;
fn as_bytes(&self) -> &[u8; SECRET_KEY_LEN];

Public keys

Constants

• pub const PUBLIC_KEY_LEN: usize -- length in raw/serialized bytes.

Traits

• Into/TryInto, From/TryFrom -- conversions to/from bytes:
  • impl TryFrom<[u8; PUBLIC_KEY_LEN]> for PublicKey (or impl TryFrom<&[u8; PUBLIC_KEY_LEN]> for PublicKey?) public key must be verified, not all bytestring represent a valid public key -- conversion from raw/serialized bytes;
  • impl TryFrom<&[u8]> for PublicKey -- conversion from a slice, can fail as slice can have bad length;
  • impl From<PublicKey> for [u8; SECRET_KEY_LEN] -- conversion to a serialized byte array;
• Clone, Copy, Debug, PartialEq/Eq/Ord, Hash, AsRef<[u8]> -- the usual interoperability traits;
• signature::Verifier -- discouraged in order to minimized dependencies.

Security

• Public key is always correct; not necessarily trusted.
• Implementations must not modify public key bytes during conversion/validation, public key bytes can be used as a unique identifier.

Additional types

Definitions of additional types such as Signature, SharedKey, etc. is outside of scope of the current guide.

Template

use core::convert::TryFrom;
use zeroize::Zeroize;

pub const SECRET_KEY_LEN: usize = 32;
pub const PUBLIC_KEY_LEN: usize = 32;

#[derive(Zeroize)]
#[zeroize(drop)]
pub struct SecretKey([u8; SECRET_KEY_LEN]);

impl SecretKey {
    #[cfg(feature = "random")]
    #[cfg_attr(docsrs, doc(cfg(feature = "random")))]
    pub fn generate() -> crate::Result<Self> {
        let mut sk = Self([0_u8; SECRET_KEY_LEN]);
        crate::utils::rand::fill(&mut sk.0[..])?;
        Ok(sk)
    }

    #[cfg(feature = "rand")]
    pub fn generate_with<R: rand::CryptoRng + rand::RngCore>(rng: &mut R) -> Self {
        let mut sk = Self([0_u8; SECRET_KEY_LEN]);
        rng.fill_bytes(&mut sk.0);
        sk
    }

    pub fn public_key(&self) -> PublicKey {
        PublicKey(self.0)
    }

    pub fn as_bytes(&self) -> &[u8; SECRET_KEY_LEN] {
        &self.0
    }
    pub fn from_bytes(bytes: [u8; SECRET_KEY_LEN]) -> Self {
        Self(bytes)
    }
    pub fn try_from_slice(slice: &[u8]) -> crate::Result<Self> {
        if slice.len() != SECRET_KEY_LEN {
            Err(crate::Error::ConvertError {
                from: "slice",
                to: "secret key wrapper",
            })
        } else {
            let mut sk = Self([0_u8; SECRET_KEY_LEN]);
            sk.0.copy_from_slice(slice);
            Ok(sk)
        }
    }
}

impl From<[u8; SECRET_KEY_LEN]> for SecretKey {
    fn from(bytes: [u8; SECRET_KEY_LEN]) -> Self {
        Self::from_bytes(bytes)
    }
}

impl TryFrom<&[u8]> for SecretKey {
    type Error = crate::Error;
    fn try_from(slice: &[u8]) -> Result<Self, crate::Error> {
        Self::try_from_slice(slice)
    }
}

impl From<SecretKey> for [u8; SECRET_KEY_LEN] {
    fn from(sk: SecretKey) -> Self {
        *sk.as_bytes()
    }
}

impl AsRef<[u8]> for SecretKey {
    fn as_ref(&self) -> &[u8] {
        &self.as_bytes()[..]
    }
}

#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub struct PublicKey([u8; PUBLIC_KEY_LEN]);

impl PublicKey {
    pub fn as_bytes(&self) -> &[u8; PUBLIC_KEY_LEN] {
        &self.0
    }
    pub fn try_from_bytes(bytes: [u8; PUBLIC_KEY_LEN]) -> crate::Result<Self> {
        //todo: validate public key
        Ok(Self(bytes))
    }
    pub fn try_from_slice(slice: &[u8]) -> crate::Result<Self> {
        if slice.len() != PUBLIC_KEY_LEN {
            Err(crate::Error::ConvertError {
                from: "slice",
                to: "public key wrapper",
            })
        } else {
            let mut pk = Self([0_u8; PUBLIC_KEY_LEN]);
            pk.0.copy_from_slice(slice);
            Ok(pk)
        }
    }
}

impl From<&SecretKey> for PublicKey {
    fn from(sk: &SecretKey) -> Self {
        sk.public_key()
    }
}

impl TryFrom<[u8; PUBLIC_KEY_LEN]> for PublicKey {
    type Error = crate::Error;
    fn try_from(bytes: [u8; PUBLIC_KEY_LEN]) -> Result<Self, crate::Error> {
        Self::try_from_bytes(bytes)
    }
}

impl TryFrom<&[u8]> for PublicKey {
    type Error = crate::Error;
    fn try_from(slice: &[u8]) -> Result<Self, crate::Error> {
        Self::try_from_slice(slice)
    }
}

impl From<PublicKey> for [u8; PUBLIC_KEY_LEN] {
    fn from(sk: PublicKey) -> Self {
        *sk.as_bytes()
    }
}

impl AsRef<[u8]> for PublicKey {
    fn as_ref(&self) -> &[u8] {
        &self.as_bytes()[..]
    }
}