From owner-freebsd-bugs@FreeBSD.ORG Fri Aug 9 00:40:01 2013 Return-Path: Delivered-To: freebsd-bugs@smarthost.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 56AEFC0B for ; Fri, 9 Aug 2013 00:40:01 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 34DD32EFC for ; Fri, 9 Aug 2013 00:40:01 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.7/8.14.7) with ESMTP id r790e1Y6023332 for ; Fri, 9 Aug 2013 00:40:01 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.7/8.14.7/Submit) id r790e0nv023330; Fri, 9 Aug 2013 00:40:00 GMT (envelope-from gnats) Date: Fri, 9 Aug 2013 00:40:00 GMT Message-Id: <201308090040.r790e0nv023330@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org Cc: From: Bruce Evans Subject: Re: kern/181127: [PATCH] set{domain, host}name doesn't permit NUL terminated strings that are MAXHOSTNAMELEN long X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: Bruce Evans List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Aug 2013 00:40:01 -0000 The following reply was made to PR kern/181127; it has been noted by GNATS. From: Bruce Evans To: Garrett Cooper Cc: Bruce Evans , freebsd-gnats-submit@freebsd.org, freebsd-bugs@freebsd.org Subject: Re: kern/181127: [PATCH] set{domain, host}name doesn't permit NUL terminated strings that are MAXHOSTNAMELEN long Date: Fri, 9 Aug 2013 10:39:37 +1000 (EST) On Thu, 8 Aug 2013, Garrett Cooper wrote: > On Aug 8, 2013, at 4:35 AM, Bruce Evans wrote: > >> On Thu, 8 Aug 2013, Garrett Cooper wrote: >> >>>> Synopsis: [PATCH] set{domain,host}name doesn't permit NUL terminated strings that are MAXHOSTNAMELEN long >>> ... >>>> Description: >>> The noted link/patch fixes POSIX and generic requirement compliance for set{domain,host}name per the manpages by accounting for the fact that the string >>> must be NUL terminated. >> >> The bugs seem to be mainly in the tests, so the proposed fix enlarges them. >> MAXHOSTNAMELEN is already 1 larger than the POSIX limit {HOST_NAME_MAX} >> (see the sysconf(3) sources). > > So the fix is bogus. Ok, missed that MAXHOSTNAMELEN was '\0' inclusive. > >>> Found with the NetBSD t_set{domain,host}name testcases: >>> >>> Before: >>> >>> $ pwd >>> /usr/tests/lib/libc/gen >>> $ sudo atf-run t_setdomainname | atf-report >>> t_setdomainname (1/1): 3 test cases >>> setdomainname_basic: [0.019497s] Failed: /usr/src/lib/libc/tests/gen/t_setdomainname.c:66: setdomainname(domains[i],sizeof(domains[i])) == 0 not met >>> setdomainname_limit: [0.004173s] Passed. >>> setdomainname_perm: [0.005297s] Passed. >>> [0.029872s] >> >> I'm not sure what these do, but according to the Synopsis, >> set{domain,host}name correctly doesn't permit NUL terminated strings that >> are MAXHOSTNAMELEN long (not counting space for the NUL). MAXHOSTNAMELEN >> counts space for the NUL and is 1 larger than {HOST_NAME_MAX}. > > Yes. It's kind of odd why NetBSD passes here, but this should work on FreeBSD as well as they aren't doing anything going out-of-bounds in the testcases (see https://github.com/yaneurabeya/freebsd/blob/master/lib/libc/tests/gen/t_setdomainname.c , https://github.com/yaneurabeya/freebsd/blob/master/lib/libc/tests/gen/t_sethostname.c if you're curious). It uses MAXHOSTNAMELEN + 1 in (only) 1 place, and then seems to check that setdomainname() on a null name with "length" MAXHOSTNAMELEN + 1 fails. It doesn't seem to test any strings of nearly length MAXHOSTNAMELEN. > ... > >>> @@ -314,11 +314,11 @@ sysctl_hostname(SYSCTL_HANDLER_ARGS) >>> >>> SYSCTL_PROC(_kern, KERN_HOSTNAME, hostname, >>> CTLTYPE_STRING | CTLFLAG_RW | CTLFLAG_PRISON | CTLFLAG_MPSAFE, >>> - (void *)(offsetof(struct prison, pr_hostname)), MAXHOSTNAMELEN, >>> + (void *)(offsetof(struct prison, pr_hostname)), MAXHOSTNAMELEN+1, >>> sysctl_hostname, "A", "Hostname"); >>> SYSCTL_PROC(_kern, KERN_NISDOMAINNAME, domainname, >>> CTLTYPE_STRING | CTLFLAG_RW | CTLFLAG_PRISON | CTLFLAG_MPSAFE, >>> - (void *)(offsetof(struct prison, pr_domainname)), MAXHOSTNAMELEN, >>> + (void *)(offsetof(struct prison, pr_domainname)), MAXHOSTNAMELEN+1, >>> sysctl_hostname, "A", "Name of the current YP/NIS domain"); >>> SYSCTL_PROC(_kern, KERN_HOSTUUID, hostuuid, >>> CTLTYPE_STRING | CTLFLAG_RW | CTLFLAG_PRISON | CTLFLAG_MPSAFE, >> >> The sysctls were originally simple SYSCTL_STRING()s and I think they >> worked then. Now they are quite complicated, to support jails, etc., >> but they still use sysctl_handle_string() so I think they handle >> (non)strings and (non)termination the same. Note that >> sysctl_handle_string() doesn't actually return strings unless the >> buffer is large enough to hold the NUL terminator. It just truncates. >> This is reflected in the gethostname(3) API. The name length for >> gethostname() must be 1 larger than {HOST_NAME_MAX} to ensure >> getting a string. OTOH, the name length for sethostname(3) should >> not include space for the NUL, so it must not be larger than >> {HOST_NAME_MAX}. If it is larger than {HOST_NAME_MAX}, then the >> syscall will just fail. If it is larger than the string length >> (to include the NUL and possibly more) but not larger than >> {HOST_NAME_MAX}, then the syscall will succeed and the string will >> just be terminated more than once. (It would be safer to write NULs >> from the end of the string until the end of the buffer in all cases.) > > So translation is: is there's a bug in the sysctl handler after jail support was added and there's no reasonable way to fix it without reverting things back to their sane forms? No. I suspected a bug in the jail support, but couldn't see any. You will have to check with name and string lengths of nearly MAXHOSTNAMELEN + 1 on (or whatever the kernel buffer size is for plain SYSCTL_STRING()) to see if the jail support gives any differences. Bruce