diff --git a/ChangeLog.d/net_poll-fd_setsize.txt b/ChangeLog.d/net_poll-fd_setsize.txt new file mode 100644 index 000000000..e4db8c7e3 --- /dev/null +++ b/ChangeLog.d/net_poll-fd_setsize.txt @@ -0,0 +1,4 @@ +Security + * Fix a stack buffer overflow with mbedtls_net_poll() and + mbedtls_net_recv_timeout() when given a file descriptor that is + beyond FD_SETSIZE. Reported by FigBug in #4169. diff --git a/include/mbedtls/net_sockets.h b/include/mbedtls/net_sockets.h index 55fd18b52..319f4be53 100644 --- a/include/mbedtls/net_sockets.h +++ b/include/mbedtls/net_sockets.h @@ -124,6 +124,7 @@ int mbedtls_net_connect( mbedtls_net_context *ctx, const char *host, const char * * \return 0 if successful, or one of: * MBEDTLS_ERR_NET_SOCKET_FAILED, + * MBEDTLS_ERR_NET_UNKNOWN_HOST, * MBEDTLS_ERR_NET_BIND_FAILED, * MBEDTLS_ERR_NET_LISTEN_FAILED * @@ -143,6 +144,8 @@ int mbedtls_net_bind( mbedtls_net_context *ctx, const char *bind_ip, const char * can be NULL if client_ip is null * * \return 0 if successful, or + * MBEDTLS_ERR_NET_SOCKET_FAILED, + * MBEDTLS_ERR_NET_BIND_FAILED, * MBEDTLS_ERR_NET_ACCEPT_FAILED, or * MBEDTLS_ERR_NET_BUFFER_TOO_SMALL if buf_size is too small, * MBEDTLS_ERR_SSL_WANT_READ if bind_fd was set to @@ -155,6 +158,10 @@ int mbedtls_net_accept( mbedtls_net_context *bind_ctx, /** * \brief Check and wait for the context to be ready for read/write * + * \note The current implementation of this function uses + * select() and returns an error if the file descriptor + * is \c FD_SETSIZE or greater. + * * \param ctx Socket to check * \param rw Bitflag composed of MBEDTLS_NET_POLL_READ and * MBEDTLS_NET_POLL_WRITE specifying the events @@ -236,16 +243,21 @@ int mbedtls_net_send( void *ctx, const unsigned char *buf, size_t len ); * 'timeout' seconds. If no error occurs, the actual amount * read is returned. * + * \note The current implementation of this function uses + * select() and returns an error if the file descriptor + * is \c FD_SETSIZE or greater. + * * \param ctx Socket * \param buf The buffer to write to * \param len Maximum length of the buffer * \param timeout Maximum number of milliseconds to wait for data * 0 means no timeout (wait forever) * - * \return the number of bytes received, - * or a non-zero error code: - * MBEDTLS_ERR_SSL_TIMEOUT if the operation timed out, + * \return The number of bytes received if successful. + * MBEDTLS_ERR_SSL_TIMEOUT if the operation timed out. * MBEDTLS_ERR_SSL_WANT_READ if interrupted by a signal. + * Another negative error code (MBEDTLS_ERR_NET_xxx) + * for other failures. * * \note This function will block (until data becomes available or * timeout is reached) even if the socket is set to diff --git a/library/net_sockets.c b/library/net_sockets.c index 54c2b472f..ad1ac13fb 100644 --- a/library/net_sockets.c +++ b/library/net_sockets.c @@ -465,6 +465,13 @@ int mbedtls_net_poll( mbedtls_net_context *ctx, uint32_t rw, uint32_t timeout ) if( fd < 0 ) return( MBEDTLS_ERR_NET_INVALID_CONTEXT ); + /* A limitation of select() is that it only works with file descriptors + * that are strictly less than FD_SETSIZE. This is a limitation of the + * fd_set type. Error out early, because attempting to call FD_SET on a + * large file descriptor is a buffer overflow on typical platforms. */ + if( fd >= FD_SETSIZE ) + return( MBEDTLS_ERR_NET_POLL_FAILED ); + #if defined(__has_feature) #if __has_feature(memory_sanitizer) /* Ensure that memory sanitizers consider read_fds and write_fds as @@ -584,6 +591,13 @@ int mbedtls_net_recv_timeout( void *ctx, unsigned char *buf, if( fd < 0 ) return( MBEDTLS_ERR_NET_INVALID_CONTEXT ); + /* A limitation of select() is that it only works with file descriptors + * that are strictly less than FD_SETSIZE. This is a limitation of the + * fd_set type. Error out early, because attempting to call FD_SET on a + * large file descriptor is a buffer overflow on typical platforms. */ + if( fd >= FD_SETSIZE ) + return( MBEDTLS_ERR_NET_POLL_FAILED ); + FD_ZERO( &read_fds ); FD_SET( fd, &read_fds ); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index e2bc42018..fb604271d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -134,6 +134,7 @@ add_test_suite(md) add_test_suite(mdx) add_test_suite(memory_buffer_alloc) add_test_suite(mpi) +add_test_suite(net) add_test_suite(nist_kw) add_test_suite(oid) add_test_suite(pem) diff --git a/tests/suites/test_suite_net.data b/tests/suites/test_suite_net.data new file mode 100644 index 000000000..4f516c8b6 --- /dev/null +++ b/tests/suites/test_suite_net.data @@ -0,0 +1,8 @@ +Context init-free-free +context_init_free:0 + +Context init-free-init-free +context_init_free:1 + +net_poll beyond FD_SETSIZE +poll_beyond_fd_setsize: diff --git a/tests/suites/test_suite_net.function b/tests/suites/test_suite_net.function new file mode 100644 index 000000000..f429fc922 --- /dev/null +++ b/tests/suites/test_suite_net.function @@ -0,0 +1,137 @@ +/* BEGIN_HEADER */ + +#include "mbedtls/net_sockets.h" + +#if defined(unix) || defined(__unix__) || defined(__unix) || \ + defined(__APPLE__) || defined(__QNXNTO__) || \ + defined(__HAIKU__) || defined(__midipix__) +#define MBEDTLS_PLATFORM_IS_UNIXLIKE +#endif + +#if defined(MBEDTLS_PLATFORM_IS_UNIXLIKE) +#include +#include +#include +#include +#include +#include +#endif + + +#if defined(MBEDTLS_PLATFORM_IS_UNIXLIKE) +/** Open a file on the given file descriptor. + * + * This is disruptive if there is already something open on that descriptor. + * Caller beware. + * + * \param ctx An initialized, but unopened socket context. + * On success, it refers to the opened file (\p wanted_fd). + * \param wanted_fd The desired file descriptor. + * + * \return \c 0 on succes, a negative error code on error. + */ +static int open_file_on_fd( mbedtls_net_context *ctx, int wanted_fd ) +{ + int got_fd = open( "/dev/null", O_RDONLY ); + TEST_ASSERT( got_fd >= 0 ); + if( got_fd != wanted_fd ) + { + TEST_ASSERT( dup2( got_fd, wanted_fd ) >= 0 ); + TEST_ASSERT( close( got_fd ) >= 0 ); + } + ctx->fd = wanted_fd; + return( 0 ); +exit: + return( -1 ); +} +#endif /* MBEDTLS_PLATFORM_IS_UNIXLIKE */ + +/* END_HEADER */ + +/* BEGIN_DEPENDENCIES + * depends_on:MBEDTLS_NET_C + * END_DEPENDENCIES + */ + +/* BEGIN_CASE */ +void context_init_free( int reinit ) +{ + mbedtls_net_context ctx; + + mbedtls_net_init( &ctx ); + mbedtls_net_free( &ctx ); + + if( reinit ) + mbedtls_net_init( &ctx ); + mbedtls_net_free( &ctx ); + + /* This test case always succeeds, functionally speaking. A plausible + * bug might trigger an invalid pointer dereference or a memory leak. */ + goto exit; +} +/* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_PLATFORM_IS_UNIXLIKE */ +void poll_beyond_fd_setsize( ) +{ + /* Test that mbedtls_net_poll does not misbehave when given a file + * descriptor greater or equal to FD_SETSIZE. This code is specific to + * platforms with a Unix-like select() function, which is where + * FD_SETSIZE is a concern. */ + + struct rlimit rlim_nofile; + int restore_rlim_nofile = 0; + int ret; + mbedtls_net_context ctx; + uint8_t buf[1]; + + mbedtls_net_init( &ctx ); + + /* On many systems, by default, the maximum permitted file descriptor + * number is less than FD_SETSIZE. If so, raise the limit if + * possible. + * + * If the limit can't be raised, a file descriptor opened by the + * net_sockets module will be less than FD_SETSIZE, so the test + * is not necessary and we mark it as skipped. + * A file descriptor could still be higher than FD_SETSIZE if it was + * opened before the limit was lowered (which is something an application + * might do); but we don't do such things in our test code, so the unit + * test will run if it can. + */ + TEST_ASSERT( getrlimit( RLIMIT_NOFILE, &rlim_nofile ) == 0 ); + if( rlim_nofile.rlim_cur < FD_SETSIZE + 1 ) + { + rlim_t old_rlim_cur = rlim_nofile.rlim_cur; + rlim_nofile.rlim_cur = FD_SETSIZE + 1; + TEST_ASSUME( setrlimit( RLIMIT_NOFILE, &rlim_nofile ) == 0 ); + rlim_nofile.rlim_cur = old_rlim_cur; + restore_rlim_nofile = 1; + } + + TEST_ASSERT( open_file_on_fd( &ctx, FD_SETSIZE ) == 0 ); + + /* In principle, mbedtls_net_poll() with valid arguments should succeed. + * However, we know that on Unix-like platforms (and others), this function + * is implemented on top of select() and fd_set, which do not support + * file descriptors greater or equal to FD_SETSIZE. So we expect to hit + * this platform limitation. + * + * If mbedtls_net_poll() does not proprely check that ctx.fd is in range, + * it may still happen to return the expected failure code, but if this + * is problematic on the particular platform where the code is running, + * a memory sanitizer such as UBSan should catch it. + */ + ret = mbedtls_net_poll( &ctx, MBEDTLS_NET_POLL_READ, 0 ); + TEST_EQUAL( ret, MBEDTLS_ERR_NET_POLL_FAILED ); + + /* mbedtls_net_recv_timeout() uses select() and fd_set in the same way. */ + ret = mbedtls_net_recv_timeout( &ctx, buf, sizeof( buf ), 0 ); + TEST_EQUAL( ret, MBEDTLS_ERR_NET_POLL_FAILED ); + +exit: + mbedtls_net_free( &ctx ); + if( restore_rlim_nofile ) + setrlimit( RLIMIT_NOFILE, &rlim_nofile ); +} +/* END_CASE */