Sanity-check untrusted arguments in trusted sm_entry/exit stubs

[jovanbulck]

Currently, the trusted compiler-generated sm_entry/exit stubs do not properly check all arguments (return address/ return entry point). This allows an attacker to redirect control flow to arbitrary SM code, as described here (sect 5.2.2).

SMs can use the sancus_get_id and sancus_get_caller_id instructions to verify that a provided address indeed belongs to the caller SM.

[fritzalder]

We can add this to #33 or we can make a new PR.
Essentially, I would add a new variable on the SSA frame called __sm_ocall_id. On OCALLs (sm_exit.S in __sm_exit or in compiler pass (not sure where exactly)) we store the ID. On returns (i.e., in sm_entry.S), we check that the caller_id is equal to the stored one.

I'm not sure whether there is any benefit in also storing the address that we branched to. That address is checked before the call and it does not really matter whether the SM is still loaded at that address when we parse the response as long as we were called by that SM (i.e., if caller_id matches, we do not care whether sancus_get_id still returns the correct ID).

[jovanbulck]

yes this is relevant for Aion, as it affects temporal isolation of different threads: when thread B is scheduled it can try to jump into an SM belonging to thread A and crash it. Hence, all validation of untrusted arguments should happen in the atomic section before accepting/rejecting the call

From my msc thesis, there's actually 2 checks missing:

1. for ECALLs: return address (provided in r7): check sancus_get_id(r7) == sancus_get_caller_id() (ie otherwise we might ecall return inside our own enclave)
2. for OCALL returns: r6=0xffff iff waiting for ocall return && sancus_get_caller_id() == __sm_ocall_id (as you outlined above)

[jovanbulck]

since this is relatively small and closely related to #33 (even crucial for aion availability) I suggest to fix it right away before merging

this will make sure the issue is fixed cause it's open for too long already 😅

[fritzalder]

Your point 1. above includes that we indeed do need the __sm_original_caller_id as we discussed it before. We need to store the original caller and verify we return to that caller before exiting to prevent that it has been unloaded in the meantime. So we do need to store the original caller too (in contrast to just the 'most recent' caller as we do now).
I'll make the changes in the linker and add that variable in the SSA frame.

For 2. I'm not sure whether I read your comment correctly but I would not double check a proper use of 0xffff since the second check would fail anyway on improper use. I.e., do not extra store a marker whether we OCALL'ed before since the existence of ocall_id should suffice for that.

[jovanbulck]

for 1, not necessarily: we're mostly/only interested here to protect ourselves and not jump to within our own module (as is now possible). I say we don't care if an SM calls another SM and then unloads itself. Doing bounds checks is hard, so I suggest to simply use the ID check upon entry and then jump there (guaranteed to be outside) on ecall ret, even if the caller has meanwhile changed, that's not our problem

for 2 yes agreed, checking ocall_id is the import check and will prevent 0xfff from being abused as a magic return into empty stack (as is now also possible)

[fritzalder]

Both are addressed in 7630fa3 in #33 now.