diff mbox series

[Openvpn-devel,v2] openvpnserv: skip conversion of empty domain names

Message ID 20261031225615.18613-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v2] openvpnserv: skip conversion of empty domain names |

Commit Message

Gert Doering Oct. 31, 2026, 10:56 p.m. UTC 
  From: Heiko Hund <heiko@ist.eigentlich.net>

Empty entries in an interface's comma separated domain list, were
converted to "." in the resulting MULTI_SZ and used as a match all
in the resulting NRPT rules, so all DNS queries would be sent
to that adapter's name server(s) instead of only the ones for its
domains. Such lists are not expected to be found in the registry,
but skip empty entries anyway, just to be sure.

Also rework the ConvertItfDnsDomains() unit tests to use a helper
that runs the conversion on an individually sized heap buffer with
everything after the input poisoned. This detects terminators that
are counted but not written, which zero-initialized stack buffers
hid. Also add more tests for empty entries and buffer size boundaries.

Bonus: fix typo in related doxygen comment.

Reported-by: Marc Heuse <marc@srlabs.de>
Change-Id: Id21a6b106c49d6647783e09a506add0726a1a6d2
Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net>
Acked-by: Gert Doering <gert@greenie.muc.de>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/2016
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/2016
This mail reflects revision 2 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <gert@greenie.muc.de>
diff mbox series

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index ed163a4..1a441ec 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -2272,6 +2272,7 @@ 
  * In case of an error \p size is set to 0 and the contents of \p domains
  * are invalid.
  * Note that domains are deleted from the string if they match a search domain.
+ * Empty entries (e.g. from leading, trailing or double commas) are skipped.
  *
  * @param[in]     search_domains  optional string of comma separated search domains
  * @param[in,out] domains         buffer that contains the input comma-separated
@@ -2313,7 +2314,7 @@ 
         PWCHAR comma = wcschr(domain, L',');
         size_t domain_len = comma ? (size_t)(comma - domain) : wcslen(domain);
 
