Date: Fri, 01 May 2026 15:08:19 +0000 From: Mark Johnston <markj@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Cc: Dag-Erling=?utf-8?Q? Sm=C3=B8rg?=rav <des@FreeBSD.org> Subject: git: b362b6b6c8f2 - releng/13.5 - dhclient: Improve server and filename validation Message-ID: <69f4c1e3.190a1.6c8060e7@gitrepo.freebsd.org>
index | next in thread | raw e-mail
The branch releng/13.5 has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=b362b6b6c8f28c1ff545bf7c6cdd09388879d1d8 commit b362b6b6c8f28c1ff545bf7c6cdd09388879d1d8 Author: Dag-Erling Smørgrav <des@FreeBSD.org> AuthorDate: 2026-04-30 16:45:35 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2026-04-30 21:20:57 +0000 dhclient: Improve server and filename validation * Don't iterate over each string three times; once is enough. * Reject control characters (anything below space) in addition to the double quote and backslash. * If an unsafe character is encountered, discard the string instead of rejecting the entire lease. * If backslashes are encountered in the file name option, convert them to forward slashes instead of rejecting the option. * Tweak the warning messages a bit. Looking through the rest of the code, it seems to me that notes generally end with a period while warnings generally don't. Approved by: so Security: FreeBSD-EN-26:11.dhclient Fixes: 8008e4b88daf ("dhclient: Check for unexpected characters in some DHCP server options") PR: 294886 MFC after: 1 week Reviewed by: brooks, markj Differential Revision: https://reviews.freebsd.org/D56740 (cherry picked from commit 873a195ba63575e46686cfd6ea9670a0ca340fa0) (cherry picked from commit b1ece85741db0db60ce68daca564f03dc711fe30) --- sbin/dhclient/dhclient.c | 75 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 54 insertions(+), 21 deletions(-) diff --git a/sbin/dhclient/dhclient.c b/sbin/dhclient/dhclient.c index e8fcdb133dc4..afb37a79d9fc 100644 --- a/sbin/dhclient/dhclient.c +++ b/sbin/dhclient/dhclient.c @@ -1167,7 +1167,7 @@ packet_to_lease(struct packet *packet) lease = malloc(sizeof(struct client_lease)); if (!lease) { - warning("dhcpoffer: no memory to record lease."); + warning("dhcpoffer: no memory to record lease"); return (NULL); } @@ -1208,7 +1208,7 @@ packet_to_lease(struct packet *packet) /* If the server name was filled out, copy it. Do not attempt to validate the server name as a host name. - RFC 2131 merely states that sname is NUL-terminated (which do + RFC 2131 merely states that sname is NUL-terminated (which we do not assume) and that it is the server's host name. Since the ISC client and server allow arbitrary characters, we do as well. */ @@ -1216,39 +1216,72 @@ packet_to_lease(struct packet *packet) !(packet->options[DHO_DHCP_OPTION_OVERLOAD].data[0] & 2)) && packet->raw->sname[0]) { lease->server_name = malloc(DHCP_SNAME_LEN + 1); - if (!lease->server_name) { - warning("dhcpoffer: no memory for server name."); + if (lease->server_name == NULL) { + warning("dhcpoffer: no memory for server name"); free_client_lease(lease); return (NULL); } - memcpy(lease->server_name, packet->raw->sname, DHCP_SNAME_LEN); - lease->server_name[DHCP_SNAME_LEN]='\0'; - if (strchr(lease->server_name, '"') != NULL || - strchr(lease->server_name, '\\') != NULL) { - warning("dhcpoffer: server name contains invalid characters."); - free_client_lease(lease); - return (NULL); + for (i = 0; i < DHCP_SNAME_LEN; i++) { + if (packet->raw->sname[i] == '\0') { + break; + } + if (packet->raw->sname[i] < ' ' || + packet->raw->sname[i] == '"' || + packet->raw->sname[i] == '\\') { + warning("dhcpoffer: server name contains " + "unsafe characters"); + free(lease->server_name); + lease->server_name = NULL; + break; + } + lease->server_name[i] = packet->raw->sname[i]; + } + /* Terminate and zero-pad */ + if (lease->server_name != NULL) { + while (i < DHCP_SNAME_LEN + 1) { + lease->server_name[i++] = '\0'; + } } } - /* Ditto for the filename. */ + /* Ditto for the file name. */ if ((!packet->options[DHO_DHCP_OPTION_OVERLOAD].len || !(packet->options[DHO_DHCP_OPTION_OVERLOAD].data[0] & 1)) && packet->raw->file[0]) { /* Don't count on the NUL terminator. */ lease->filename = malloc(DHCP_FILE_LEN + 1); - if (!lease->filename) { - warning("dhcpoffer: no memory for filename."); + if (lease->filename == NULL) { + warning("dhcpoffer: no memory for file name"); free_client_lease(lease); return (NULL); } - memcpy(lease->filename, packet->raw->file, DHCP_FILE_LEN); - lease->filename[DHCP_FILE_LEN]='\0'; - if (strchr(lease->filename, '"') != NULL || - strchr(lease->filename, '\\') != NULL) { - warning("dhcpoffer: filename contains invalid characters."); - free_client_lease(lease); - return (NULL); + for (i = 0; i < DHCP_FILE_LEN; i++) { + if (packet->raw->file[i] == '\0') { + break; + } + if (packet->raw->file[i] < ' ' || + packet->raw->file[i] == '"') { + warning("dhcpoffer: file name contains " + "unsafe characters"); + free(lease->filename); + lease->filename = NULL; + break; + } + if (packet->raw->file[i] == '\\') { + /* + * This is common in Windows-centric + * environments. Instead of rejecting, + * silently convert to forward slash. + */ + packet->raw->file[i] = '/'; + } + lease->filename[i] = packet->raw->file[i]; + } + /* Terminate and zero-pad */ + if (lease->filename != NULL) { + while (i < DHCP_FILE_LEN + 1) { + lease->filename[i++] = '\0'; + } } } return lease;home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?69f4c1e3.190a1.6c8060e7>