bl4ck5un / mbedtls-SGX

mbedtls-SGX: a SGX-friendly TLS stack (ported from mbedtls)
Apache License 2.0
91 stars 32 forks source link

Security issue: missing return value check when invoking `ocall_mbedtls_net_accept` #13

Closed Eadom closed 5 years ago

Eadom commented 5 years ago

I found a implementation issue in ocall_mbedtls_net_accept that will casue a vulnerability.

This is the definition in EDL.

int ocall_mbedtls_net_accept( [in] mbedtls_net_context *bind_ctx, [out] mbedtls_net_context *client_ctx, [out, size=buf_size] void *client_ip, size_t buf_size, [out] size_t *ip_len );

As we known, all the value OCALL returns is untrusted. mbedtls_net_accept_ocall is a wrapper function of OCALL function ocall_mbedtls_net_accept. mbedtls_net_accept_ocall should have checked whether the value returned from ocall_mbedtls_net_accept is valid, E.g., the ip_len should always less than or equal to buf_size.

Missing the check means the callee takes the responsibility to check returned value. Unfortunately, in example/enclave/s_server.c, the sample code doesn't check the value returned from mbedtls_net_accept_ocall, which will cause a stack memory leak.

int ssl_server()
{
    ...
    unsigned char client_ip[16] = { 0 };
    ...
    if( ( ret = mbedtls_net_accept_ocall( &listen_fd, &client_fd,
                    client_ip, sizeof( client_ip ), &cliip_len ) ) != 0 )
    ...
#if defined(MBEDTLS_SSL_DTLS_HELLO_VERIFY)
    if( opt.transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM )
    {
        if( ( ret = mbedtls_ssl_set_client_transport_id( &ssl,
                        client_ip, cliip_len ) ) != 0 )
    ...
}

The code above invokes mbedtls_net_accept_ocall to get client_ip and cliip_len at first, then it invokes mbedtls_ssl_set_client_transport_id to store client_ip with length of cliip_len. The cliip_len is returned from OCALL and there is no check on it. If the attacker returns cliip_len that is larger than sizeof(client_ip), mbedtls_ssl_set_client_transport_id will store larger size of contents than client_ip should be. That's a stack memory leak.

To fix this issue, we can implement a wrapper function in enclave. It invokes mbedtls_net_accept_ocall and check returned ip_len.

bl4ck5un commented 5 years ago

Thanks. Good catch. Would you like to fix it and submit a PR?