From owner-freebsd-bugs@FreeBSD.ORG Thu Aug 8 23:00:58 2013 Return-Path: Delivered-To: freebsd-bugs@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 5AE8197D; Thu, 8 Aug 2013 23:00:58 +0000 (UTC) (envelope-from yaneurabeya@gmail.com) Received: from mail-pb0-x22f.google.com (mail-pb0-x22f.google.com [IPv6:2607:f8b0:400e:c01::22f]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 2B67D29A0; Thu, 8 Aug 2013 23:00:58 +0000 (UTC) Received: by mail-pb0-f47.google.com with SMTP id rr4so3943354pbb.20 for ; Thu, 08 Aug 2013 16:00:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:mime-version:content-type:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=xlEXwzBzTxnNA2qh/ZlvR94STy9rLps6wzpkWCQPJNI=; b=m9DABxIjXLX9rddyqWO14q89E//A0qI9aZUINdx4lVwolokZWUNm7oRunQo0l47kHP 3w5eUGFdzzPfbB8tboxjQ7Z5dFk/MSnxRx8d+42r5+0Ztx0kysziSN07AeE6iBY1EiZH MszAGm3da0R/69+yTPbCCxx8McXYB1DZgCZl0Dp1wx6rNoCYlueVEWYAuZsCdoiQspRO efGacMZXGcVslP8SG1/tD9dwno+cc/1kfuedpl0iG0t0XnHUBhW3+bXho1A8y1+R1lHv yBqU2qxK8VGnGNMSqgiDPUokRNbOWqDXYMzmwK0yiGkdM4u3nNCzAM3L6MfX7T0uy31U OQzg== X-Received: by 10.66.193.98 with SMTP id hn2mr8224493pac.173.1376002857206; Thu, 08 Aug 2013 16:00:57 -0700 (PDT) Received: from [192.168.242.58] (c-67-182-131-225.hsd1.wa.comcast.net. [67.182.131.225]) by mx.google.com with ESMTPSA id kc8sm16449768pbc.18.2013.08.08.16.00.55 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 08 Aug 2013 16:00:56 -0700 (PDT) Subject: Re: kern/181127: [PATCH] set{domain, host}name doesn't permit NUL terminated strings that are MAXHOSTNAMELEN long Mime-Version: 1.0 (Apple Message framework v1283) Content-Type: text/plain; charset=us-ascii From: Garrett Cooper In-Reply-To: <20130808193834.Q2900@besplex.bde.org> Date: Thu, 8 Aug 2013 16:00:53 -0700 Content-Transfer-Encoding: quoted-printable Message-Id: <6FF893BC-1ACD-486F-B314-F57FDE3FADB9@gmail.com> References: <201308080211.r782BnsS000117@oldred.freebsd.org> <20130808193834.Q2900@besplex.bde.org> To: Bruce Evans X-Mailer: Apple Mail (2.1283) Cc: freebsd-bugs@freebsd.org, freebsd-gnats-submit@freebsd.org X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Aug 2013 23:00:58 -0000 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...=