From 1c0e48a2cebadb6601dfbe8d1d68909e4d54d730 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Feb 2021 18:37:46 +0100 Subject: [PATCH 1/8] New test suite: net A place to put tests for the net_sockets module (MBEDTLS_NET_C feature). Start with a context smoke test. Signed-off-by: Gilles Peskine --- tests/CMakeLists.txt | 1 + tests/suites/test_suite_net.data | 6 ++++++ tests/suites/test_suite_net.function | 28 ++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+) create mode 100644 tests/suites/test_suite_net.data create mode 100644 tests/suites/test_suite_net.function 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..98da8d99e --- /dev/null +++ b/tests/suites/test_suite_net.data @@ -0,0 +1,6 @@ +Context init-free-free +context_init_free:0 + +Context init-free-init-free +context_init_free:1 + diff --git a/tests/suites/test_suite_net.function b/tests/suites/test_suite_net.function new file mode 100644 index 000000000..03b4bd36f --- /dev/null +++ b/tests/suites/test_suite_net.function @@ -0,0 +1,28 @@ +/* BEGIN_HEADER */ + +#include "mbedtls/net_sockets.h" + +/* 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 */ From 6667a78c9bb8233d1938f35aff7daa7cfc1ed11f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Feb 2021 19:41:29 +0100 Subject: [PATCH 2/8] Add test for mbedtls_net_poll beyond FD_SETSIZE mbedtls_net_poll() and mbedtls_net_recv_timeout() rely on select(), which represents sets of file descriptors through the fd_set type. This type cannot hold file descriptors larger than FD_SETSIZE. Make sure that these functions identify this failure code. Without a proper range check of the file descriptor in the mbedtls_net_xxx function, this test fails when running with UBSan: ``` net_poll beyond FD_SETSIZE ........................................ source/library/net_sockets.c:482:9: runtime error: index 16 out of bounds for type '__fd_mask [16]' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior source/library/net_sockets.c:482:9 in ``` This is a non-regression test for https://github.com/ARMmbed/mbedtls/issues/4169 . The implementation of this test is specific to Unix-like platforms. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_net.data | 2 + tests/suites/test_suite_net.function | 104 +++++++++++++++++++++++++++ 2 files changed, 106 insertions(+) diff --git a/tests/suites/test_suite_net.data b/tests/suites/test_suite_net.data index 98da8d99e..4f516c8b6 100644 --- a/tests/suites/test_suite_net.data +++ b/tests/suites/test_suite_net.data @@ -4,3 +4,5 @@ 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 index 03b4bd36f..55fabd658 100644 --- a/tests/suites/test_suite_net.function +++ b/tests/suites/test_suite_net.function @@ -2,6 +2,50 @@ #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 @@ -26,3 +70,63 @@ void context_init_free( int reinit ) 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 beyond FD_SETSIZE. This code is specific to platforms + * with a Unix-like select() function. */ + + 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 or equal to FD_SETSIZE. If so, raise the limit if + * possible. + * + * If the limit can't be raised, a newly open file descriptor + * won't be higher than FD_SETSIZE, so the test is not necessary and we + * mark it as skipped. + */ + 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 beyond 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 */ From ddf437487901f61042fbd2b30b8c2a4b5a25134a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Feb 2021 19:49:44 +0100 Subject: [PATCH 3/8] Fix stack buffer overflow in net functions with large file descriptor Fix a stack buffer overflow with mbedtls_net_poll() and mbedtls_net_recv_timeout() when given a file descriptor that is beyond FD_SETSIZE. The bug was due to not checking that the file descriptor is within the range of an fd_set object. Fix #4169 Signed-off-by: Gilles Peskine --- ChangeLog.d/net_poll-fd_setsize.txt | 4 ++++ library/net_sockets.c | 14 ++++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 ChangeLog.d/net_poll-fd_setsize.txt 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/library/net_sockets.c b/library/net_sockets.c index 54c2b472f..375434abd 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 + * up to 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 + * up to 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 ); From e28f236b6b1aca7205752c3299b1142739581f4e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 24 Feb 2021 19:51:23 +0100 Subject: [PATCH 4/8] Document FD_SETSIZE limitation for mbedtls_net_{poll,recv_timeout} Signed-off-by: Gilles Peskine --- include/mbedtls/net_sockets.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/include/mbedtls/net_sockets.h b/include/mbedtls/net_sockets.h index 55fd18b52..310b397a4 100644 --- a/include/mbedtls/net_sockets.h +++ b/include/mbedtls/net_sockets.h @@ -155,6 +155,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 beyond \c FD_SETSIZE. + * * \param ctx Socket to check * \param rw Bitflag composed of MBEDTLS_NET_POLL_READ and * MBEDTLS_NET_POLL_WRITE specifying the events @@ -236,6 +240,10 @@ 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 beyond \c FD_SETSIZE. + * * \param ctx Socket * \param buf The buffer to write to * \param len Maximum length of the buffer From c8dab5b41e607a920689bbe846203c2083560ee6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 1 Mar 2021 11:39:21 +0100 Subject: [PATCH 5/8] Fix sloppy wording around stricly less-than vs less or equal Signed-off-by: Gilles Peskine --- include/mbedtls/net_sockets.h | 4 ++-- library/net_sockets.c | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/include/mbedtls/net_sockets.h b/include/mbedtls/net_sockets.h index 310b397a4..702e93f1c 100644 --- a/include/mbedtls/net_sockets.h +++ b/include/mbedtls/net_sockets.h @@ -157,7 +157,7 @@ int mbedtls_net_accept( mbedtls_net_context *bind_ctx, * * \note The current implementation of this function uses * select() and returns an error if the file descriptor - * is beyond \c FD_SETSIZE. + * is \c FD_SETSIZE or greater. * * \param ctx Socket to check * \param rw Bitflag composed of MBEDTLS_NET_POLL_READ and @@ -242,7 +242,7 @@ int mbedtls_net_send( void *ctx, const unsigned char *buf, size_t len ); * * \note The current implementation of this function uses * select() and returns an error if the file descriptor - * is beyond \c FD_SETSIZE. + * is \c FD_SETSIZE or greater. * * \param ctx Socket * \param buf The buffer to write to diff --git a/library/net_sockets.c b/library/net_sockets.c index 375434abd..ad1ac13fb 100644 --- a/library/net_sockets.c +++ b/library/net_sockets.c @@ -466,9 +466,9 @@ int mbedtls_net_poll( mbedtls_net_context *ctx, uint32_t rw, uint32_t timeout ) return( MBEDTLS_ERR_NET_INVALID_CONTEXT ); /* A limitation of select() is that it only works with file descriptors - * up to 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. */ + * 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 ); @@ -592,9 +592,9 @@ int mbedtls_net_recv_timeout( void *ctx, unsigned char *buf, return( MBEDTLS_ERR_NET_INVALID_CONTEXT ); /* A limitation of select() is that it only works with file descriptors - * up to 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. */ + * 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 ); From 97c57fe4392510187e67e17470dfcecf845c6c80 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 1 Mar 2021 11:40:56 +0100 Subject: [PATCH 6/8] Fix sloppiness around stricly less-than vs less or equal Fix sloppy wording around stricly less-than vs less or equal in comments. Also fix an off-by-one error in a comparison which led to calling setrlimit if the limit was exactly the minimum required for the test, which was unnecessary but harmless. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_net.function | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/suites/test_suite_net.function b/tests/suites/test_suite_net.function index 55fabd658..111a34950 100644 --- a/tests/suites/test_suite_net.function +++ b/tests/suites/test_suite_net.function @@ -75,8 +75,9 @@ void context_init_free( int reinit ) void poll_beyond_fd_setsize( ) { /* Test that mbedtls_net_poll does not misbehave when given a file - * descriptor beyond FD_SETSIZE. This code is specific to platforms - * with a Unix-like select() function. */ + * 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; @@ -87,15 +88,15 @@ void poll_beyond_fd_setsize( ) mbedtls_net_init( &ctx ); /* On many systems, by default, the maximum permitted file descriptor - * number is less or equal to FD_SETSIZE. If so, raise the limit if + * number is less than FD_SETSIZE. If so, raise the limit if * possible. * - * If the limit can't be raised, a newly open file descriptor - * won't be higher than FD_SETSIZE, so the test is not necessary and we - * mark it as skipped. + * 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. */ TEST_ASSERT( getrlimit( RLIMIT_NOFILE, &rlim_nofile ) == 0 ); - if( rlim_nofile.rlim_cur <= FD_SETSIZE + 1 ) + if( rlim_nofile.rlim_cur < FD_SETSIZE + 1 ) { rlim_t old_rlim_cur = rlim_nofile.rlim_cur; rlim_nofile.rlim_cur = FD_SETSIZE + 1; @@ -109,8 +110,8 @@ void poll_beyond_fd_setsize( ) /* 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 beyond FD_SETSIZE. So we expect to hit this platform - * limitation. + * 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 From 574cf7b59f295bc4000fc7fbe01213bb6c7ea93a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 1 Mar 2021 11:43:22 +0100 Subject: [PATCH 7/8] Clarify how a file descriptor could still be more than the limit It can happen if the file descriptor was opened before lowering the limit. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_net.function | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/suites/test_suite_net.function b/tests/suites/test_suite_net.function index 111a34950..f429fc922 100644 --- a/tests/suites/test_suite_net.function +++ b/tests/suites/test_suite_net.function @@ -94,6 +94,10 @@ void poll_beyond_fd_setsize( ) * 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 ) From 9264e01730a4a99614e228bdbdbdbcbb2f4ab642 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 3 Mar 2021 12:25:06 +0100 Subject: [PATCH 8/8] Update error codes listed in the net_sockets documentation Signed-off-by: Gilles Peskine --- include/mbedtls/net_sockets.h | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/net_sockets.h b/include/mbedtls/net_sockets.h index 702e93f1c..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 @@ -250,10 +253,11 @@ int mbedtls_net_send( void *ctx, const unsigned char *buf, size_t len ); * \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