From 88163368809ff35916d6e665971b2590f633d00a Mon Sep 17 00:00:00 2001 From: Jernej Kos Date: Fri, 10 Jan 2020 12:14:45 +0100 Subject: [PATCH 1/2] go/worker/keymanager: Don't close the client runtimes quit channel Closing the channel can cause the worker to panic during shutdown if the Quit() channel of the client runtime watcher's node instance is notified before the worker's stopCh and the goroutine tries to propagate the quit event -- writing into a closed channel and causing a panic. --- go/worker/keymanager/worker.go | 1 - 1 file changed, 1 deletion(-) diff --git a/go/worker/keymanager/worker.go b/go/worker/keymanager/worker.go index b5aaab8db06..f832f8eab02 100644 --- a/go/worker/keymanager/worker.go +++ b/go/worker/keymanager/worker.go @@ -432,7 +432,6 @@ func (w *Worker) worker() { // are using us as a key manager. clientRuntimes := make(map[common.Namespace]*clientRuntimeWatcher) clientRuntimesQuitCh := make(chan *clientRuntimeWatcher) - defer close(clientRuntimesQuitCh) rtCh, rtSub, err := w.commonWorker.Registry.WatchRuntimes(w.ctx) if err != nil { w.logger.Error("failed to watch runtimes", From 890e4ec026611fde10849b0c01ce3713c349da9a Mon Sep 17 00:00:00 2001 From: Jernej Kos Date: Fri, 10 Jan 2020 13:27:22 +0100 Subject: [PATCH 2/2] runtime: Extract common sealing code --- .changelog/2537.trivial.md | 1 + keymanager-runtime/src/policy.rs | 49 +++-------- runtime/src/common/crypto/signature.rs | 42 ++++++++++ runtime/src/common/sgx/egetkey.rs | 2 +- runtime/src/common/sgx/mod.rs | 1 + runtime/src/common/sgx/seal.rs | 107 +++++++++++++++++++++++++ 6 files changed, 161 insertions(+), 41 deletions(-) create mode 100644 .changelog/2537.trivial.md create mode 100644 runtime/src/common/sgx/seal.rs diff --git a/.changelog/2537.trivial.md b/.changelog/2537.trivial.md new file mode 100644 index 00000000000..2f23fdb890f --- /dev/null +++ b/.changelog/2537.trivial.md @@ -0,0 +1 @@ +runtime: Extract common sealing code. diff --git a/keymanager-runtime/src/policy.rs b/keymanager-runtime/src/policy.rs index 781dfa8fcdb..39615175068 100644 --- a/keymanager-runtime/src/policy.rs +++ b/keymanager-runtime/src/policy.rs @@ -6,18 +6,18 @@ use std::{ use failure::Fallible; use lazy_static::lazy_static; -use rand::{rngs::OsRng, Rng}; use sgx_isa::Keypolicy; use tiny_keccak::sha3_256; -use zeroize::Zeroize; use oasis_core_keymanager_api::*; use oasis_core_runtime::{ common::{ cbor, - crypto::mrae::deoxysii::{DeoxysII, NONCE_SIZE, TAG_SIZE}, runtime::RuntimeId, - sgx::{avr::EnclaveIdentity, egetkey::egetkey}, + sgx::{ + avr::EnclaveIdentity, + seal::{seal, unseal}, + }, }, rpc::Context as RpcContext, runtime_context, @@ -175,37 +175,14 @@ impl Policy { }) .unwrap(); - let ct_len = ciphertext.len(); - if ct_len == 0 { - return None; - } else if ct_len < TAG_SIZE + NONCE_SIZE { - panic!("persisted state is corrupted, invalid size"); - } - let ct_len = ct_len - NONCE_SIZE; - - // Split the ciphertext || tag || nonce. - let mut nonce = [0u8; NONCE_SIZE]; - nonce.copy_from_slice(&ciphertext[ct_len..]); - let ciphertext = &ciphertext[..ct_len]; - - let d2 = Self::new_d2(); - let plaintext = d2 - .open(&nonce, ciphertext.to_vec(), vec![]) - .expect("persisted state is corrupted"); - - // Deserialization failures are fatal, because it is state corruption. - Some(CachedPolicy::parse(&plaintext).expect("failed to deserialize persisted policy")) + unseal(Keypolicy::MRENCLAVE, &POLICY_SEAL_CONTEXT, &ciphertext).map(|plaintext| { + // Deserialization failures are fatal, because it is state corruption. + CachedPolicy::parse(&plaintext).expect("failed to deserialize persisted policy") + }) } fn save_raw_policy(raw_policy: &Vec) { - let mut rng = OsRng::new().unwrap(); - - // Encrypt the raw policy. - let mut nonce = [0u8; NONCE_SIZE]; - rng.fill(&mut nonce); - let d2 = Self::new_d2(); - let mut ciphertext = d2.seal(&nonce, raw_policy.to_vec(), vec![]); - ciphertext.extend_from_slice(&nonce); + let ciphertext = seal(Keypolicy::MRENCLAVE, &POLICY_SEAL_CONTEXT, &raw_policy); // Persist the encrypted master secret. StorageContext::with_current(|_mkvs, untrusted_local| { @@ -213,14 +190,6 @@ impl Policy { }) .expect("failed to persist master secret"); } - - fn new_d2() -> DeoxysII { - let mut seal_key = egetkey(Keypolicy::MRENCLAVE, &POLICY_SEAL_CONTEXT); - let d2 = DeoxysII::new(&seal_key); - seal_key.zeroize(); - - d2 - } } #[derive(Clone, Debug)] diff --git a/runtime/src/common/crypto/signature.rs b/runtime/src/common/crypto/signature.rs index 7c9b01d6765..ad4147a26d9 100644 --- a/runtime/src/common/crypto/signature.rs +++ b/runtime/src/common/crypto/signature.rs @@ -6,6 +6,7 @@ use ed25519_dalek; use failure::Fallible; use rand::rngs::OsRng; use serde_derive::{Deserialize, Serialize}; +use zeroize::Zeroize; use super::hash::Hash; @@ -40,6 +41,27 @@ impl PrivateKey { PrivateKey(ed25519_dalek::Keypair::generate(&mut rng)) } + /// Convert this private key into bytes. + pub fn to_bytes(&self) -> Vec { + let mut bytes = self.0.secret.to_bytes(); + let bvec = (&bytes).to_vec(); + bytes.zeroize(); + bvec + } + + /// Construct a private key from bytes returned by `to_bytes`. + /// + /// # Panics + /// + /// This method will panic in case the passed bytes do not have the correct length. + pub fn from_bytes(mut bytes: Vec) -> PrivateKey { + let secret = ed25519_dalek::SecretKey::from_bytes(&bytes).unwrap(); + bytes.zeroize(); + let public = (&secret).into(); + + PrivateKey(ed25519_dalek::Keypair { secret, public }) + } + /// Generate a new private key from a test key seed. pub fn from_test_seed(seed: String) -> Self { let seed = Hash::digest_bytes(seed.as_bytes()); @@ -193,4 +215,24 @@ mod tests { 0xf9, 0x46, 0xb3, 0x1d, ])) } + + #[test] + fn test_private_key_to_bytes() { + let secret = PrivateKey::generate(); + let bytes = secret.to_bytes(); + let from_bytes = PrivateKey::from_bytes(bytes); + assert_eq!(secret.public_key(), from_bytes.public_key()); + } + + #[test] + #[should_panic] + fn test_private_key_to_bytes_malformed_a() { + PrivateKey::from_bytes(vec![]); + } + + #[test] + #[should_panic] + fn test_private_key_to_bytes_malformed_b() { + PrivateKey::from_bytes(vec![1, 2, 3]); + } } diff --git a/runtime/src/common/sgx/egetkey.rs b/runtime/src/common/sgx/egetkey.rs index f226b01e9f6..580a6ab214d 100644 --- a/runtime/src/common/sgx/egetkey.rs +++ b/runtime/src/common/sgx/egetkey.rs @@ -61,7 +61,7 @@ fn egetkey_impl(key_policy: Keypolicy, context: &[u8]) -> [u8; 16] { /// egetkey returns a 256 bit key suitable for sealing secrets to the /// enclave in cold storage, derived from the results of the `EGETKEY` -/// instruction. The `context` field a domain separation tag. +/// instruction. The `context` field is a domain separation tag. /// /// Note: The key can also be used for other things (eg: as an X25519 /// private key). diff --git a/runtime/src/common/sgx/mod.rs b/runtime/src/common/sgx/mod.rs index fa50d45b684..a73cbb5b9b7 100644 --- a/runtime/src/common/sgx/mod.rs +++ b/runtime/src/common/sgx/mod.rs @@ -2,3 +2,4 @@ pub mod avr; pub mod egetkey; +pub mod seal; diff --git a/runtime/src/common/sgx/seal.rs b/runtime/src/common/sgx/seal.rs new file mode 100644 index 00000000000..e004c63ce48 --- /dev/null +++ b/runtime/src/common/sgx/seal.rs @@ -0,0 +1,107 @@ +//! Wrappers for sealing secrets to the enclave in cold storage. +use rand::{rngs::OsRng, Rng}; +use sgx_isa::Keypolicy; +use zeroize::Zeroize; + +use crate::common::{ + crypto::mrae::deoxysii::{DeoxysII, NONCE_SIZE, TAG_SIZE}, + sgx::egetkey::egetkey, +}; + +/// Seal a secret to the enclave. +/// +/// The `context` field is a domain separation tag. +pub fn seal(key_policy: Keypolicy, context: &[u8], data: &[u8]) -> Vec { + let mut rng = OsRng::new().unwrap(); + + // Encrypt the raw policy. + let mut nonce = [0u8; NONCE_SIZE]; + rng.fill(&mut nonce); + let d2 = new_d2(key_policy, context); + let mut ciphertext = d2.seal(&nonce, data.to_vec(), vec![]); + ciphertext.extend_from_slice(&nonce); + + ciphertext +} + +/// Unseal a previously sealed secret to the enclave. +/// +/// The `context` field is a domain separation tag. +/// +/// # Panics +/// +/// All parsing and authentication errors of the ciphertext are fatal and +/// will result in a panic. +pub fn unseal(key_policy: Keypolicy, context: &[u8], ciphertext: &[u8]) -> Option> { + let ct_len = ciphertext.len(); + if ct_len == 0 { + return None; + } else if ct_len < TAG_SIZE + NONCE_SIZE { + panic!("ciphertext is corrupted, invalid size"); + } + let ct_len = ct_len - NONCE_SIZE; + + // Split the ciphertext || tag || nonce. + let mut nonce = [0u8; NONCE_SIZE]; + nonce.copy_from_slice(&ciphertext[ct_len..]); + let ciphertext = &ciphertext[..ct_len]; + + let d2 = new_d2(key_policy, context); + let plaintext = d2 + .open(&nonce, ciphertext.to_vec(), vec![]) + .expect("ciphertext is corrupted"); + + Some(plaintext) +} + +fn new_d2(key_policy: Keypolicy, context: &[u8]) -> DeoxysII { + let mut seal_key = egetkey(key_policy, context); + let d2 = DeoxysII::new(&seal_key); + seal_key.zeroize(); + + d2 +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_seal_unseal() { + // Test different policies. + let sealed_a = seal(Keypolicy::MRSIGNER, b"MRSIGNER", b"Mr. Signer"); + let unsealed_a = unseal(Keypolicy::MRSIGNER, b"MRSIGNER", &sealed_a); + assert_eq!(unsealed_a, Some(b"Mr. Signer".to_vec())); + + let sealed_b = seal(Keypolicy::MRENCLAVE, b"MRENCLAVE", b"Mr. Enclave"); + let unsealed_b = unseal(Keypolicy::MRENCLAVE, b"MRENCLAVE", &sealed_b); + assert_eq!(unsealed_b, Some(b"Mr. Enclave".to_vec())); + + // Test zero-length ciphertext. + let unsealed_c = unseal(Keypolicy::MRENCLAVE, b"MRENCLAVE", b""); + assert_eq!(unsealed_c, None); + } + + #[test] + #[should_panic] + fn test_incorrect_context() { + // Test incorrect context. + let sealed_b = seal(Keypolicy::MRENCLAVE, b"MRENCLAVE1", b"Mr. Enclave"); + unseal(Keypolicy::MRENCLAVE, b"MRENCLAVE2", &sealed_b); + } + + #[test] + #[should_panic] + fn test_incorrect_ciphertext_a() { + let sealed_b = seal(Keypolicy::MRENCLAVE, b"MRENCLAVE", b"Mr. Enclave"); + unseal(Keypolicy::MRENCLAVE, b"MRENCLAVE", &sealed_b[..2]); + } + + #[test] + #[should_panic] + fn test_incorrect_ciphertext_b() { + let mut sealed_b = seal(Keypolicy::MRENCLAVE, b"MRENCLAVE", b"Mr. Enclave"); + sealed_b[0] = 0x00; + unseal(Keypolicy::MRENCLAVE, b"MRENCLAVE", &sealed_b); + } +}