Merge pull request #4173 from gilles-peskine-arm/net_poll-fd_setsize-development
Fix stack corruption in mbedtls_net_poll with large file descriptor
diff --git a/ChangeLog.d/net_poll-fd_setsize.txt b/ChangeLog.d/net_poll-fd_setsize.txt
new file mode 100644
index 0000000..e4db8c7
--- /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 55fd18b..319f4be 100644
--- a/include/mbedtls/net_sockets.h
+++ b/include/mbedtls/net_sockets.h
@@ -124,6 +124,7 @@
*
* \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 @@
* 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 @@
/**
* \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 @@
* '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 54c2b47..ad1ac13 100644
--- a/library/net_sockets.c
+++ b/library/net_sockets.c
@@ -465,6 +465,13 @@
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 @@
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 e2bc420..fb60427 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -134,6 +134,7 @@
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 0000000..4f516c8
--- /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 0000000..f429fc9
--- /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 <sys/fcntl.h>
+#include <sys/resource.h>
+#include <sys/stat.h>
+#include <sys/time.h>
+#include <sys/types.h>
+#include <unistd.h>
+#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 */