search

keystone-enclave / riscv-pk

Security monitor for Keystone Enclave (mirror of riscv-pk). Will be deprecated when openSBI port is ready
Other
36 stars 14 forks source link

Missing checks for integer overflow in detect_region_overlap() #17

Closed david-oswald closed 5 years ago

david-oswald commented 5 years ago

There seems to be a (potential) issue related to an integer overflow in detect_region_overlap(): https://github.com/keystone-enclave/riscv-pk/blob/keystone/sm/pmp.c#L71

In there, the overlap flag is computed as follows:

region_overlap |= ((uintptr_t) epm_base < addr + size) && ((uintptr_t) epm_base + epm_size > addr);

There is no check for overflow of the additions here. Let's say that epm_base = 0x82800000 and epm_size = 100000 and that addr = 0x1 and size = 0xffffffffffffffff. Clearly the two regions actually do overlap. However, putting these values in the above condition will result in:

(0x82800000 < 0x0) && (0x82900000 > 0x1)

which evaluates to 0. detect_region_overlap() is used in enclave creation, but the above issue cannot be exploited as far as I see:

https://github.com/keystone-enclave/riscv-pk/blob/keystone/sm/pmp.c#L260

What stops the above issue from allowing overlapping regions are the various constraints imposed on the size (e.g. we cannot create UTM from 0x1 with size SIZE_MAX). It might still be possible to do something by e.g. moving the EPM to a high address or so; have not looked in detail.

As said I don't think the above leads to an attack, but might be in the future if the overlap check is used in more (different) places.

I think it makes a lot of sense to use a dedicated macro or function for preventing against such issues, see e.g. here what Open Enclave does: https://github.com/Microsoft/openenclave/blob/master/include/openenclave/bits/safemath.h

OP-TEE has the following: https://github.com/OP-TEE/optee_os/blob/master/core/kernel/tee_misc.c#L71

dkohlbre commented 5 years ago

Yep, we'll need to harden that (and take a look at nearby similar calculations). Thanks for noticing.

If you happen to be using any tools to find those, we'd love to integrate them into our CI flow. Currently haven't had a chance to start putting in static analysis tooling.

We might try and use the GCC/Clang builtin checks for these, I'll have to see if they work on RISC-V.

david-oswald commented 5 years ago

This one was found manually. But I agree automated checks would be good to have. This page seems to be helpful in that regard: https://www.pixelbeat.org/programming/gcc/integer_overflow.html

dkohlbre commented 5 years ago

This was landed in the cleanup. Further work will be in another bug.