Merge pull request #240 from geonnave/ffi-safety-lakers-c

Towards safer code in the FFI

examples/lakers-c-native/Makefile

@@ -1,5 +1,5 @@
 CC=gcc
-CFLAGS=-Wall -I../../target/include
+CFLAGS=-Wall -I../../target/include -fsanitize=address,undefined,leak
 
 TARGET=lakers_c_native
 
@@ -16,6 +16,8 @@ ifeq ($(LAKERS_EAD), authz)
 	CFLAGS += -DLAKERS_EAD_AUTHZ
 endif
 
+LDFLAGS=-fsanitize=address,undefined,leak
+
 all: $(TARGET)
 
 # rule for building the target executable

examples/lakers-c-native/README.md

@@ -17,3 +17,5 @@ make LAKERS_EAD=authz && ./lakers_c_native
 - See the README in the `lakers-c` crate.
 - Install [libcoap](https://libcoap.net/install.html):
   - tested with the following configuration: `./configure --disable-doxygen --disable-manpages --disable-dtls --disable-oscore`
+
+Note: the following sanitizers are enabled in the `Makefile`: `address,undefined,leak` (see for example the [AddressSanitizer](https://clang.llvm.org/docs/AddressSanitizer.html)). They may help catch bugs but make the executable larger and slower.

examples/lakers-c-native/main.c

@@ -43,10 +43,13 @@ static coap_response_t message_handler(coap_session_t *session COAP_UNUSED,
     has_coap_response = 1;
     // coap_show_pdu(COAP_LOG_WARN, received);
     const uint8_t *data;
-    coap_get_data(received, &coap_response_payload_len, &data);
-    memcpy(coap_response_payload, data, coap_response_payload_len);
-    puts("received coap response");
-    print_hex((uint8_t *)coap_response_payload, coap_response_payload_len);
+    if (coap_get_data(received, &coap_response_payload_len, &data)) {
+        memcpy(coap_response_payload, data, coap_response_payload_len);
+        puts("received coap response");
+        print_hex((uint8_t *)coap_response_payload, coap_response_payload_len);
+    } else {
+        puts("received coap response without payload");
+    }
     return COAP_RESPONSE_OK;
 }
 
@@ -152,9 +155,9 @@ int main(void)
     uint8_t c_r;
     CredentialRPK fetched_cred_r = {0};
 #ifdef LAKERS_EAD_AUTHZ
-    res = initiator_parse_message_2(&initiator, &message_2, cred_r, &c_r, &fetched_cred_r, &ead_2);
+    res = initiator_parse_message_2(&initiator, &message_2, &cred_r, &c_r, &fetched_cred_r, &ead_2);
 #else
-    res = initiator_parse_message_2(&initiator, &message_2, cred_r, &c_r, &fetched_cred_r, &ead_2);
+    res = initiator_parse_message_2(&initiator, &message_2, &cred_r, &c_r, &fetched_cred_r, &ead_2);
 #endif
     if (res != 0) {
         printf("Error parse msg2: %d\n", res);
@@ -170,16 +173,16 @@ int main(void)
         puts("ead-authz voucher received and validated");
     }
 #endif
-    res = initiator_verify_message_2(&initiator, &I, cred_i, fetched_cred_r);
+    res = initiator_verify_message_2(&initiator, &I, &cred_i, &fetched_cred_r);
     if (res != 0) {
         printf("Error verify msg2: %d\n", res);
         return 1;
     }
 
     puts("preparing msg3");
-    EdhocMessageBuffer message_3;
-    uint8_t prk_out[SHA256_DIGEST_LEN];
-    res = initiator_prepare_message_3(&initiator, ByReference, NULL, &message_3, prk_out);
+    EdhocMessageBuffer message_3 = {0};
+    uint8_t prk_out[SHA256_DIGEST_LEN] = {0};
+    res = initiator_prepare_message_3(&initiator, ByReference, NULL, &message_3, &prk_out);
     if (res != 0) {
         printf("Error prep msg3: %d\n", res);
         return 1;

lakers-c/src/initiator.rs

@@ -27,13 +27,15 @@ pub unsafe extern "C" fn initiator_new(initiator: *mut EdhocInitiator) -> i8 {
     suites_i[0..suites_i_len].copy_from_slice(&EDHOC_SUPPORTED_SUITES[..]);
     let (x, g_x) = default_crypto().p256_generate_key_pair();
 
-    (*initiator).start = InitiatorStart {
+    let start = InitiatorStart {
         x,
         g_x,
         suites_i,
         suites_i_len,
     };
 
+    core::ptr::write(&mut (*initiator).start, start);
+
     0
 }
 
@@ -68,8 +70,8 @@ pub unsafe extern "C" fn initiator_prepare_message_1(
 
     let result = match i_prepare_message_1(&state, crypto, c_i, &ead_1) {
         Ok((state, msg_1)) => {
-            *message_1 = msg_1;
-            (*initiator_c).wait_m2 = state;
+            core::ptr::write(&mut *message_1, msg_1);
+            core::ptr::write(&mut (*initiator_c).wait_m2, state);
             0
         }
         Err(err) => err as i8,
@@ -83,7 +85,7 @@ pub unsafe extern "C" fn initiator_parse_message_2(
     // input params
     initiator_c: *mut EdhocInitiator,
     message_2: *const EdhocMessageBuffer,
-    expected_cred_r: CredentialRPK,
+    expected_cred_r: *const CredentialRPK,
     // output params
     c_r_out: *mut u8,
     valid_cred_r_out: *mut CredentialRPK,
@@ -101,7 +103,7 @@ pub unsafe extern "C" fn initiator_parse_message_2(
     let crypto = &mut default_crypto();
 
     // manually take `state` because Rust cannot move out of a dereferenced raw pointer directly
-    // raw pointers do not have ownership information, requiring manual handling of the data
+    // (raw pointers do not have ownership information, requiring manual handling of the data)
     let state = core::ptr::read(&(*initiator_c).wait_m2);
 
     let result = match i_parse_message_2(&state, crypto, &(*message_2)) {
@@ -110,7 +112,7 @@ pub unsafe extern "C" fn initiator_parse_message_2(
             *c_r_out = c_r;
 
             // NOTE: checking here to avoid having IdCredOwnedC being passed across the ffi boundary
-            let Ok(valid_cred_r) = credential_check_or_fetch(Some(expected_cred_r), id_cred_r)
+            let Ok(valid_cred_r) = credential_check_or_fetch(Some(*expected_cred_r), id_cred_r)
             else {
                 return -1;
             };
@@ -135,8 +137,8 @@ pub unsafe extern "C" fn initiator_verify_message_2(
     initiator_c: *mut EdhocInitiator,
     i: *const BytesP256ElemLen,
     // i_len: usize,
-    mut cred_i: CredentialRPK,
-    valid_cred_r: CredentialRPK,
+    mut cred_i: *mut CredentialRPK,
+    valid_cred_r: *mut CredentialRPK,
 ) -> i8 {
     if initiator_c.is_null() || i.is_null() {
         return -1;
@@ -145,10 +147,10 @@ pub unsafe extern "C" fn initiator_verify_message_2(
 
     let state = core::ptr::read(&(*initiator_c).processing_m2).to_rust();
 
-    match i_verify_message_2(&state, crypto, valid_cred_r, &(*i)) {
+    match i_verify_message_2(&state, crypto, *valid_cred_r, &(*i)) {
         Ok(state) => {
             (*initiator_c).processed_m2 = state;
-            (*initiator_c).cred_i = &mut cred_i as *mut CredentialRPK;
+            (*initiator_c).cred_i = cred_i;
             0
         }
         Err(err) => err as i8,
@@ -170,7 +172,7 @@ pub unsafe extern "C" fn initiator_prepare_message_3(
     }
     let crypto = &mut default_crypto();
 
-    let mut state = core::ptr::read(&(*initiator_c).processed_m2);
+    let state = core::ptr::read(&(*initiator_c).processed_m2);
 
     let ead_3 = if ead_3_c.is_null() {
         None
@@ -180,7 +182,7 @@ pub unsafe extern "C" fn initiator_prepare_message_3(
     };
 
     match i_prepare_message_3(
-        &mut state,
+        &state,
         crypto,
         *(*initiator_c).cred_i,
         cred_transfer,

lakers-c/src/lib.rs

@@ -9,6 +9,7 @@
 use lakers::*;
 use lakers_crypto::{default_crypto, CryptoTrait};
 
+#[cfg(feature = "ead-authz")]
 pub mod ead_authz;
 pub mod initiator;
 

lib/src/edhoc.rs

@@ -134,7 +134,7 @@ pub fn r_prepare_message_2(
     let mut ct: BufferCiphertext2 = BufferCiphertext2::new();
     ct.fill_with_slice(plaintext_2.as_slice()).unwrap(); // TODO(hax): can we prove with hax that this won't panic since they use the same underlying buffer length?
 
-    let ciphertext_2 = encrypt_decrypt_ciphertext_2(crypto, &prk_2e, &th_2, ct);
+    let ciphertext_2 = encrypt_decrypt_ciphertext_2(crypto, &prk_2e, &th_2, &ct);
 
     ct.fill_with_slice(ciphertext_2.as_slice()).unwrap(); // TODO(hax): same as just above.
 
@@ -310,7 +310,7 @@ pub fn i_parse_message_2<'a>(
         // compute prk_2e
         let prk_2e = compute_prk_2e(crypto, &state.x, &g_y, &th_2);
 
-        let plaintext_2 = encrypt_decrypt_ciphertext_2(crypto, &prk_2e, &th_2, ciphertext_2);
+        let plaintext_2 = encrypt_decrypt_ciphertext_2(crypto, &prk_2e, &th_2, &ciphertext_2);
 
         // decode plaintext_2
         let plaintext_2_decoded = decode_plaintext_2(&plaintext_2);
@@ -399,7 +399,7 @@ pub fn i_verify_message_2(
 }
 
 pub fn i_prepare_message_3(
-    state: &mut ProcessedM2,
+    state: &ProcessedM2,
     crypto: &mut impl CryptoTrait,
     cred_i: CredentialRPK,
     cred_transfer: CredentialTransfer,
@@ -888,7 +888,7 @@ fn encrypt_decrypt_ciphertext_2(
     crypto: &mut impl CryptoTrait,
     prk_2e: &BytesHashLen,
     th_2: &BytesHashLen,
-    mut ciphertext_2: BufferCiphertext2,
+    ciphertext_2: &BufferCiphertext2,
 ) -> BufferCiphertext2 {
     // convert the transcript hash th_2 to BytesMaxContextBuffer type
     let mut th_2_context: BytesMaxContextBuffer = [0x00; MAX_KDF_CONTEXT_LEN];
@@ -904,11 +904,13 @@ fn encrypt_decrypt_ciphertext_2(
         ciphertext_2.len,
     );
 
+    let mut result = BufferCiphertext2::default();
     for i in 0..ciphertext_2.len {
-        ciphertext_2.content[i] ^= keystream_2[i];
+        result.content[i] = ciphertext_2.content[i] ^ keystream_2[i];
     }
+    result.len = ciphertext_2.len;
 
-    ciphertext_2
+    result
 }
 
 fn compute_salt_4e3m(
@@ -1417,7 +1419,7 @@ mod tests {
             &mut default_crypto(),
             &PRK_2E_TV,
             &TH_2_TV,
-            ciphertext_2_tv,
+            &ciphertext_2_tv,
         );
 
         assert_eq!(plaintext_2.len, PLAINTEXT_2_LEN_TV);
@@ -1427,7 +1429,7 @@ mod tests {
 
         // test encryption
         let ciphertext_2 =
-            encrypt_decrypt_ciphertext_2(&mut default_crypto(), &PRK_2E_TV, &TH_2_TV, plaintext_2);
+            encrypt_decrypt_ciphertext_2(&mut default_crypto(), &PRK_2E_TV, &TH_2_TV, &plaintext_2);
 
         assert_eq!(ciphertext_2.len, CIPHERTEXT_2_LEN_TV);
         for i in 0..CIPHERTEXT_2_LEN_TV {