[S] Change in openvpn[master]: dhcp: Fix off-by-one in write_dhcp_search_str() temp buffer guard

Attention is currently required from: cron2.

Hello plaisthos, razvanc, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1884?usp=email

to look at the new patch set (#3).

The following approvals got outdated and were removed:
Code-Review+2 by cron2, Code-Review+2 by razvanc


Change subject: dhcp: Fix off-by-one in write_dhcp_search_str() temp buffer guard
......................................................................

dhcp: Fix off-by-one in write_dhcp_search_str() temp buffer guard

Each search list entry consumes strlen(ptr) + 2 bytes of tmp_buf: one
leading label length byte, the domain characters, and one trailing NUL.
The guard only accounted for strlen(ptr) + 1, so a sequence of entries
whose accumulated length lands exactly on the boundary passed the check
and then wrote tmp_buf[256], one byte past the 256 byte array.

The existing "len > 255" check enforces the correct upper bound, but it
runs after that write has already happened.

The entries can be pushed by the server: --dhcp-option falls under
OPT_P_DHCPDNS, which pull_permission_mask() includes, and
validate_domain() imposes no length limit.

Reproduced under AddressSanitizer, which reports a one byte
stack-buffer-overflow at dhcp.c:308. The added unit test covers the
boundary; it fails before this change and passes after it. The two
existing cases marked "maximum length" are unaffected, since a 253
character domain still satisfies 253 + 0 + 2 <= 256.

This was reported independently by Andre Kropp and Chính Nguyễn Văn.
Patch author is Andre Kropp, recording both reports in the Reported-By:

CVE: 2026-81738
Change-Id: I6a886a1cac2d4725859dab2325cd362312ddc659
Signed-off-by: Nexory <[email protected]>
Acked-by: Gert Doering <[email protected]>
Acked-by: Razvan Cojocaru <[email protected]>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1884
Reported-By: Andre Kropp (Nexory)
Reported-By: ChinhNguyen
Message-Id: <[email protected]>
Signed-off-by: Gert Doering <[email protected]>
---
M src/openvpn/dhcp.c
M tests/unit_tests/openvpn/test_dhcp.c
2 files changed, 21 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/84/1884/3

diff --git a/src/openvpn/dhcp.c b/src/openvpn/dhcp.c
index a54ab3f..5cdcfcf 100644
--- a/src/openvpn/dhcp.c
+++ b/src/openvpn/dhcp.c
@@ -277,7 +277,9 @@
     {
         const char *ptr = str_array[i];
 
-        if (strlen(ptr) + len + 1 > sizeof(tmp_buf))
+        /* Each entry consumes strlen(ptr) + 2 bytes: one leading label length
+         * byte and one trailing NUL. */
+        if (strlen(ptr) + len + 2 > sizeof(tmp_buf))
         {
             *error = true;
             msg(M_WARN, "write_dhcp_search_str: temp buffer overflow building DHCP options");
diff --git a/tests/unit_tests/openvpn/test_dhcp.c b/tests/unit_tests/openvpn/test_dhcp.c
index 104fc9a..3a84e1e 100644
--- a/tests/unit_tests/openvpn/test_dhcp.c
+++ b/tests/unit_tests/openvpn/test_dhcp.c
@@ -120,6 +120,24 @@
     assert_memory_equal(BPTR(&out_buf), output_5, sizeof(output_5));
     assert_false(error);
 
+    /* Several entries whose accumulated length lands exactly on the guard
+     * boundary. Each entry consumes strlen()+2 bytes of tmp_buf (one length
+     * prefix plus one trailing NUL), but the guard only accounts for
+     * strlen()+1, so the last entry writes one byte past tmp_buf[256].
+     * Sizes: 4 x 50 leaves len == 208, the final 47 makes
+     * 47 + 208 + 1 == 256, which the guard still accepts. */
+#define D50 "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+#define D47 "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+    const char *overflow_list[] = { D50, D50, D50, D50, D47 };
+    assert_int_equal(strlen(D50), 50);
+    assert_int_equal(strlen(D47), 47);
+    buf_clear(&out_buf);
+    write_dhcp_search_str(&out_buf, DHCP_DOMAIN_SEARCH, overflow_list, 5, &error);
+    /* total is 257 > 255, so the option must be rejected -- the point of this
+     * case is that tmp_buf must not be written out of bounds on the way. */
+    assert_true(error);
+    error = false;
+
     gc_free(&gc);
 }
 

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1884?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I6a886a1cac2d4725859dab2325cd362312ddc659
Gerrit-Change-Number: 1884
Gerrit-PatchSet: 3
Gerrit-Owner: cron2 <[email protected]>
Gerrit-Reviewer: cron2 <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-Reviewer: razvanc <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: cron2 <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel