Check for integer overflow in sancus_is_outside_sm macro

[jovanbulck]

thanks for the PR! Not sure about your first clause "( __OUTSIDE_SM(p, sm) && ((len <= 0) ||". If the length is zero, the condition should always be true and the outside_sm is not necessary I'd say. A negative length doesn't make any sense and the disadvantage of a macro is that we can't easily enforce an unsigned type as in an inlined function..

The logical OR is flawed I think. You never check the first condition __OUTSIDE_SM(p, sm) in the OR clause? So only the end pointer is checked, which renders the check vulnerable AFAIS?

I feel the logic is getting to complex for a macro and I think this will be better implemented as an always_inline function with typed arguments, making use of the OUTSIDE_SM helper macros?

Maybe a good place to start is the logic from the SGX-SDK:

https://github.com/intel/linux-sgx/blob/4589daddd58bec7367a6a9de3fe301e6de17671a/sdk/trts/trts.cpp#L101

// sgx_is_outside_enclave()
// Parameters:
//      addr - the start address of the buffer
//      size - the size of the buffer
// Return Value:
//      1 - the buffer is strictly outside the enclave
//      0 - the whole buffer or part of the buffer is not outside the enclave,
//          or the buffer is wrap around
//
int sgx_is_outside_enclave(const void *addr, size_t size)
{
    size_t start = reinterpret_cast<size_t>(addr);
    size_t end = 0;
    size_t enclave_start = (size_t)&__ImageBase;
    size_t enclave_end = enclave_start + g_global_data.enclave_size - 1;
    // g_global_data.enclave_end = enclave_base + enclave_size - 1;
    // so the enclave range is [enclave_start, enclave_end] inclusively

    if(size > 0)
    {
        end = start + size - 1;
    }
    else
    {
        end = start;
    }
    if( (start <= end) && ((end < enclave_start) || (start > enclave_end)) )
    {
        return 1;
    }
    return 0;
}

[Stienvdh]

Thanks for the comments! I think you might have mentally skipped a bracket, as the start address is always checked, and then there is (size is below or equal to zero) OR (buffer does not wrap around and end address is outside of enclave), which is similar to the SGX logic. Nevertheless, this might be the proof this should be in a function, I'll take care of it!

[jovanbulck]

thanks, yes you're absolutely right I mis-parsed the brackets my bad! :sweat_smile: It's indeed an indication that we'll be better off with an inlined function :)

[jovanbulck]

hm important update--re-reading the Tale of 2 worlds analysis, I remember the problem w this code goes beyond merely integer overflows:

Sancus. We found both logical errors and integer overflow vulnerabilities in the sancus_is_outside_sm() function provided by the trusted runtime. Particularly, the current implementation does not properly detect an untrusted buffer that spans the entire enclave address range, or a rogue length specifier that triggers an integer overflow to wrap around the 16-bit address space.

The code needs to be rewritten as in the Intel SGX code stub above. Currently, the code is fine if your buffer starts below the enclave and then (without overflowing) ends after the enclave, overwriting the entire enclave(!)

[jovanbulck]

closing, this is now handled and ready to be merged in #36