search

sancus-tee / sancus-core

Minimal OpenMSP430 hardware extensions for isolation and attestation
BSD 3-Clause "New" or "Revised" License
20 stars 13 forks source link

Crypto: `sancus_unwrap` always fails if `cipher_len` is zero #26

Open gianlu33 opened 2 years ago

gianlu33 commented 2 years ago

Ideally, what we would like to happen is a simple comparison between the provided MAC (tag) and the MAC calculated over the provided associated data (ad). This is not happening though, the return value of sancus_unwrap is always false.

Interestingly, sancus_wrap works fine if the plaintext length is zero. In fact, it returns true and the calculated MAC is correct.

Note that the result if the same no matter the value of the cipher pointer (NULL or not): if cipher_len is zero, sancus_unwrap always fails.

fritzalder commented 2 years ago

That's interesting..do we have a sancus_wrap example? I think I never actively used sancus_wrap before.

gianlu33 commented 2 years ago

I just added a test on my repo: gianlu33/sancus-examples@19c68f9

You can adjust the value of MSG_SIZE to see what happens when the size is zero.

You can also experience the bug mentioned in #1 if you specify an odd number for MSG_SIZE (e.g., 1)

Edit: oops, I messed up with the TAG length.. I updated the commit

jovanbulck commented 2 years ago

hm it seems like you want the inverse of sancus_tag, say sancus_untag. That indeed doesn't exist in sm_support.h and is not supported atm when manually calling sancus_wrap. This could probably be better documented.

In principle one could fix the hardware to support this, but not sure this is worth it. You essentially can call sancus_tag() in software and do a proper constant-time comparison in software.

jovanbulck commented 2 years ago

That's interesting..do we have a sancus_wrap example? I think I never actively used sancus_wrap before.

there's sensor-reader for that, with a full round-trip: wrap of a random number in the sancus enclave and an unwrap later in the makefile to simulate the remote stakeholder

jovanbulck commented 2 years ago

linking this to #27