kern/181127: [PATCH] set{domain, host}name doesn't permit NUL terminated strings that are MAXHOSTNAMELEN long

[Garrett Cooper]

The following reply was made to PR kern/181127; it has been noted by GNATS.

From: Garrett Cooper <yaneurabeya at gmail.com>
To: Bruce Evans <brde at optusnet.com.au>
Cc: freebsd-gnats-submit at freebsd.org,
 freebsd-bugs at freebsd.org
Subject: Re: kern/181127: [PATCH] set{domain, host}name doesn't permit NUL terminated strings that are MAXHOSTNAMELEN long
Date: Thu, 8 Aug 2013 16:00:53 -0700

 On Aug 8, 2013, at 4:35 AM, Bruce Evans wrote:
 
 > On Thu, 8 Aug 2013, Garrett Cooper wrote:
 >=20
 >>> 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.
 >=20
 > 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:
 >>=20
 >> Before:
 >>=20
 >> $ 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])) =3D=3D 0 not met
 >>   setdomainname_limit: [0.004173s] Passed.
 >>   setdomainname_perm: [0.005297s] Passed.
 >> [0.029872s]
 >=20
 > 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_se=
 tdomainname.c , =
 https://github.com/yaneurabeya/freebsd/blob/master/lib/libc/tests/gen/t_se=
 thostname.c if you're curious).
 
 ...
 
 >> @@ -314,11 +314,11 @@ sysctl_hostname(SYSCTL_HANDLER_ARGS)
 >>=20
 >> 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,
 >=20
 > 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?
 
 Thanks...=