Implement SGX encrypted channel demo #30

lkatalin · 2019-10-30T20:47:55Z

I'm submitting this as PR not because I think it's done, but because I hope to solicit extra comments and reviews over the next week to add to my to-do list for when I get back.

I have many TODO comments and questions in the code. Anything that is a question I would especially like feedback or ideas on.

Some things that I plan to do before the PR is merged (and would love ideas on any of these):

Edit: DONE. Switch the cipher from CTR to GCM. ~~Currently this gives a key length error and I have not had a chance to debug it. Possibly it has something to do with the length of tag or aad, but unsure.~~
Edit: DONE. Add larger integers instead of single digits. ~~This requires changing the way the streams interact and making the data u32 instead of u8, which adds complication. It's probably not that hard, but seems fiddly.~~
Edit: Rejected. Have the attestation request initiated by the tenant sending its pub key. This just seems more logical. However, it requires changing more things about stream handling. For example, having the daemon pass the tenant's pubkey to the enclave before attestation means the pubkey goes out of scope after the daemon's stream is handled. Thus it is not usable when handling the tenant<-->enclave stream.

lkatalin · 2019-11-11T20:29:27Z

I fixed the cipher to be AES256GCM and also added the capability to add u32s.

haraldh · 2019-11-12T08:58:47Z

intel-sgx/attestation-enclave/src/main.rs

+    // field of the enclave's attestation Report, which will be transmitted to the tenant.
+    let mut entropy = Rdseed;
+    let mut rng = CtrDrbg::new(&mut entropy, None)?;
+    let mut ec_key = Pk::generate_ec(&mut rng, curve.clone())?;


Does this need a check, if the point is on the curve and not identity? A quick search in the mbed-tls source didn't reveal such a check.

@npmccallum ?

In the update I'm about to push, I'm now using https://github.com/fortanix/rust-mbedtls/blob/master/mbedtls/src/pk/mod.rs#L312, though I'm not completely sure I'm using it correctly.

haraldh · 2019-11-12T09:07:32Z

intel-sgx/attestation-enclave/src/main.rs


    // The enclave handles each incoming connection from attestation daemon.
-    for stream in TcpListener::bind(LISTENER_ADDR).unwrap().incoming() {
+    for stream in daemon_streams.incoming() {


since we are only handling one connection, we could get rid of the for loop and instead use:

// The enclave handles _one_ incoming connection from the attestation daemon. let mut stream = daemon_streams .incoming() .next() .unwrap_or(Err(std::io::ErrorKind::ConnectionReset.into()))?;

For some reason, when I make this change, the program hangs. So I have left it alone for now.

haraldh · 2019-11-12T09:23:08Z

intel-sgx/attestation-enclave/src/main.rs

+        let tenant_key = iterator.next().unwrap()?;
+        let ad = iterator.next().unwrap()?;
+        let iv1 = iterator.next().unwrap()?;
+        let iv2 = iterator.next().unwrap()?;
+        let tag1 = iterator.next().unwrap()?;
+        let tag2 = iterator.next().unwrap()?;
+        let ciphertext1 = iterator.next().unwrap()?;
+        let ciphertext2 = iterator.next().unwrap()?;


probably needs some Vec::len() check

This is related to another change requested by @mbestavros to not limit the length of data being sent, since in the future we will send larger chunks of data (ex. binary files). Should I add checks for the current type of data being passed, or get rid of such artificial size limits? Does this matter for the demo? @haraldh @npmccallum

haraldh · 2019-11-12T09:25:04Z

intel-sgx/attestation-enclave/src/main.rs

+        raw::CipherMode::GCM,
+        (symm_key.len() * 8) as _,
+    )
+    .unwrap();


unwrap() or ? ?

haraldh · 2019-11-12T09:28:07Z

intel-sgx/attestation-enclave/src/main.rs

+        raw::CipherMode::GCM,
+        (symm_key.len() * 8) as _,
+    )
+    .unwrap();


unwrap() or ? ?

haraldh · 2019-11-12T09:36:35Z

intel-sgx/attestation-enclave/src/main.rs

+
+        // TODO: This line exits the program after one run. Otherwise, it appears as though the tenant can be run
+        // again, but instead the program just hangs the second time. Why?
+        break;


Well, you would have to open a new connection from the tenant with TcpStream::connect(ENCL_CONN) to get a new loop iteration without the break.

haraldh · 2019-11-12T09:40:15Z

intel-sgx/attestation-enclave/src/main.rs

