Merge pull request #4173 from gilles-peskine-arm/net_poll-fd_setsize-development

Fix stack corruption in mbedtls_net_poll with large file descriptor

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.

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,
+ * \return         The number of bytes received if successful.
- *                 or a non-zero error code:
+ *                 MBEDTLS_ERR_SSL_TIMEOUT if the operation timed out.
- *                 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

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 );
 

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)

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:

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 */