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.

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

Add memory protections for secret values #108

Merged
merged 13 commits into from
Apr 27, 2020
10 changes: 10 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@ keywords = ["cryptography", "proxy-re-encryption", "PRE", "ECC", "transform-encr
description = "A pure-Rust implementation of Transform Encryption, a Proxy Re-encryption scheme"
edition = "2018"

[target.'cfg(all(unix, not(target_arch = "wasm32")))'.dependencies]
libc = { version = "0.2" }

[target.'cfg(all(windows, not(target_arch = "wasm32")))'.dependencies]
winapi = { version = "0.3", features = [ "memoryapi", "sysinfoapi" ] }

[target.'cfg(all(any(target_os = "macos", target_os = "ios"), not(target_arch = "wasm32")))'.dependencies]
mach_o_sys = { version = "0.1" }

[dependencies]
#Explicit dependency so we can pass the wasm-bindgen flag to it
getrandom = {version = "~0.1.1", optional = true}
Expand Down Expand Up @@ -52,6 +61,7 @@ unstable = []
default = ["ed25519-dalek/u64_backend"]
wasm = ["ed25519-dalek/u32_backend", "clear_on_drop/no_cc", "getrandom/wasm-bindgen"]


[[bench]]
name = "api_benchmark"
harness = false
Expand Down
52 changes: 40 additions & 12 deletions src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use crate::internal::fp12elem::Fp12Elem;
pub use crate::internal::hashable::Hashable;
use crate::internal::hashable::Hashable32;
use crate::internal::homogeneouspoint::TwistedHPoint;
use crate::internal::memlock;
use crate::internal::pairing;
pub use crate::internal::rand_bytes::*;
use crate::internal::schnorr::{SchnorrSign, SchnorrSigning};
Expand Down Expand Up @@ -103,10 +104,12 @@ impl Plaintext {
/// Construct a Plaintext from raw bytes
pub fn new(bytes: [u8; Plaintext::ENCODED_SIZE_BYTES]) -> Plaintext {
// since new takes a fixed size array, we know it is safe to decode the resultant vector
Plaintext::from(
let p = Plaintext::from(
Fp12Elem::<Monty256>::decode(bytes.to_vec())
.expect("Developer error: did you change ENCODED_SIZE_BYTES?"),
)
);
memlock::mlock(&p);
clintfred marked this conversation as resolved.
Show resolved Hide resolved
p
}

new_from_slice!(Plaintext);
Expand All @@ -125,25 +128,30 @@ bytes_eq_and_hash!(Plaintext);

impl From<Fp12Elem<Monty256>> for Plaintext {
fn from(fp12: Fp12Elem<Monty256>) -> Self {
Plaintext {
let p = Plaintext {
bytes: fp12.to_bytes_fp256(),
_internal_fp12: fp12,
}
};
memlock::mlock(&p);
p
}
}

impl Default for Plaintext {
fn default() -> Self {
Plaintext {
let p = Plaintext {
bytes: [0u8; Plaintext::ENCODED_SIZE_BYTES],
_internal_fp12: Fp12Elem::default(),
}
};
memlock::mlock(&p);
clintfred marked this conversation as resolved.
Show resolved Hide resolved
p
}
}
impl Drop for Plaintext {
fn drop(&mut self) {
self.bytes.clear();
self._internal_fp12.clear();
memlock::munlock(&self);
}
}
impl BytesDecoder for Plaintext {
Expand Down Expand Up @@ -986,7 +994,7 @@ impl PublicKey {
}
}

#[derive(Default, Debug, Clone)]
#[derive(Default, Clone)]
pub struct PrivateKey {
bytes: [u8; PrivateKey::ENCODED_SIZE_BYTES],
_internal_key: internal::PrivateKey<Monty256>,
Expand All @@ -1001,10 +1009,12 @@ impl PrivateKey {

pub fn new(bytes: [u8; PrivateKey::ENCODED_SIZE_BYTES]) -> PrivateKey {
let internal_key = internal::PrivateKey::from_fp256(Fp256::from(bytes).to_monty());
PrivateKey {
let pk = PrivateKey {
bytes: internal_key.value.to_bytes_32(),
_internal_key: internal_key,
}
};
memlock::mlock(&pk);
pk
}

new_from_slice!(PrivateKey);
Expand Down Expand Up @@ -1042,20 +1052,38 @@ impl Hashable32 for PrivateKey {
}
}

/// Avoid accidental logging of secrets
impl fmt::Debug for PrivateKey {
Copy link
Member

Choose a reason for hiding this comment

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

This stuff seems to have a different goal than memory protection, and I think we should have a debug here that is distinguishing in at least some way. Probably just don't want to have a Display at all if the goal is to avoid accidental {}.

Copy link
Member

Choose a reason for hiding this comment

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

#110 is where we can discuss that further. This should be removed for now and we can keep this change non-breaking.

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.write_str("!!PRIVATE!!").map_err(|_| fmt::Error)
}
}

/// Avoid accidental logging of secrets
impl fmt::Display for PrivateKey {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.write_str("!!PRIVATE!!").map_err(|_| fmt::Error)
}
}

impl From<internal::PrivateKey<Monty256>> for PrivateKey {
fn from(internal_pk: internal::PrivateKey<Monty256>) -> Self {
PrivateKey {
let pk = PrivateKey {
bytes: internal_pk.value.to_bytes_32(),
_internal_key: internal_pk,
}
};
memlock::mlock(&pk);
pk
}
}

// use Drop to call clear on members of PrivateKey to zero memory before moving the stack pointer
impl Drop for PrivateKey {
fn drop(&mut self) {
self.bytes.clear();
self._internal_key.clear()
self._internal_key.clear();
// unlock after zeroing
memlock::munlock(&self)
}
}
new_bytes_type!(SchnorrSignature, 64);
Expand Down
35 changes: 24 additions & 11 deletions src/api_480.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use crate::internal::fp12elem::Fp12Elem;
pub use crate::internal::hashable::Hashable;
use crate::internal::hashable::Hashable60;
use crate::internal::homogeneouspoint::TwistedHPoint;
use crate::internal::memlock;
use crate::internal::pairing;
pub use crate::internal::rand_bytes::*;
use crate::internal::schnorr::{SchnorrSign, SchnorrSigning};
Expand Down Expand Up @@ -102,10 +103,12 @@ impl Plaintext {
/// Construct a Plaintext from raw bytes
pub fn new(bytes: [u8; Plaintext::ENCODED_SIZE_BYTES]) -> Plaintext {
// since new takes a fixed size array, we know it is safe to decode the resultant vector
Plaintext::from(
let p = Plaintext::from(
Fp12Elem::<Monty480>::decode(bytes.to_vec())
.expect("Developer error: did you change ENCODED_SIZE_BYTES?"),
)
);
memlock::mlock(&p);
p
}

new_from_slice!(Plaintext);
Expand All @@ -124,25 +127,30 @@ bytes_eq_and_hash!(Plaintext);

impl From<Fp12Elem<Monty480>> for Plaintext {
fn from(fp12: Fp12Elem<Monty480>) -> Self {
Plaintext {
let p = Plaintext {
bytes: fp12.to_bytes_fp480(),
_internal_fp12: fp12,
}
};
memlock::mlock(&p);
p
}
}

impl Default for Plaintext {
fn default() -> Self {
Plaintext {
let p = Plaintext {
bytes: [0u8; Plaintext::ENCODED_SIZE_BYTES],
_internal_fp12: Fp12Elem::default(),
}
};
memlock::mlock(&p);
p
}
}
impl Drop for Plaintext {
fn drop(&mut self) {
self.bytes.clear();
self._internal_fp12.clear();
memlock::munlock(&self);
}
}
impl BytesDecoder for Plaintext {
Expand Down Expand Up @@ -1038,10 +1046,12 @@ impl PrivateKey {

pub fn new(bytes: [u8; PrivateKey::ENCODED_SIZE_BYTES]) -> PrivateKey {
let internal_key = internal::PrivateKey::from_fp480(Fp480::from(bytes).to_monty());
PrivateKey {
let pk = PrivateKey {
bytes: SixtyBytes(internal_key.value.to_bytes_60()),
_internal_key: internal_key,
}
};
memlock::mlock(&pk);
pk
}

new_from_slice!(PrivateKey);
Expand Down Expand Up @@ -1087,18 +1097,21 @@ impl Hashable for PrivateKey {

impl From<internal::PrivateKey<Monty480>> for PrivateKey {
fn from(internal_pk: internal::PrivateKey<Monty480>) -> Self {
PrivateKey {
let pk = PrivateKey {
bytes: SixtyBytes(internal_pk.value.to_bytes_60()),
_internal_key: internal_pk,
}
};
memlock::mlock(&pk);
pk
}
}

// use Drop to call clear on members of PrivateKey to zero memory before moving the stack pointer
impl Drop for PrivateKey {
fn drop(&mut self) {
self.bytes.clear();
self._internal_key.clear()
self._internal_key.clear();
memlock::munlock(&self)
}
}
new_bytes_type!(SchnorrSignature, 120);
Expand Down
12 changes: 9 additions & 3 deletions src/internal/ed25519.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::api_common::RecryptErr;
use crate::internal::hashable::Hashable;
use crate::internal::memlock;
use crate::internal::ByteVector;
use crate::internal::{array_split_64, take_lock};
use clear_on_drop::clear::Clear;
Expand Down Expand Up @@ -66,7 +67,9 @@ impl SigningKeypair {
let keypair = ed25519_dalek::Keypair::generate::<CR>(&mut *take_lock(&rng));

//Unchecked is safe because the public is on the curve and the size is statically guaranteed.
SigningKeypair::new_unchecked(keypair.to_bytes())
let skp = SigningKeypair::new_unchecked(keypair.to_bytes());
memlock::mlock_slice(&skp.bytes[..]);
clintfred marked this conversation as resolved.
Show resolved Hide resolved
skp
}
///
///Create a SigningKeypair from a byte array slice. If the array is not the right length or if the public
Expand Down Expand Up @@ -105,7 +108,9 @@ impl SigningKeypair {
}

pub(crate) fn new_unchecked(bytes: [u8; 64]) -> SigningKeypair {
SigningKeypair { bytes }
let skp = SigningKeypair { bytes };
memlock::mlock_slice(&skp.bytes[..]);
skp
}

///Get the public_key portion of this SigningKeypair.
Expand Down Expand Up @@ -137,7 +142,8 @@ impl<'a> From<&'a SigningKeypair> for PublicSigningKey {

impl Drop for SigningKeypair {
fn drop(&mut self) {
self.bytes.clear()
self.bytes.clear();
memlock::munlock_slice(&self.bytes[..])
}
}
new_bytes_type!(Ed25519Signature, 64);
Expand Down
Loading