Merge pull request #4067 from stevew817/feature/allow_multilength_aead
Add support for key policies (MAC & AEAD)
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/ecp.c b/library/ecp.c
index 3b68e8e..6a005d5 100644
--- a/library/ecp.c
+++ b/library/ecp.c
@@ -1243,6 +1243,13 @@
while( (N).s < 0 && mbedtls_mpi_cmp_int( &(N), 0 ) != 0 ) \
MBEDTLS_MPI_CHK( mbedtls_mpi_add_mpi( &(N), &(N), &grp->P ) )
+#if ( defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) && \
+ !( defined(MBEDTLS_ECP_NO_FALLBACK) && \
+ defined(MBEDTLS_ECP_DOUBLE_JAC_ALT) && \
+ defined(MBEDTLS_ECP_ADD_MIXED_ALT) ) ) || \
+ ( defined(MBEDTLS_ECP_MONTGOMERY_ENABLED) && \
+ !( defined(MBEDTLS_ECP_NO_FALLBACK) && \
+ defined(MBEDTLS_ECP_DOUBLE_ADD_MXZ_ALT) ) )
static inline int mbedtls_mpi_sub_mod( const mbedtls_ecp_group *grp,
mbedtls_mpi *X,
const mbedtls_mpi *A,
@@ -1254,6 +1261,7 @@
cleanup:
return( ret );
}
+#endif /* All functions referencing mbedtls_mpi_sub_mod() are alt-implemented without fallback */
/*
* Reduce a mbedtls_mpi mod p in-place, to use after mbedtls_mpi_add_mpi and mbedtls_mpi_mul_int.
@@ -1276,6 +1284,10 @@
return( ret );
}
+#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED) && \
+ !( defined(MBEDTLS_ECP_NO_FALLBACK) && \
+ defined(MBEDTLS_ECP_DOUBLE_JAC_ALT) && \
+ defined(MBEDTLS_ECP_ADD_MIXED_ALT) )
static inline int mbedtls_mpi_shift_l_mod( const mbedtls_ecp_group *grp,
mbedtls_mpi *X,
size_t count )
@@ -1286,6 +1298,7 @@
cleanup:
return( ret );
}
+#endif /* All functions referencing mbedtls_mpi_shift_l_mod() are alt-implemented without fallback */
#if defined(MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED)
/*
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 */
diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function
index a6b0b84..55b9416 100644
--- a/tests/suites/test_suite_psa_crypto.function
+++ b/tests/suites/test_suite_psa_crypto.function
@@ -2864,6 +2864,7 @@
unsigned char *output_data2 = NULL;
size_t output_length2 = 0;
size_t tag_length = PSA_AEAD_TAG_LENGTH( alg );
+ psa_status_t status = PSA_ERROR_GENERIC_ERROR;
psa_status_t expected_result = expected_result_arg;
psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
@@ -2884,14 +2885,24 @@
PSA_ASSERT( psa_import_key( &attributes, key_data->x, key_data->len,
&key ) );
- TEST_EQUAL( psa_aead_encrypt( key, alg,
- nonce->x, nonce->len,
- additional_data->x,
- additional_data->len,
- input_data->x, input_data->len,
- output_data, output_size,
- &output_length ),
- expected_result );
+ status = psa_aead_encrypt( key, alg,
+ nonce->x, nonce->len,
+ additional_data->x,
+ additional_data->len,
+ input_data->x, input_data->len,
+ output_data, output_size,
+ &output_length );
+
+ /* If the operation is not supported, just skip and not fail in case the
+ * encryption involves a common limitation of cryptography hardwares and
+ * an alternative implementation. */
+ if( status == PSA_ERROR_NOT_SUPPORTED )
+ {
+ MBEDTLS_TEST_PSA_SKIP_IF_ALT_AES_192( key_type, key_data->len * 8 );
+ MBEDTLS_TEST_PSA_SKIP_IF_ALT_GCM_NOT_12BYTES_NONCE( alg, nonce->len );
+ }
+
+ TEST_EQUAL( status, expected_result );
if( PSA_SUCCESS == expected_result )
{
