Attention is currently required from: cron2, plaisthos, razvanc.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1905?usp=email
to review the following change.
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.
(Note: The unit tests have not been included in the cherry-pick to 2.6
because test_dhcp.c did not exist in this version.)
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]>
(cherry picked from commit 1ece6fe6fd586501b486aac6932d424c167a3adb)
---
M src/openvpn/dhcp.c
1 file changed, 192 insertions(+), 0 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/05/1905/1
diff --git a/src/openvpn/dhcp.c b/src/openvpn/dhcp.c
index f137b01..0e03cec 100644
--- a/src/openvpn/dhcp.c
+++ b/src/openvpn/dhcp.c
@@ -186,3 +186,195 @@
}
return 0;
}
+
+#if defined(_WIN32) || defined(DHCP_UNIT_TEST)
+
+/*
+ * Convert DHCP options from the command line / config file
+ * into a raw DHCP-format options string.
+ */
+
+static void
+write_dhcp_u8(struct buffer *buf, const uint8_t type, const uint8_t data, bool
*error)
+{
+ if (!buf_safe(buf, 3))
+ {
+ *error = true;
+ msg(M_WARN, "write_dhcp_u8: buffer overflow building DHCP options");
+ return;
+ }
+ buf_write_u8(buf, type);
+ buf_write_u8(buf, 1);
+ buf_write_u8(buf, data);
+}
+
+static void
+write_dhcp_u32_array(struct buffer *buf, const uint8_t type, const uint32_t
*data,
+ const unsigned int len, bool *error)
+{
+ if (len > 0)
+ {
+ const size_t size = len * sizeof(uint32_t);
+
+ if (!buf_safe(buf, 2 + size))
+ {
+ *error = true;
+ msg(M_WARN, "write_dhcp_u32_array: buffer overflow building DHCP
options");
+ return;
+ }
+ if (size < 1 || size > 255)
+ {
+ *error = true;
+ msg(M_WARN, "write_dhcp_u32_array: size (%zu) must be > 0 and <=
255", size);
+ return;
+ }
+ buf_write_u8(buf, type);
+ buf_write_u8(buf, (uint8_t)size);
+ for (unsigned int i = 0; i < len; ++i)
+ {
+ buf_write_u32(buf, data[i]);
+ }
+ }
+}
+
+static void
+write_dhcp_str(struct buffer *buf, const uint8_t type, const char *str, bool
*error)
+{
+ const size_t len = strlen(str);
+ if (!buf_safe(buf, 2 + len))
+ {
+ *error = true;
+ msg(M_WARN, "write_dhcp_str: buffer overflow building DHCP options");
+ return;
+ }
+ if (len < 1 || len > 255)
+ {
+ *error = true;
+ msg(M_WARN, "write_dhcp_str: string '%s' must be > 0 bytes and <= 255
bytes", str);
+ return;
+ }
+ buf_write_u8(buf, type);
+ buf_write_u8(buf, (uint8_t)len);
+ buf_write(buf, str, len);
+}
+
+/*
+ * RFC3397 states that multiple searchdomains are encoded as follows:
+ * - at start the length of the entire option is given
+ * - each subdomain is preceded by its length
+ * - each searchdomain is separated by a NUL character
+ * e.g. if you want "openvpn.net" and "duckduckgo.com" then you end up with
+ * 0x1D 0x7 openvpn 0x3 net 0x00 0x0A duckduckgo 0x3 com 0x00
+ */
+static void
+write_dhcp_search_str(struct buffer *buf, const uint8_t type, const char
*const *str_array,
+ int array_len, bool *error)
+{
+ char tmp_buf[256];
+ size_t len = 0;
+ size_t label_length_pos;
+
+ for (int i = 0; i < array_len; i++)
+ {
+ const char *ptr = str_array[i];
+
+ /* 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");
+ return;
+ }
+ /* Loop over all subdomains separated by a dot and replace the dot
+ * with the length of the subdomain */
+
+ /* label_length_pos points to the byte to be replaced by the length
+ * of the following domain label */
+ label_length_pos = len++;
+
+ while (true)
+ {
+ if (*ptr == '.' || *ptr == '\0')
+ {
+ /* cast is protected by sizeof(tmp_buf) */
+ tmp_buf[label_length_pos] = (char)(len - label_length_pos - 1);
+ label_length_pos = len;
+ if (*ptr == '\0')
+ {
+ break;
+ }
+ }
+ tmp_buf[len++] = *ptr++;
+ }
+ /* And close off with an extra NUL char */
+ tmp_buf[len++] = 0;
+ }
+
+ if (!buf_safe(buf, 2 + len))
+ {
+ *error = true;
+ msg(M_WARN, "write_search_dhcp_str: buffer overflow building DHCP
options");
+ return;
+ }
+ if (len > 255)
+ {
+ *error = true;
+ msg(M_WARN, "write_dhcp_search_str: search domain string must be <=
255 bytes");
+ return;
+ }
+
+ buf_write_u8(buf, type);
+ buf_write_u8(buf, (uint8_t)len);
+ buf_write(buf, tmp_buf, len);
+}
+
+bool
+build_dhcp_options_string(struct buffer *buf, const struct tuntap_options *o)
+{
+ bool error = false;
+ if (o->domain)
+ {
+ write_dhcp_str(buf, DHCP_DOMAIN_NAME, o->domain, &error);
+ }
+
+ if (o->netbios_scope)
+ {
+ write_dhcp_str(buf, DHCP_NETBIOS_SCOPE, o->netbios_scope, &error);
+ }
+
+ if (o->netbios_node_type)
+ {
+ write_dhcp_u8(buf, DHCP_NETBIOS_NODE_TYPE, o->netbios_node_type,
&error);
+ }
+
+ write_dhcp_u32_array(buf, DHCP_DOMAIN_SERVER, (uint32_t *)o->dns,
o->dns_len, &error);
+ write_dhcp_u32_array(buf, DHCP_NETBIOS_DOMAIN_SERVER, (uint32_t *)o->wins,
o->wins_len, &error);
+ write_dhcp_u32_array(buf, DHCP_NTP_SERVER, (uint32_t *)o->ntp, o->ntp_len,
&error);
+ write_dhcp_u32_array(buf, DHCP_NETBIOS_DIST_SERVER, (uint32_t *)o->nbdd,
o->nbdd_len, &error);
+
+ if (o->domain_search_list_len > 0)
+ {
+ write_dhcp_search_str(buf, DHCP_DOMAIN_SEARCH, o->domain_search_list,
o->domain_search_list_len, &error);
+ }
+
+ /* the MS DHCP server option 'Disable Netbios-over-TCP/IP
+ * is implemented as vendor option 001, value 002.
+ * A value of 001 means 'leave NBT alone' which is the default */
+ if (o->disable_nbt)
+ {
+ if (!buf_safe(buf, 8))
+ {
+ msg(M_WARN, "build_dhcp_options_string: buffer overflow building
DHCP options");
+ return false;
+ }
+ buf_write_u8(buf, DHCP_VENDOR);
+ buf_write_u8(buf, 6); /* total length field */
+ buf_write_u8(buf, 0x001);
+ buf_write_u8(buf, 4); /* length of the vendor specified field */
+ buf_write_u32(buf, 0x002);
+ }
+ return !error;
+}
+
+#endif /* defined(_WIN32) */
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1905?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: release/2.6
Gerrit-Change-Id: I6a886a1cac2d4725859dab2325cd362312ddc659
Gerrit-Change-Number: 1905
Gerrit-PatchSet: 1
Gerrit-Owner: MaxF <[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: plaisthos <[email protected]>
Gerrit-Attention: cron2 <[email protected]>
Gerrit-Attention: razvanc <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
