ivila commented on issue #273: URL: https://github.com/apache/teaclave-trustzone-sdk/issues/273#issuecomment-5403669007
> this issue is not resolved and should be re-opened. Adding comments to the inherently-unsound function signatures neither resolves the inherent unsound-ness, nor does it provide a safe api. > > Teaclave's security posture should dictate that we _always_ assume malicious REE, so its not possible to hold the buffer constant while the TEE reads it. Exposing the function is therefore always unsound. The only read and write accessses the teaclave api should be exposing are c-style pointers, or safe wrappers using read_volatile and write_volatile. Exposing an `&[u8]` or `&mut [u8]` is inherently unsound, so the API should not exist (even one with an `unsafe` keyword). > > Also I'm pretty sure using TEE_Memmove in this fashion is _also_ unsound - its [identical to a regular memmove](https://github.com/OP-TEE/optee_os/blob/995fe5808b4c6389bd15753429fe23b9fa7b8e0a/lib/libutee/tee_api.c#L612) and IIRC memmoving from volatile memory is unsound. > > Side note: I apologize I haven't had time to address this with an updated PR. eventually (months) ill get around to it. I agree that there are two separate concerns here: Rust-level soundness of the API signature, and the security semantics of copying memory controlled by the REE. I don't think replacing TEE_MemMove with read_volatile / write_volatile addresses the latter. The REE buffer is untrusted by definition and may change at any point while it is being copied. read_volatile does not make that buffer stable, nor does it give us an atomic snapshot; the REE can still modify different bytes between individual volatile reads. So from the TEE threat-model perspective, a volatile byte-by-byte copy has essentially the same property as TEE_MemMove: the resulting copy may correspond to some adversarially chosen state of the REE buffer. The security boundary here is therefore the enclave-owned copy. The purpose of this operation is to materialize the untrusted input into a TEE-owned Vec; anything that requires integrity or consistency must validate that owned copy afterwards. We must not validate the REE buffer and then continue consuming the REE buffer, since that would indeed introduce a TOCTOU issue. That said, I think the Rust API-signature question is somewhat orthogonal. If constructing an &[u8] / &mut [u8] over REE-controlled memory itself violates Rust's reference invariants because the REE can concurrently mutate it, then that is a legitimate reason to consider changing the boundary API to raw pointers or another representation. Marking a function unsafe does not by itself make an otherwise impossible-to-satisfy reference invariant sound. What I don't think follows is that read_volatile / write_volatile is the required fix. Volatile accesses don't provide consistency, synchronization, or protection against malicious concurrent modification; they only affect how the compiler performs the accesses. If we change the API, I think it should be because we want to avoid creating Rust references to REE-controlled memory, not because volatile copying makes the REE memory trustworthy. So I would separate the two questions: * whether the public boundary should expose raw pointers rather than Rust slices, to avoid imposing Rust reference invariants on REE-controlled memory. * how the untrusted memory is copied into enclave-owned memory. For the second question, I don't see a security property provided by read_volatile that TEE_MemMove lacks for this use case. In either case, the copied value must be treated as untrusted and validated only after it resides in TEE-owned memory. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