+        let mut iterator = deserializer.into_iter::<Vec<u8>>();
+
+        let tenant_key = iterator.next().unwrap()?;
+        let ad = iterator.next().unwrap()?;


As far as I understand ad is authenticated data. So it's unencrypted but signed. Maybe we can use it to send a hash of the plaintext data and check the integrity with it after decryption?
@npmccallum ?

I changed this field to hold a sha256 hash of the plaintext both in the tenant and the enclave.

haraldh · 2019-11-12T09:52:49Z

intel-sgx/attestation-tenant/src/key.rs

+    // TODO: Is this a good curve to use for ECDH keys?
+    pub fn new_pair_secp256r1() -> Result<Self, Box<dyn Error>> {
+        let curve = EcGroup::from_curve_name(Nid::X9_62_PRIME256V1)?;
+        let eckey_priv = EcKey::generate(&curve)?;


again.. needs eckey_priv.check_key() I guess

https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=crypto/ec/ec_key.c;h=7fa247593d91b45347704e62e184e1138fc8bd01;hb=46ebd9e3bb623d3c15ef2203038956f3f7213620#l304

mbestavros

A very targeted change, but a needed one:

As we begin to think about sending more varied (read: larger) data over the encrypted channel, we should not artificially limit the size of the data being sent and arriving to a few bytes.

For now, removing the size declaration for the input/output arrays completely will be easiest, though we may want to consider re-adding a more thought-about higher limit further down the road to address potential security and denial-of-service concerns.

mbestavros · 2019-11-18T20:39:53Z

intel-sgx/attestation-enclave/src/main.rs

+        let mut plaintext1 = [0u8; 32];
+        let mut plaintext2 = [0u8; 32];


We should not artificially limit the plaintext length here.

mbestavros · 2019-11-18T20:40:38Z

intel-sgx/attestation-tenant/src/main.rs

+    let mut val1_as_bytes = [0u8; 4];
+    NativeEndian::write_u32(&mut val1_as_bytes, val1);
+    let mut val2_as_bytes = [0u8; 4];
+    NativeEndian::write_u32(&mut val2_as_bytes, val2);


We should not artificially limit the plaintext length here.

mbestavros · 2019-11-18T21:12:39Z

intel-sgx/attestation-enclave/src/main.rs

+        NativeEndian::write_u32(&mut sum_as_bytes, sum);
+
+        // The sum is encrypted.
+        let mut ciphersum = [0u8; 5];


One more quick one I forgot, sorry!

lkatalin force-pushed the encrypted-connection branch from 3900cd0 to a38b88d Compare November 11, 2019 20:27

haraldh suggested changes Nov 12, 2019

View reviewed changes

haraldh requested a review from npmccallum November 12, 2019 09:56

mbestavros changed the title ~~Established encrypted channel between tenant and enclave using DHKE.~~ Implement SGX encrypted channel demo Nov 18, 2019

mbestavros suggested changes Nov 18, 2019

View reviewed changes

mbestavros mentioned this pull request Nov 18, 2019

What is the SGX demo progress? #34

Closed

mbestavros suggested changes Nov 18, 2019

View reviewed changes

Established encrypted channel between tenant and enclave using DHKE.

52eac95

lkatalin force-pushed the encrypted-connection branch from a38b88d to 52eac95 Compare December 2, 2019 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SGX encrypted channel demo #30

Implement SGX encrypted channel demo #30

lkatalin commented Oct 30, 2019 •

edited

Loading

lkatalin commented Nov 11, 2019

haraldh Nov 12, 2019

lkatalin Dec 2, 2019

haraldh Nov 12, 2019

lkatalin Dec 2, 2019 •

edited

Loading

haraldh Nov 12, 2019

lkatalin Dec 2, 2019

haraldh Nov 12, 2019

haraldh Nov 12, 2019

haraldh Nov 12, 2019

haraldh Nov 12, 2019

lkatalin Dec 2, 2019

haraldh Nov 12, 2019

mbestavros left a comment •

edited

Loading

mbestavros Nov 18, 2019

mbestavros Nov 18, 2019

mbestavros Nov 18, 2019

		let mut plaintext1 = [0u8; 32];
		let mut plaintext2 = [0u8; 32];

Implement SGX encrypted channel demo #30

Are you sure you want to change the base?

Implement SGX encrypted channel demo #30

Conversation

lkatalin commented Oct 30, 2019 • edited Loading

lkatalin commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkatalin Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbestavros left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkatalin commented Oct 30, 2019 •

edited

Loading

lkatalin Dec 2, 2019 •

edited

Loading

mbestavros left a comment •

edited

Loading