[Openvpn-devel,v2] dhcp: Fix off-by-one in write_dhcp_search_str() temp buffer guard
Commit Message
From: Nexory <St4yl3r30@hotmail.de>
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 <St4yl3r30@hotmail.de>
Acked-by: Gert Doering <gert@greenie.muc.de>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1884
Reported-By: Andre Kropp (Nexory)
Reported-By: ChinhNguyen
---
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/+/1884
This mail reflects revision 2 of this Change.
Acked-by according to Gerrit (reflected above):
Gert Doering <gert@greenie.muc.de>
Comments
Thanks to both patch author and independent discoverer :-) - both
credits have been recorded, and I think I even got the UTF8 bits right.
Your patch has been applied to the master, release/2.7 and release/2.6
branches.
Release/2.6 has the same code, but it's still in tun.c, and there is
no unit test. So I have backported the fix + comment only, keeping
your commit message.
commit 1ece6fe6fd586501b486aac6932d424c167a3adb (master)
commit cc2032f3e2e3a930aed657653fb7a771a9b43b1d (release/2.7)
commit cedf9bc20f1af01c544489e62da6ff2c7ca82488 (release/2.6)
Author: Nexory
Date: Mon Aug 31 10:34:40 2026 +0200
dhcp: Fix off-by-one in write_dhcp_search_str() temp buffer guard
Signed-off-by: Nexory <St4yl3r30@hotmail.de>
Acked-by: Gert Doering <gert@greenie.muc.de>
Acked-by: Razvan Cojocaru <razvanc@mailbox.org>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1884
Message-Id: <20260831083449.12484-1-gert@greenie.muc.de>
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
@@ -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");
@@ -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);
}