From 8ae9cf4698b4fadae8cfbfbc801cf93d2385629d Mon Sep 17 00:00:00 2001 From: Bryan Jacobs Date: Sun, 27 Mar 2022 13:07:29 +1100 Subject: [PATCH 1/3] Revert "libnm: buffer reads in nm_vpn_service_plugin_read_vpn_details()" This partially reverts commit 4a9fcb0fc32e, which replaced one-byte reads with buffered ones in the VPN service plugin. Unfortunately the buffering means that commands coming after the magic "DONE" string were being pulled into the buffer. Secrets agents expect a "QUIT" to come after the "DONE", and since with buffering "QUIT" was in the buffer, this led to a twenty-second delay on every VPN connection using a secrets manager. https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1164 Fixes: 4a9fcb0fc32e ('libnm: buffer reads in nm_vpn_service_plugin_read_vpn_details()') --- src/libnm-client-impl/nm-vpn-service-plugin.c | 84 +++++-------------- 1 file changed, 20 insertions(+), 64 deletions(-) diff --git a/src/libnm-client-impl/nm-vpn-service-plugin.c b/src/libnm-client-impl/nm-vpn-service-plugin.c index 2a217502de..7b04c30799 100644 --- a/src/libnm-client-impl/nm-vpn-service-plugin.c +++ b/src/libnm-client-impl/nm-vpn-service-plugin.c @@ -729,54 +729,6 @@ nm_vpn_service_plugin_secrets_required(NMVpnServicePlugin *plugin, /*****************************************************************************/ -typedef struct { - char *buf; - gsize n_buf; - int fd; - bool eof : 1; - char buf_full[1024]; -} ReadFdBuf; - -static inline gboolean -_read_fd_buf_c(ReadFdBuf *read_buf, char *ch) -{ - gssize n_read; - - if (read_buf->n_buf > 0) - goto out_data; - if (read_buf->eof) - return FALSE; - -again: - n_read = read(read_buf->fd, read_buf->buf_full, sizeof(read_buf->buf_full)); - if (n_read <= 0) { - if (n_read < 0 && errno == EAGAIN) { - struct pollfd pfd; - int r; - - memset(&pfd, 0, sizeof(pfd)); - pfd.fd = read_buf->fd; - pfd.events = POLLIN; - - r = poll(&pfd, 1, -1); - if (r > 0) - goto again; - /* error or timeout. Fall through and set EOF. */ - } - read_buf->eof = TRUE; - return FALSE; - } - - read_buf->buf = read_buf->buf_full; - read_buf->n_buf = n_read; - -out_data: - read_buf->n_buf--; - *ch = read_buf->buf[0]; - read_buf->buf++; - return TRUE; -} - #define DATA_KEY_TAG "DATA_KEY=" #define DATA_VAL_TAG "DATA_VAL=" #define SECRET_KEY_TAG "SECRET_KEY=" @@ -808,7 +760,7 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable nm_auto_free_gstring GString *val = NULL; nm_auto_free_gstring GString *line = NULL; GString *str = NULL; - ReadFdBuf read_buf; + char c; if (out_data) g_return_val_if_fail(*out_data == NULL, FALSE); @@ -819,27 +771,31 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable secrets = g_hash_table_new_full(nm_str_hash, g_str_equal, g_free, (GDestroyNotify) nm_free_secret); - read_buf.buf = NULL; - read_buf.n_buf = 0; - read_buf.fd = fd; - read_buf.eof = FALSE; - line = g_string_new(NULL); /* Read stdin for data and secret items until we get a DONE */ while (1) { - gboolean eof; - char c = '\0'; + ssize_t nr; - eof = !_read_fd_buf_c(&read_buf, &c); + nr = read(fd, &c, 1); + if (nr < 0) { + if (errno == EAGAIN) { + struct pollfd pfd; + int r; - if (!eof && c == '\0') { - /* On the first '\0' char, we also assume the data is finished. Abort. */ - read_buf.eof = TRUE; - eof = TRUE; + memset(&pfd, 0, sizeof(pfd)); + pfd.fd = fd; + pfd.events = POLLIN; + + r = poll(&pfd, 1, -1); + if (r > 0) + continue; + + /* error or timeout. Fall through and break. */ + } + break; } - - if (!eof && c != '\n') { + if (nr > 0 && c != '\n') { g_string_append_c(line, c); if (line->len > 512 * 1024) { /* we are about to read a huge line. That is not right, abort. */ @@ -893,7 +849,7 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable next: g_string_truncate(line, 0); - if (eof) + if (nr == 0) break; } From 6235815248314c0bd3deb485692881620a859cf9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 28 Mar 2022 09:40:22 +0200 Subject: [PATCH 2/3] libnm: handle NUL characters in nm_vpn_service_plugin_read_vpn_details() and fix test We expect to read NUL terminated strings. Upon NUL, we should do something. Treat it as a line break. Fixes: 8ae9cf4698b4 ('Revert "libnm: buffer reads in nm_vpn_service_plugin_read_vpn_details()"') --- src/libnm-client-impl/nm-vpn-service-plugin.c | 6 ++++++ src/libnm-client-impl/tests/test-libnm.c | 10 ++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/libnm-client-impl/nm-vpn-service-plugin.c b/src/libnm-client-impl/nm-vpn-service-plugin.c index 7b04c30799..3493d1db10 100644 --- a/src/libnm-client-impl/nm-vpn-service-plugin.c +++ b/src/libnm-client-impl/nm-vpn-service-plugin.c @@ -795,6 +795,12 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable } break; } + + if (nr > 0 && c == '\0') { + /* '\0' are not supported. Replace with newline. */ + c = '\n'; + } + if (nr > 0 && c != '\n') { g_string_append_c(line, c); if (line->len > 512 * 1024) { diff --git a/src/libnm-client-impl/tests/test-libnm.c b/src/libnm-client-impl/tests/test-libnm.c index d3265e9fb4..f24828f57e 100644 --- a/src/libnm-client-impl/tests/test-libnm.c +++ b/src/libnm-client-impl/tests/test-libnm.c @@ -2550,8 +2550,14 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "DONE\n" "\n" "", - READ_VPN_DETAIL_DATA({"some\nkey-2", "val2"}, {"some-key", "string"}, ), - READ_VPN_DETAIL_DATA(), ); + READ_VPN_DETAIL_DATA({"some\nkey-2", "val2"}, + {"some-key", "string"}, + {"key3\nkey-2", "val3"}, ), + READ_VPN_DETAIL_DATA({"some-secret", "val3"}, + {"key-inval", "in\xc1val"}, + {"ke\xc0yx", "inval"}, + {"keyx", ""}, + {"", "val3"}), ); } /*****************************************************************************/ From b1b1ee8cc41af4f57902560f6769daf15852311e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 28 Mar 2022 10:37:08 +0200 Subject: [PATCH 3/3] libnm/tests: test that nm_vpn_service_plugin_read_vpn_details() does not consume "QUIT" command --- src/libnm-client-impl/tests/test-libnm.c | 95 ++++++++++++++++++++++-- 1 file changed, 88 insertions(+), 7 deletions(-) diff --git a/src/libnm-client-impl/tests/test-libnm.c b/src/libnm-client-impl/tests/test-libnm.c index f24828f57e..1810e139e9 100644 --- a/src/libnm-client-impl/tests/test-libnm.c +++ b/src/libnm-client-impl/tests/test-libnm.c @@ -2272,6 +2272,12 @@ _memfd_create(const char *name) return -1; } +typedef enum { + READ_VPN_DETAIL_TYPE_GOOD, + READ_VPN_DETAIL_TYPE_NO_DONE, + READ_VPN_DETAIL_TYPE_BROKEN, +} ReadVpnDetailType; + typedef struct { const char *key; const char *val; @@ -2285,6 +2291,7 @@ _do_read_vpn_details_impl1(const char *file, int memfd, char *mem, gsize len, + ReadVpnDetailType detail_type, const ReadVpnDetailData *expected_data, guint expected_data_len, const ReadVpnDetailData *expected_secrets, @@ -2294,10 +2301,27 @@ _do_read_vpn_details_impl1(const char *file, off_t lseeked; gs_unref_hashtable GHashTable *data = NULL; gs_unref_hashtable GHashTable *secrets = NULL; + char ch; + gboolean append_quit; + char read_buf[1024]; + gssize n_read; + gssize i; written = write(memfd, mem, len); g_assert_cmpint(written, ==, (gssize) len); + append_quit = nmtst_get_rand_bool(); + + if (append_quit) { + if (len > 0 && mem[len - 1] != '\n') { + ch = '\n'; + written = write(memfd, &ch, 1); + g_assert_cmpint(written, ==, 1); + } + written = write(memfd, "QUIT", 4); + g_assert_cmpint(written, ==, 4); + } + lseeked = lseek(memfd, 0, SEEK_SET); g_assert_cmpint(lseeked, ==, 0); @@ -2365,10 +2389,58 @@ _do_read_vpn_details_impl1(const char *file, NM_PRAGMA_WARNING_REENABLE #undef _assert_hash + + n_read = read(memfd, read_buf, sizeof(read_buf)); + + if (0) { + gs_free char *ss = NULL; + + g_print(">>>> n_read=%zd; \"%s\"", + n_read, + n_read > 0 ? ( + ss = nm_utils_buf_utf8safe_escape_cp(read_buf, + n_read, + NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL)) + : ""); + } + + g_assert_cmpint(n_read, >=, 0); + g_assert_cmpint(n_read, <, sizeof(read_buf)); + + i = read(memfd, &ch, 1); + g_assert_cmpint(i, ==, 0); + + switch (detail_type) { + case READ_VPN_DETAIL_TYPE_GOOD: + g_assert_cmpint(n_read, >=, 0); + i = 0; + while (i < n_read && NM_IN_SET(read_buf[i], '\n')) + i++; + if (append_quit) + g_assert_cmpmem("QUIT", 4, &read_buf[i], n_read - i); + else + g_assert_cmpint(n_read - i, ==, 0); + break; + case READ_VPN_DETAIL_TYPE_NO_DONE: + g_assert_cmpint(n_read, ==, 0); + break; + case READ_VPN_DETAIL_TYPE_BROKEN: + g_assert_cmpint(n_read, >, 0); + if (append_quit) { + g_assert_cmpint(n_read, >, 4); + g_assert(memmem(&read_buf[i], n_read + i, "QUIT", 4)); + } + break; + default: + g_assert_not_reached(); + break; + } + return TRUE; } #define _do_read_vpn_details_impl0(str, \ + detail_type, \ expected_data, \ expected_data_len, \ expected_secrets, \ @@ -2389,6 +2461,7 @@ _do_read_vpn_details_impl1(const char *file, _memfd, \ "" str "", \ NM_STRLEN(str), \ + (detail_type), \ expected_data, \ expected_data_len, \ expected_secrets, \ @@ -2397,14 +2470,16 @@ _do_read_vpn_details_impl1(const char *file, } \ G_STMT_END -#define _do_read_vpn_details_empty(str) _do_read_vpn_details_impl0(str, NULL, 0, NULL, 0, {}) +#define _do_read_vpn_details_empty(str) \ + _do_read_vpn_details_impl0(str, READ_VPN_DETAIL_TYPE_GOOD, NULL, 0, NULL, 0, {}) -#define _do_read_vpn_details(str, expected_data, expected_secrets, pre_setup_cmd) \ - _do_read_vpn_details_impl0(str, \ - expected_data, \ - G_N_ELEMENTS(expected_data), \ - expected_secrets, \ - G_N_ELEMENTS(expected_secrets), \ +#define _do_read_vpn_details(str, detail_type, expected_data, expected_secrets, pre_setup_cmd) \ + _do_read_vpn_details_impl0(str, \ + detail_type, \ + expected_data, \ + G_N_ELEMENTS(expected_data), \ + expected_secrets, \ + G_N_ELEMENTS(expected_secrets), \ pre_setup_cmd) static void @@ -2430,6 +2505,7 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "DONE\n" "\n" "", + READ_VPN_DETAIL_TYPE_GOOD, READ_VPN_DETAIL_DATA({"some-key", "string"}, {"some-other-key", "val2"}, ), READ_VPN_DETAIL_DATA({"some-secret", "val3"}, ), ); @@ -2437,6 +2513,7 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "DATA_KEY=some-key\n" "DATA_VAL=string\n" "DONE\n", + READ_VPN_DETAIL_TYPE_GOOD, READ_VPN_DETAIL_DATA({"some-key", "string"}, ), READ_VPN_DETAIL_DATA(), ); @@ -2465,6 +2542,7 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "\n" "DONE\n" "", + READ_VPN_DETAIL_TYPE_BROKEN, READ_VPN_DETAIL_DATA({"some-key", "string\ncontinued after a line break"}, ), READ_VPN_DETAIL_DATA({"key names\ncan have\ncontinuations too", "value"}, ), ); @@ -2478,6 +2556,7 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "=continuations too\n" "SECRET_VAL=value\n" "", + READ_VPN_DETAIL_TYPE_NO_DONE, READ_VPN_DETAIL_DATA({"some-key", "string\ncontinued after a line break"}, ), READ_VPN_DETAIL_DATA({"key names\ncan have\ncontinuations too", "value"}, ), ); @@ -2511,6 +2590,7 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "DONE\n" "\n" "", + READ_VPN_DETAIL_TYPE_GOOD, READ_VPN_DETAIL_DATA({"some\nkey-2", "val2"}, {"some-key", "string"}, {"key3\nkey-2", "val3"}, ), @@ -2550,6 +2630,7 @@ test_nm_vpn_service_plugin_read_vpn_details(void) "DONE\n" "\n" "", + READ_VPN_DETAIL_TYPE_GOOD, READ_VPN_DETAIL_DATA({"some\nkey-2", "val2"}, {"some-key", "string"}, {"key3\nkey-2", "val3"}, ),