From: Nexory <[email protected]> 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]> 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 <[email protected]> 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); } _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
