cbeck88 opened a new pull request #191: Make mutex unlock errors fatal rather than ignored URL: https://github.com/apache/incubator-teaclave-sgx-sdk/pull/191 In the paper "Remote Attestation is not Sufficient" by Yogesh Swami: https://www.blackhat.com/docs/us-17/thursday/us-17-Swami-SGX-Remote-Attestation-Is-Not-Sufficient-wp.pdf it is described that one way an adversary can attack an enclave is by maliciously interrupting it during its execution, re-entering ECALLs, not returning from OCALLs, etc. If they can cause undefined behavior in the enclave this way, then they may be able to mount an attack. We have been considering a patch like this in our internal fork of rust-sgx-sdk, the motivation is the following scenario ``` { let _ = mutex.lock(); my_ocall(); } ``` We would like to be able to say that if untrusted tries to mess with control flow and return from `my_ocall` unexpectedly, or on the "wrong" thread, then a panic will ensue, rather than the failure to properly unlock the mutex being silently ignored. I thought I would send you a patch and ask you what you think about this, please let me know! --- For comparison: In POSIX, unlocking a mutex that you don't own is "illegal", and undefined behavior in general, but not necessarily a hard error: https://stackoverflow.com/questions/1778780/if-you-unlock-an-already-unlocked-mutex-is-the-behavior-undefined In the Intel tlibcxx C++ code, they have the following: https://github.com/intel/linux-sgx/blob/master/sdk/tlibcxx/src/mutex.cpp#L51 ``` void mutex::unlock() _NOEXCEPT { int ec = __libcpp_mutex_unlock(&__m_); (void)ec; assert(ec == 0); } ``` That is, when compiling in debug mode, it's an assertion failure, but in release mode the error code is ignored. The Intel SGX SDK documentation defines `sgx_thread_mutex_unlock` and specifies ``` Return value 0 The mutex is unlocked successfully. EINVAL The trusted mutex object is invalid or it is not locked by any thread. EPERM The mutex is locked by another thread. ``` and you wrap this as `rsgx_thread_mutex_unlock` with a rust `Result` object. However, before this commit, we ultimately drop / ignore the result in the `SgxMutexGuard` destructor. This commit makes us panic instead. In rust `libstd`, we don't have this panic in any configuration, the error is always ignored https://github.com/rust-lang/rust/blob/master/src/libstd/sync/mutex.rs#L445 so in particular, fortanix sgx folks have either not thought of this or chose not to do it. My instinct is that this probably doesn't do any measurable harm and helps to "fail fast" in a security critical environment which is usually good. OTOH the motivating example is somewhat handwaving. Maybe there is a better one, or some other reason not to worry about this, I'm not really sure.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
