From c062403abd71550057b3647b01cc8af4cc2fc18c Mon Sep 17 00:00:00 2001 From: Alexandr Nedvedicky Date: Fri, 26 Jan 2024 08:05:47 +0100 Subject: OpenSSL 3.2.0, QUIC, macOS, error 56 on connected UDP socket current `translate_msg()` function attempts to set `->msg_name` (and `->msg_namelen`) with `BIO`'s peer name (connection destination) regardless if underlying socket is connected or not. Such implementation uncovers differences in socket implementation between various OSes. As we have learned hard way `sendmsg()` and `sendmmsg()` on `OpenBSD` and (`MacOS` too) fail to send messages with `->msg_name` being set on connected socket. In such case the caller receives `EISCON` errro. I think `translate_msg()` caller should provide a hint to indicate whether we deal with connected (or un-connected) socket. For connected sockets the peer's name should not be set/filled by `translate_msg()`. On the other hand if socket is un-connected, then `translate_msg()` must populate `->msg_name` and `->msg_namelen` members. The caller can use `getpeername(2)` to see if socket is connected. If `getpeername()` succeeds then we must be dealing with connected socket and `translate_msg()` must not set `->msg_name` and `->msg_namelen` members. If `getpeername(2)` fails, then `translate_msg()` must provide peer's name (destination address) in `->msg_name` and set `->msg_namelen` accordingly. The propposed fix introduces `is_connected()` function, which applies `getpeername()` to socket bound to `BIO` instance. The `dgram_sendmmsg()` uses `is_connected()` as a hint for `translate_msg()` function, so msghdr gets initialized with respect to socket state. The change also modifies existing `test/quic_client_test.c` so it also covers the case of connected socket. To keep things simple we can introduce optional argument `connect_first` to `./quic_client_test` function. Without `connect_first` the test run as usual. With `connect_first` the test creates and connects socket first. Then it passes such socket to `BIO` sub-system to perform `QUIC` connect test as usual. Fixes #23251 Reviewed-by: Neil Horman Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/23396) --- crypto/bio/bss_dgram.c | 28 ++++++++++++----- test/quic_client_test.c | 84 +++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 88 insertions(+), 24 deletions(-) diff --git a/crypto/bio/bss_dgram.c b/crypto/bio/bss_dgram.c index 7dbdfe181a..f6d688b353 100644 --- a/crypto/bio/bss_dgram.c +++ b/crypto/bio/bss_dgram.c @@ -560,6 +560,8 @@ static long dgram_ctrl(BIO *b, int cmd, long num, void *ptr) socklen_t addr_len; BIO_ADDR addr; # endif + struct sockaddr_storage ss; + socklen_t ss_len = sizeof(ss); data = (bio_dgram_data *)b->ptr; @@ -577,6 +579,10 @@ static long dgram_ctrl(BIO *b, int cmd, long num, void *ptr) b->shutdown = (int)num; b->init = 1; dgram_update_local_addr(b); + if (getpeername(b->num, (struct sockaddr *)&ss, &ss_len) == 0) { + BIO_ADDR_make(&data->peer, BIO_ADDR_sockaddr((BIO_ADDR *)&ss)); + data->connected = 1; + } # if defined(SUPPORT_LOCAL_ADDR) if (data->local_addr_enabled) { if (enable_local_addr(b, 1) < 1) @@ -1067,19 +1073,27 @@ static void translate_msg_win(BIO *b, WSAMSG *mh, WSABUF *iov, static void translate_msg(BIO *b, struct msghdr *mh, struct iovec *iov, unsigned char *control, BIO_MSG *msg) { + bio_dgram_data *data; + iov->iov_base = msg->data; iov->iov_len = msg->data_len; - /* macOS requires msg_namelen be 0 if msg_name is NULL */ - mh->msg_name = msg->peer != NULL ? &msg->peer->sa : NULL; - if (msg->peer != NULL && dgram_get_sock_family(b) == AF_INET) - mh->msg_namelen = sizeof(struct sockaddr_in); + data = (bio_dgram_data *)b->ptr; + if (data->connected == 0) { + /* macOS requires msg_namelen be 0 if msg_name is NULL */ + mh->msg_name = msg->peer != NULL ? &msg->peer->sa : NULL; + if (msg->peer != NULL && dgram_get_sock_family(b) == AF_INET) + mh->msg_namelen = sizeof(struct sockaddr_in); # if OPENSSL_USE_IPV6 - else if (msg->peer != NULL && dgram_get_sock_family(b) == AF_INET6) - mh->msg_namelen = sizeof(struct sockaddr_in6); + else if (msg->peer != NULL && dgram_get_sock_family(b) == AF_INET6) + mh->msg_namelen = sizeof(struct sockaddr_in6); # endif - else + else + mh->msg_namelen = 0; + } else { + mh->msg_name = NULL; mh->msg_namelen = 0; + } mh->msg_iov = iov; mh->msg_iovlen = 1; diff --git a/test/quic_client_test.c b/test/quic_client_test.c index 5defd65939..43adc504c3 100644 --- a/test/quic_client_test.c +++ b/test/quic_client_test.c @@ -18,6 +18,9 @@ static const char msg1[] = "GET LICENSE.txt\r\n"; static char msg2[16000]; +#define DST_PORT 4433 +#define DST_ADDR 0x7f000001UL + static int is_want(SSL *s, int ret) { int ec = SSL_get_error(s, ret); @@ -25,42 +28,47 @@ static int is_want(SSL *s, int ret) return ec == SSL_ERROR_WANT_READ || ec == SSL_ERROR_WANT_WRITE; } -static int test_quic_client(void) +static int test_quic_client_ex(int fd_arg) { int testresult = 0, ret; - int c_fd = INVALID_SOCKET; + int c_fd; BIO *c_net_bio = NULL, *c_net_bio_own = NULL; BIO_ADDR *s_addr_ = NULL; struct in_addr ina = {0}; SSL_CTX *c_ctx = NULL; SSL *c_ssl = NULL; - short port = 4433; + short port = DST_PORT; int c_connected = 0, c_write_done = 0, c_shutdown = 0; size_t l = 0, c_total_read = 0; OSSL_TIME start_time; unsigned char alpn[] = { 8, 'h', 't', 't', 'p', '/', '0', '.', '9' }; - ina.s_addr = htonl(0x7f000001UL); - /* Setup test client. */ - c_fd = BIO_socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP, 0); - if (!TEST_int_ne(c_fd, INVALID_SOCKET)) - goto err; + if (fd_arg == INVALID_SOCKET) { + /* Setup test client. */ + c_fd = BIO_socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP, 0); + if (!TEST_int_ne(c_fd, INVALID_SOCKET)) + goto err; - if (!TEST_true(BIO_socket_nbio(c_fd, 1))) - goto err; + if (!TEST_true(BIO_socket_nbio(c_fd, 1))) + goto err; - if (!TEST_ptr(s_addr_ = BIO_ADDR_new())) - goto err; + if (!TEST_ptr(s_addr_ = BIO_ADDR_new())) + goto err; - if (!TEST_true(BIO_ADDR_rawmake(s_addr_, AF_INET, &ina, sizeof(ina), - htons(port)))) - goto err; + ina.s_addr = htonl(DST_ADDR); + if (!TEST_true(BIO_ADDR_rawmake(s_addr_, AF_INET, &ina, sizeof(ina), + htons(port)))) + goto err; + } else { + c_fd = fd_arg; + } if (!TEST_ptr(c_net_bio = c_net_bio_own = BIO_new_dgram(c_fd, 0))) goto err; - if (!BIO_dgram_set_peer(c_net_bio, s_addr_)) + /* connected socket does not need to set peer */ + if (s_addr_ != NULL && !BIO_dgram_set_peer(c_net_bio, s_addr_)) goto err; if (!TEST_ptr(c_ctx = SSL_CTX_new(OSSL_QUIC_client_method()))) @@ -157,11 +165,51 @@ err: SSL_CTX_free(c_ctx); BIO_ADDR_free(s_addr_); BIO_free(c_net_bio_own); - if (c_fd != INVALID_SOCKET) + if (fd_arg == INVALID_SOCKET && c_fd != INVALID_SOCKET) BIO_closesocket(c_fd); return testresult; } +static int test_quic_client(void) +{ + return (test_quic_client_ex(INVALID_SOCKET)); +} + +static int test_quic_client_connect_first(void) +{ + struct sockaddr_in sin = {0}; + int c_fd; + int rv; + +#ifdef SA_LEN + sin.sin_len = sizeof(struct sockaddr_in); +#endif + sin.sin_family = AF_INET; + sin.sin_port = htons(DST_PORT); + sin.sin_addr.s_addr = htonl(DST_ADDR); + + c_fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP); + if (!TEST_int_ne(c_fd, INVALID_SOCKET)) + goto err; + + if (!TEST_int_eq(connect(c_fd, (const struct sockaddr *)&sin, sizeof(sin)), 0)) + goto err; + + if (!TEST_true(BIO_socket_nbio(c_fd, 1))) + goto err; + + rv = test_quic_client_ex(c_fd); + + close(c_fd); + + return (rv); + +err: + if (c_fd != INVALID_SOCKET) + close(c_fd); + return (0); +} + OPT_TEST_DECLARE_USAGE("certfile privkeyfile\n") int setup_tests(void) @@ -172,5 +220,7 @@ int setup_tests(void) } ADD_TEST(test_quic_client); + ADD_TEST(test_quic_client_connect_first); + return 1; } -- cgit v1.2.3