Check for TOCTOU bugs through compiler optimization

[HW42]

We sometimes have code which does roughly this:

int val = *uvar; // uvar is a pointer to untrusted memory
if (val > MAX_VAL) { // some check
    fail();
}
do_something(val); // now use the checked value

The idea here to first copy the value to trusted memory and then check and use the checked value. If this is done in this order this is TOCTOU safe. But we need to prevent the compiler from messing with this (for example by using the untrusted memory directly).

Changing the copy to a memcpy is (AFAIK) not enough since the compiler is allowed to optimize the memcpy away since it's part of the C standard (It's for example quite common to replace it with a mov for small fixed sized).

[dimakuv]

Do you have a solution in mind? I agree that memcpy() can be easily optimized away. I can think of two options:

• volatile int val -- will force the compiler to not optimize away this variable. Doesn't seem too efficient since every use of val will result in a memory access instead of a register access (but used sparely, will add tiny latency anyway since val is most likely in L1 cache/store buffer).

• volatile asm ("movq %uvar, %val", ...) -- will force the compiler to make a copy into a register. Even if the registers spills later, it will be on trusted enclave stack. Looks awkward though.

[dimakuv]

One more solution:

• Move these code snippets to a non-inlined function. This guarantees that the value of val is copied either in a register or on enclave stack. (This trick is used in PR gramineproject/graphene#622.)

[HW42]

My initial thought was to declare (in the above example) uvar as volatile int * (Unlike volatile int val this should not add any unnecessary operations assuming we read it only once (which we want anyway because of TOCTOU)).

But @mkow recently pointed out that for "local" variables volatile is not clearly specified, so depending on the exact code this might be a problem.

Non-inlined function looks like a nice option. One question is how to ensure that it's not inlined AFAIU the gcc documentation we need to add noinline attribute and an empty asm statement.

We probably want to move this into some generic file which provides reusable functions like:

uint64_t read_outside_uint64(uint64_t* uval);
bool copy_inside_enclave(void* ptr, uint64_t size, void* uptr);
// ...

And then use those elsewhere. We might consider to wrap untrusted pointers in a struct or something to prevent accidental direct access.

@mkow: What do you think?

[dimakuv]

I am for non-inlined functions outsourced to a separate file (e.g. enclave_framework.c).

I'm not sure I understood the point on "wrapping untrusted pointers in a struct". In the case of OCALLs, we already know that ms_ocall_name.any_field are always untrusted (so as soon as the developer sees ms, she should be super-careful). Are there other places where it's not obvious that values are untrusted?

[mkow]

But @mkow recently pointed out that for "local" variables volatile is not clearly specified, so depending on the exact code this might be a problem.

But volatile int * is not a local volatile variable ;) It's a local non-volatile pointer to a volatile int, so this is a different case.

I'd vote for going big and introducing solution analogue to __user in Linux and having dedicated (secure) functions to copy such data plus warnings if someone tries to access them without such "laundering". @HW42 Could you check how Linux implements this? Do we need any plugins for GCC or is this achieved using some C tricks?

[HW42]

I will have a look how Linux does it.

[HW42]

Ok, so in Linux we have two cases (I will ignore the optional STRUCTLEAK plugin here):

1. Normal compile: __user is just an empty macro. So beside being a visual hint to the developer it has no effect and you can dereference user space pointers (The CPU will catch you at runtime if you have SMAP).
2. make with C=1 or C=2: __user is defined as __attribute__((noderef, address_space(1))) when parsed by sparse. sparse is a C source code checker i.e. it can parse C and emit warnings for a bunch of (potential) errors. So for a wrongly used __user pointer sparse will show a warning (The correct functions for user space access use the special force attribute).

(__attribute__((noderef)) is also supported by new enough clang)

So unless we rewrite our make files to support checking by sparse this doesn't help.

I currently prefer to wrap pointers to untrusted memory in some struct. This should make abuse obvious and doesn't require any new compiler/checker support. Something like:

typedef struct {
    volatile void* __untrusted_mem_pointer;
} uptr_t;

uptr_t sgx_alloc_on_ustack(uint64_t size);
#define SGX_UNTRUSTED_STRUCT_SET(uptr, struct_name, field, value) //...
#define SGX_UNTRUSTED_STRUCT_GET(uptr, struct_name, field) //...
uint64_t sgx_copy_to_enclave(void* dst, uint64_t dst_size, uptr_t src, uint64_t src_size);
// ...

Any better ideas?

[yamahata]

Now the interface is contained within sgxcopy functions. Because the change would be contained. So the wrapping the pointer sounds fair enough.

BTW, regarding to 2, except Makefile, do you think it's worthwhile to add such syntax checks?

[mkow]

(attribute((noderef)) is also supported by new enough clang)

Just to be sure: You mean here, that clang alone is enough to have this feature? (without sparse) We plan to enable both clang-format and clang-tidy soon, so I guess we could also migrate compilation to clang (or at least support it and enable in Jenkins, to check correctness of __user before merging).

[HW42]

BTW, regarding to 2, except Makefile, do you think it's worthwhile to add such syntax checks?

In general I think having some checker/analyzer is a good idea. I don't know how helpful sparse is.

just to be sure: You mean here, that clang alone is enough to have this feature? (without sparse) We plan to enable both clang-format and clang-tidy soon, so I guess we could also migrate compilation to clang (or at least support it and enable in Jenkins, to check correctness of __user before merging).

Yes. Clang 8 seems to support noderef. Example

[mkow]

I'm voting for clang then, though it may require some non-trivial amount of work to get our code working with it.

[yamahata]

Then we're conversing that it's nice to have such syntax check. Its priority is not high, though. It's a matter of volunteer.

[boryspoplawski]

The TOCTOU part of this issue is fixed by gramineproject/graphene#1466

[mkow]

Ok, so, the only thing left here is implementing something like __user to ensure that the compiler checks this?

[boryspoplawski]

That would be for ensuring proper accesses to untrusted memory from inside enclave (not only for preventing TOCTOU issues),