diff --git a/src/client/client.c b/src/client/client.c index e4364b33..3a1b794c 100644 --- a/src/client/client.c +++ b/src/client/client.c @@ -35,6 +35,7 @@ #include #include +#include #include @@ -95,6 +96,11 @@ static const char *wolfsentry_config_path = NULL; #define MAX_NON_BLOCK_SEC 10 #endif +/* Scratch buffer for a decimal command line argument. LONG_MAX is 19 decimal + * digits, so 24 leaves room for that plus a NUL and a little slack; anything + * longer is out of range for the values parsed with it. */ +#define MAX_DECIMAL_ARG_LEN 24 + #define OCSP_STAPLING 1 #define OCSP_STAPLINGV2 2 #define OCSP_STAPLINGV2_MULTI 3 @@ -236,6 +242,7 @@ static WC_INLINE void clu_build_addr(SOCKADDR_IN4_T* addr, SOCKADDR_IN6_T* ipv6, if (getaddrinfo((char*)peer, portStr, &hints, &addrInfo) == 0) { XMEMCPY(addr, addrInfo->ai_addr, sizeof(*addr)); useLookup = 1; + freeaddrinfo(addrInfo); } #endif else @@ -311,22 +318,20 @@ static WC_INLINE void clu_build_addr(SOCKADDR_IN4_T* addr, SOCKADDR_IN6_T* ipv6, static WC_INLINE void clu_tcp_socket(SOCKET_T* sockfd, int udp, int sctp, int isIpv6) { + /* The address built by clu_build_addr() is AF_INET6 whenever isIpv6 is + * set, so every socket type has to be opened in the matching family. */ + int family = isIpv6 ? AF_INET6_V : AF_INET_V; + (void)sctp; if (udp) - *sockfd = socket(AF_INET_V, SOCK_DGRAM, IPPROTO_UDP); + *sockfd = socket(family, SOCK_DGRAM, IPPROTO_UDP); #ifdef WOLFSSL_SCTP else if (sctp) - *sockfd = socket(AF_INET_V, SOCK_STREAM, IPPROTO_SCTP); + *sockfd = socket(family, SOCK_STREAM, IPPROTO_SCTP); #endif - else { - if (isIpv6) { - *sockfd = socket(AF_INET6_V, SOCK_STREAM, IPPROTO_TCP); - } - else { - *sockfd = socket(AF_INET_V, SOCK_STREAM, IPPROTO_TCP); - } - } + else + *sockfd = socket(family, SOCK_STREAM, IPPROTO_TCP); if(WOLFSSL_SOCKET_IS_INVALID(*sockfd)) { err_sys_with_errno("socket failed\n"); } @@ -371,7 +376,11 @@ static WC_INLINE void clu_tcp_connect(SOCKET_T* sockfd, const char* ip, if (isIpv6) { clu_build_addr(NULL, &ipv6, ip, port, udp, sctp); + if (udp) { + wolfSSL_dtls_set_peer(ssl, &ipv6, sizeof(ipv6)); + } clu_tcp_socket(sockfd, udp, sctp, isIpv6); + if (!udp) { if (WOLFSSL_SOCKET_IS_INVALID(*sockfd)) err_sys_with_errno("tcp bad socket"); @@ -1103,6 +1112,7 @@ static int ClientBenchmarkThroughput(WOLFSSL_CTX* ctx, char* host, word16 port, if (ret == WOLFSSL_SUCCESS) { /* Perform throughput test */ char *tx_buffer, *rx_buffer; + const char* errMsg = NULL; /* Record connection time */ conn_time = current_time(0) - start; @@ -1133,9 +1143,13 @@ static int ClientBenchmarkThroughput(WOLFSSL_CTX* ctx, char* host, word16 port, xfer_bytes = 0; while (throughput > xfer_bytes) { int len, rx_pos, select_ret; + size_t remain; - /* Determine packet size */ - len = min(block, (int)(throughput - xfer_bytes)); + /* Determine packet size. Bound the size_t remainder by + * the block size before narrowing it to int, otherwise a + * remainder above INT_MAX becomes a negative length. */ + remain = throughput - xfer_bytes; + len = (remain > (size_t)block) ? block : (int)remain; /* Perform TX */ start = current_time(1); @@ -1154,48 +1168,64 @@ static int ClientBenchmarkThroughput(WOLFSSL_CTX* ctx, char* host, word16 port, } while (err == WC_PENDING_E); if (ret != len) { printf("SSL_write bench error %d!\n", err); - if (!exitWithRet) - err_sys("SSL_write failed"); + if (err == 0) + err = WOLFSSL_FATAL_ERROR; + errMsg = "SSL_write failed"; goto doExit; } tx_time += current_time(0) - start; /* Perform RX */ select_ret = tcp_select(sockfd, DEFAULT_TIMEOUT_SEC); - if (select_ret == TEST_RECV_READY) { - start = current_time(1); - rx_pos = 0; - while (rx_pos < len) { - ret = wolfSSL_read(ssl, &rx_buffer[rx_pos], - len - rx_pos); - if (ret <= 0) { - err = wolfSSL_get_error(ssl, 0); - #ifdef WOLFSSL_ASYNC_CRYPT - if (err == WC_PENDING_E) { - ret = wolfSSL_AsyncPoll(ssl, WOLF_POLL_FLAG_CHECK_HW); - if (ret < 0) break; - } - else - #endif - if (err != WOLFSSL_ERROR_WANT_READ) { - printf("SSL_read bench error %d\n", err); - err_sys("SSL_read failed"); - } + if (select_ret != TEST_RECV_READY) { + printf("SSL_read bench select error %d!\n", select_ret); + err = WOLFSSL_FATAL_ERROR; + errMsg = (select_ret == TEST_TIMEOUT) ? + "SSL_read timeout" : "SSL_read select failed"; + goto doExit; + } + + start = current_time(1); + rx_pos = 0; + while (rx_pos < len) { + ret = wolfSSL_read(ssl, &rx_buffer[rx_pos], + len - rx_pos); + if (ret <= 0) { + err = wolfSSL_get_error(ssl, 0); + #ifdef WOLFSSL_ASYNC_CRYPT + if (err == WC_PENDING_E) { + ret = wolfSSL_AsyncPoll(ssl, WOLF_POLL_FLAG_CHECK_HW); + if (ret < 0) break; } - else { - rx_pos += ret; + else + #endif + if (err != WOLFSSL_ERROR_WANT_READ) { + break; } } - rx_time += current_time(0) - start; + else { + rx_pos += ret; + } + } + rx_time += current_time(0) - start; + + /* Only compare once the full block has been received */ + if (rx_pos != len) { + printf("SSL_read bench error %d!\n", err); + /* wolfSSL_read() can return <= 0 without setting an + * error code, so normalize it here. Otherwise a short + * read would be returned as EXIT_SUCCESS under -H. */ + if (err == 0) + err = WOLFSSL_FATAL_ERROR; + errMsg = "SSL_read failed"; + goto doExit; } /* Compare TX and RX buffers */ if (XMEMCMP(tx_buffer, rx_buffer, len) != 0) { - free(tx_buffer); - tx_buffer = NULL; - free(rx_buffer); - rx_buffer = NULL; - err_sys("Compare TX and RX buffers failed"); + err = WOLFSSL_FATAL_ERROR; + errMsg = "Compare TX and RX buffers failed"; + goto doExit; } /* Update overall position */ @@ -1213,6 +1243,10 @@ static int ClientBenchmarkThroughput(WOLFSSL_CTX* ctx, char* host, word16 port, doExit: if (tx_buffer) XFREE(tx_buffer, NULL, DYNAMIC_TYPE_TMP_BUFFER); if (rx_buffer) XFREE(rx_buffer, NULL, DYNAMIC_TYPE_TMP_BUFFER); + /* err_sys() does not return, so only report the failure once the + * buffers have been released */ + if (errMsg != NULL && !exitWithRet) + err_sys(errMsg); } else { err_sys("wolfSSL_connect failed"); @@ -2486,12 +2520,23 @@ THREAD_RETURN WOLFSSL_THREAD client_test(void* args) break; case 'p' : - port = (word16)atoi(myoptarg); + { + long portArg = 0; + /* 0 is not a usable port to connect to, and the port == 0 + * check below is compiled out of every wolfCLU build + * (wolfclu/client.h defines NO_MAIN_DRIVER), so reject it + * here. */ + if (wolfCLU_parseDecimalBounded(myoptarg, 1, 65535, &portArg) + != WOLFCLU_SUCCESS) { + err_sys("port number must be between 1 and 65535"); + } + port = (word16)portArg; #if !defined(NO_MAIN_DRIVER) || defined(USE_WINDOWS_API) if (port == 0) err_sys("port number cannot be 0"); #endif break; + } case 'v' : if (myoptarg[0] == 'd') { @@ -2598,18 +2643,42 @@ THREAD_RETURN WOLFSSL_THREAD client_test(void* args) break; case 'B' : - throughput = atol(myoptarg); - for (; *myoptarg != '\0'; myoptarg++) { - if (*myoptarg == ',') { - block = atoi(myoptarg + 1); - break; - } + { + long throughputArg = 0; + long blockArg = (long)block; + const char* comma = XSTRSTR(myoptarg, ","); + size_t numLen = (comma != NULL) ? + (size_t)(comma - myoptarg) : XSTRLEN(myoptarg); + char numBuf[MAX_DECIMAL_ARG_LEN]; + + /* Parse both halves with the bounded decimal parser so that + * negative, out-of-range and trailing-junk input is rejected + * instead of silently saturating or truncating the way + * atol()/atoi() do, which would let the benchmark run with a + * wrapped size_t throughput. */ + if (numLen == 0 || numLen >= sizeof(numBuf)) { + Usage(); + XEXIT_T(MY_EX_USAGE); } - if (throughput == 0 || block <= 0) { + XMEMCPY(numBuf, myoptarg, numLen); + numBuf[numLen] = '\0'; + + if (wolfCLU_parseDecimalBounded(numBuf, 1, LONG_MAX, + &throughputArg) != WOLFCLU_SUCCESS) { + Usage(); + XEXIT_T(MY_EX_USAGE); + } + if (comma != NULL && + wolfCLU_parseDecimalBounded(comma + 1, 1, INT_MAX, + &blockArg) != WOLFCLU_SUCCESS) { Usage(); XEXIT_T(MY_EX_USAGE); } + + throughput = (size_t)throughputArg; + block = (int)blockArg; break; + } case 'N' : nonBlocking = 1; diff --git a/tests/client/client-test.py b/tests/client/client-test.py index 4fbca71d..c83d08e2 100644 --- a/tests/client/client-test.py +++ b/tests/client/client-test.py @@ -7,7 +7,8 @@ import unittest sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) -from wolfclu_test import WOLFSSL_BIN, CERTS_DIR, run_wolfssl, test_main +from wolfclu_test import (WOLFSSL_BIN, CERTS_DIR, run_wolfssl, + skip_if_no_filesystem, test_main) class ClientTest(unittest.TestCase): @@ -17,11 +18,7 @@ def setUpClass(cls): if not os.path.isdir(CERTS_DIR): raise unittest.SkipTest("certs directory not found") - config_log = os.path.join(".", "config.log") - if os.path.isfile(config_log): - with open(config_log, "r") as f: - if "disable-filesystem" in f.read(): - raise unittest.SkipTest("filesystem support disabled") + skip_if_no_filesystem() def test_s_client_x509(self): """Connect to a TLS server, extract cert, and verify PEM output.""" @@ -66,6 +63,65 @@ def test_client_help(self): self.assertEqual(r.returncode, 0, r.stderr) self.assertIn("s_client" , r.stderr, "help menu was not printed") +class PortArgTest(unittest.TestCase): + """Regression tests for the port half of -connect :. + + -connect forwards the text after ':' to the client's -p handler, which + used to be (word16)atoi(): "99999" silently truncated to 34463 and the + client happily connected to the wrong port. The port is now parsed with + wolfCLU_parseDecimalBounded(), so anything outside 1-65535, and anything + that is not a plain decimal number, is rejected before any connection is + attempted. + """ + + PORT_ERROR = "port number must be between 1 and 65535" + + @classmethod + def setUpClass(cls): + # s_client is compiled out under --disable-filesystem, so it never + # reaches the port parser and never prints PORT_ERROR. + skip_if_no_filesystem() + + def _assert_rejected(self, port, description): + """Run s_client against localhost: and require that the port + itself was rejected -- a non-zero exit alone is not enough, since a + truncated port would also fail to connect.""" + r = run_wolfssl("s_client", "-connect", "localhost:" + port, + timeout=30) + self.assertNotEqual(r.returncode, 0, + f"{description} port {port!r} was accepted") + self.assertIn(self.PORT_ERROR, r.stdout + r.stderr, + f"{description} port {port!r} was not rejected as an " + f"out-of-range port: {r.stdout + r.stderr}") + + def test_port_zero(self): + """0 must be rejected: it is not a port that can be connected to.""" + self._assert_rejected("0", "zero") + + def test_port_above_word16(self): + """99999 must be rejected, not truncated to 34463.""" + self._assert_rejected("99999", "out-of-range") + + def test_port_just_above_word16(self): + """65536 must be rejected, not truncated to 0.""" + self._assert_rejected("65536", "out-of-range") + + def test_port_negative(self): + """A negative port must be rejected, not wrapped.""" + self._assert_rejected("-1", "negative") + + def test_port_non_numeric(self): + """A non-numeric port must be rejected, not read as 0.""" + self._assert_rejected("abc", "non-numeric") + + def test_port_trailing_junk(self): + """Trailing junk must be rejected, not silently ignored.""" + self._assert_rejected("443abc", "trailing junk") + + def test_port_empty(self): + """An empty port must be rejected.""" + self._assert_rejected("", "empty") + class ShellInjectionTest(unittest.TestCase): """Regression tests for shell command injection via hostname. diff --git a/tests/wolfclu_test.py b/tests/wolfclu_test.py index 6909c2e3..5a13a3ee 100644 --- a/tests/wolfclu_test.py +++ b/tests/wolfclu_test.py @@ -88,6 +88,20 @@ def run_wolfssl(*args, stdin_data=None, timeout=60): return subprocess.run(cmd, **kwargs) +def skip_if_no_filesystem(): + """Raise SkipTest when wolfCLU was built with --disable-filesystem. + + WOLFCLU_NO_FILESYSTEM compiles out the front ends that need files (e.g. + wolfCLU_Client), so those commands only print "No filesystem support" + instead of doing any work. + """ + config_log = os.path.join(".", "config.log") + if os.path.isfile(config_log): + with open(config_log, "r") as f: + if "disable-filesystem" in f.read(): + raise unittest.SkipTest("filesystem support disabled") + + def is_fips(): """True when linked against a FIPS wolfSSL build (per `wolfssl -v`).""" r = run_wolfssl("-v")