-        if (ListContainsDomain(search_domains, domain, domain_len))
+        if (domain_len == 0 || ListContainsDomain(search_domains, domain, domain_len))
         {
             /* Skip this domain */
             domain = comma ? comma + 1 : domain + domain_len;
@@ -2366,7 +2367,7 @@ 
 /**
  * Return interface specific domain suffix(es)
  *
- * The \p domains paramter will be set to a MULTI_SZ domains string.
+ * The \p domains parameter will be set to a MULTI_SZ domains string.
  * In case of an error or if no domains are found for the interface
  * \p size is set to 0 and the contents of \p domains are invalid.
  * Note that the domains could have been set by DHCP or manually.
diff --git a/tests/unit_tests/openvpnserv/test_openvpnserv.c b/tests/unit_tests/openvpnserv/test_openvpnserv.c
index 4ca715b..a99a6f1 100644
--- a/tests/unit_tests/openvpnserv/test_openvpnserv.c
+++ b/tests/unit_tests/openvpnserv/test_openvpnserv.c
@@ -56,101 +56,102 @@ 
     assert_false(ListContainsDomain(L"internal.openvpn.net,openvpn.com", domain, domain_len));
 }
 
+/*
+ * Run ConvertItfDnsDomains() on a heap buffer of exactly capacity bytes,
+ * so that ASAN can detect overruns, with everything after the input string
+ * poisoned, so that terminators which are counted but not written show.
+ */
+static void
+check_convert(PCWSTR search_domains, PCWSTR input, DWORD capacity,
+              LSTATUS expected_err, PCWSTR expected)
+{
+    const DWORD glyph_size = sizeof(wchar_t);
+
+    /* Size of the expected MULTI_SZ including both terminators, or 0 */
+    DWORD expected_size = 0;
+    if (expected)
+    {
+        PCWSTR end = expected;
+        while (*end)
+        {
+            end += wcslen(end) + 1;
+        }
+        expected_size = (DWORD)(end - expected + 1) * glyph_size;
+    }
+
+    DWORD size = (DWORD)(wcslen(input) + 1) * glyph_size;
+    assert_true(size <= capacity);
+
+    wchar_t *domains = malloc(capacity);
+    assert_non_null(domains);
+    memset(domains, 0x41, capacity);
+    memcpy(domains, input, size);
+
+    LSTATUS err = ConvertItfDnsDomains(search_domains, domains, &size, capacity);
+    assert_int_equal(err, expected_err);
+    assert_int_equal(size, expected_size);
+    if (expected_size)
+    {
+        assert_memory_equal(domains, expected, expected_size);
+    }
+    free(domains);
+}
+
 #define BUF_SIZE 64
 static void
 test_convert_itf_dns_domains(void **state)
 {
-    DWORD size, len;
-    LSTATUS err;
     const DWORD glyph_size = sizeof(wchar_t);
+    const DWORD cap = BUF_SIZE * glyph_size;
 
     /* Remove the domain from a single-entry list */
-    wchar_t domains_0[BUF_SIZE] = L"openvpn.com";
-    len = (DWORD)wcslen(domains_0) + 1;
-    size = len * glyph_size;
-    err = ConvertItfDnsDomains(L"openvpn.com", domains_0, &size, sizeof(domains_0));
-    assert_int_equal(size, 0);
-    assert_int_equal(err, ERROR_FILE_NOT_FOUND);
+    check_convert(L"openvpn.com", L"openvpn.com", cap, ERROR_FILE_NOT_FOUND, NULL);
 
     /* Remove no domain from a single-entry list */
-    wchar_t domains_1[BUF_SIZE] = L"openvpn.com";
-    len = (DWORD)wcslen(domains_1) + 1;
-    size = len * glyph_size;
-    wchar_t domains_1_res[] = L".openvpn.com\0";
-    err = ConvertItfDnsDomains(L"openvpn.net", domains_1, &size, sizeof(domains_1));
-    assert_memory_equal(domains_1, domains_1_res, size);
-    assert_int_equal(size, sizeof(domains_1_res));
-    assert_int_equal(err, NO_ERROR);
+    check_convert(L"openvpn.net", L"openvpn.com", cap, NO_ERROR, L".openvpn.com\0");
 
     /* Remove the second domain from a two-entry list */
-    wchar_t domains_2[BUF_SIZE] = L"openvpn.com,openvpn.net";
-    len = (DWORD)wcslen(domains_2) + 1;
-    size = len * glyph_size;
-    wchar_t domains_2_res[] = L".openvpn.com\0";
-    err = ConvertItfDnsDomains(L"openvpn.net", domains_2, &size, sizeof(domains_2));
-    assert_memory_equal(domains_2, domains_2_res, size);
-    assert_int_equal(size, sizeof(domains_2_res));
-    assert_int_equal(err, NO_ERROR);
+    check_convert(L"openvpn.net", L"openvpn.com,openvpn.net", cap, NO_ERROR, L".openvpn.com\0");
 
     /* Remove the first domain from a two-entry list */
-    wchar_t domains_3[BUF_SIZE] = L"openvpn.com,openvpn.net";
-    len = (DWORD)wcslen(domains_3) + 1;
-    size = len * glyph_size;
-    wchar_t domains_3_res[] = L".openvpn.net\0";
-    err = ConvertItfDnsDomains(L"openvpn.com", domains_3, &size, sizeof(domains_3));
-    assert_memory_equal(domains_3, domains_3_res, size);
-    assert_int_equal(size, sizeof(domains_3_res));
-    assert_int_equal(err, NO_ERROR);
+    check_convert(L"openvpn.com", L"openvpn.com,openvpn.net", cap, NO_ERROR, L".openvpn.net\0");
 
     /* Remove no domain from a two-entry list */
-    wchar_t domains_4[BUF_SIZE] = L"openvpn.com,openvpn.net";
-    len = (DWORD)wcslen(domains_4) + 1;
-    size = len * glyph_size;
-    wchar_t domains_4_res[] = L".openvpn.com\0.openvpn.net\0";
-    err = ConvertItfDnsDomains(NULL, domains_4, &size, sizeof(domains_4));
-    assert_memory_equal(domains_4, domains_4_res, size);
-    assert_int_equal(size, sizeof(domains_4_res));
-    assert_int_equal(err, NO_ERROR);
+    check_convert(NULL, L"openvpn.com,openvpn.net", cap, NO_ERROR, L".openvpn.com\0.openvpn.net\0");
 
     /* Remove the first domain from a three-entry list */
-    wchar_t domains_5[BUF_SIZE] = L"openvpn.com,openvpn.net,openvpn.org";
-    len = (DWORD)wcslen(domains_5) + 1;
-    size = len * glyph_size;
-    wchar_t domains_5_res[] = L".openvpn.net\0.openvpn.org\0";
-    err = ConvertItfDnsDomains(L"openvpn.com", domains_5, &size, sizeof(domains_5));
-    assert_memory_equal(domains_5, domains_5_res, size);
-    assert_int_equal(size, sizeof(domains_5_res));
-    assert_int_equal(err, NO_ERROR);
+    check_convert(L"openvpn.com", L"openvpn.com,openvpn.net,openvpn.org", cap, NO_ERROR,
+                  L".openvpn.net\0.openvpn.org\0");
 
     /* Remove the middle domain from a three-entry list */
-    wchar_t domains_6[BUF_SIZE] = L"openvpn.com,openvpn.net,openvpn.org";
-    len = (DWORD)wcslen(domains_6) + 1;
-    size = len * glyph_size;
-    wchar_t domains_6_res[] = L".openvpn.com\0.openvpn.org\0";
-    err = ConvertItfDnsDomains(L"openvpn.net", domains_6, &size, sizeof(domains_6));
-    assert_memory_equal(domains_6, domains_6_res, size);
-    assert_int_equal(size, sizeof(domains_6_res));
-    assert_int_equal(err, NO_ERROR);
+    check_convert(L"openvpn.net", L"openvpn.com,openvpn.net,openvpn.org", cap, NO_ERROR,
+                  L".openvpn.com\0.openvpn.org\0");
 
     /* Remove the last domain from a three-entry list */
-    wchar_t domains_7[BUF_SIZE] = L"openvpn.com,openvpn.net,openvpn.org";
-    len = (DWORD)wcslen(domains_7) + 1;
-    size = len * glyph_size;
-    wchar_t domains_7_res[] = L".openvpn.com\0.openvpn.net\0";
-    err = ConvertItfDnsDomains(L"openvpn.org", domains_7, &size, sizeof(domains_7));
-    assert_memory_equal(domains_7, domains_7_res, size);
-    assert_int_equal(size, sizeof(domains_7_res));
-    assert_int_equal(err, NO_ERROR);
+    check_convert(L"openvpn.org", L"openvpn.com,openvpn.net,openvpn.org", cap, NO_ERROR,
+                  L".openvpn.com\0.openvpn.net\0");
 
-    /* Remove the last domain from a four-entry list because of size constraints*/
-    wchar_t domains_8[BUF_SIZE] = L"openvpn.com,openvpn.net,openvpn.org,am-ende-noch-eine-lange.de";
-    len = (DWORD)wcslen(domains_8) + 1;
-    size = len * glyph_size;
-    wchar_t domains_8_res[] = L".openvpn.com\0.openvpn.net\0.openvpn.org\0";
-    err = ConvertItfDnsDomains(NULL, domains_8, &size, sizeof(domains_8));
-    assert_memory_equal(domains_8, domains_8_res, size);
-    assert_int_equal(size, sizeof(domains_8_res));
-    assert_int_equal(err, ERROR_MORE_DATA);
+    /* Empty entries are skipped */
+    check_convert(NULL, L",a.com", cap, NO_ERROR, L".a.com\0");
+    check_convert(NULL, L"a.com,", cap, NO_ERROR, L".a.com\0");
+    check_convert(NULL, L"a.com,,b.com", cap, NO_ERROR, L".a.com\0.b.com\0");
+    check_convert(L"a.com", L",a.com,", cap, ERROR_FILE_NOT_FOUND, NULL);
+    check_convert(NULL, L",,,", cap, ERROR_FILE_NOT_FOUND, NULL);
+    check_convert(NULL, L"", cap, ERROR_FILE_NOT_FOUND, NULL);
+
+    /* Size is 0 if all domains are removed */
+    check_convert(L"a.com,b.com", L"b.com,a.com", cap, ERROR_FILE_NOT_FOUND, NULL);
+
+    /* Output exactly fills the buffer: 2 * (dot + 5 + NUL) + NUL */
+    check_convert(NULL, L"a.com,b.com", 15 * glyph_size, NO_ERROR, L".a.com\0.b.com\0");
+
+    /* Remove the last domain from a four-entry list because of size constraints,
+     * all four would need 3 * 13 + 28 + 1 = 68 glyphs, the buffer is one glyph short */
+    check_convert(NULL, L"openvpn.com,openvpn.net,openvpn.org,am-ende-noch-eine-lange.de",
+                  67 * glyph_size, ERROR_MORE_DATA, L".openvpn.com\0.openvpn.net\0.openvpn.org\0");
+
+    /* Input fills the buffer, no space for dot and terminators */
+    check_convert(NULL, L"a.com", 6 * glyph_size, ERROR_MORE_DATA, NULL);
 }
 
 static void