From owner-freebsd-emulation@FreeBSD.ORG Sat Mar 12 06:54:40 2011 Return-Path: Delivered-To: freebsd-emulation@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 62F5B106566B for ; Sat, 12 Mar 2011 06:54:40 +0000 (UTC) (envelope-from john@feith.com) Received: from feith1.FEITH.COM (feith1.FEITH.COM [192.251.93.1]) by mx1.freebsd.org (Postfix) with ESMTP id 348628FC0A for ; Sat, 12 Mar 2011 06:54:39 +0000 (UTC) Received: from jwlab.FEITH.COM (jwlab.FEITH.COM [192.251.93.16]) by feith1.FEITH.COM (8.14.4+Sun/8.12.9) with ESMTP id p2C6fbGr025424; Sat, 12 Mar 2011 01:41:38 -0500 (EST) (envelope-from john@jwlab.FEITH.COM) Received: from jwlab.FEITH.COM (localhost [127.0.0.1]) by jwlab.FEITH.COM (8.13.8+Sun/8.13.8) with ESMTP id p2C7lobw011446; Sat, 12 Mar 2011 02:47:50 -0500 (EST) Received: (from john@localhost) by jwlab.FEITH.COM (8.13.8+Sun/8.13.8/Submit) id p2C7lnRO011445; Sat, 12 Mar 2011 02:47:49 -0500 (EST) Date: Sat, 12 Mar 2011 02:47:49 -0500 (EST) From: John Wehle Message-Id: <201103120747.p2C7lnRO011445@jwlab.FEITH.COM> To: alexander@leidinger.net MIME-Version: 1.0 Content-Type: text/plain X-DCC--Metrics: feith1; whitelist X-Scanned-By: MIMEDefang 2.67 on 192.251.93.1 X-Archived: cashew Cc: freebsd-emulation@FreeBSD.org Subject: Re: kern/149168: [linux] [patch] Linux sendmsg / recvmsg / etc fixes for pulseaudio X-BeenThere: freebsd-emulation@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Development of Emulators of other operating systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 12 Mar 2011 06:54:40 -0000 >> +struct l_user_cap_data { >> + l_int effective; >> + l_int permitted; >> + l_int inheritable; >> +}; > > You zero the data part in capget. Does this mean that the programs > gets told that we are not able to do the right thing, or gets the > program tricked into thinking we are doing the right thing (= security > hole)? I did say in the patch that the cap routines were NO-OP stubs. A full implementation is probably a fair amount of work (and not something I'm looking to do at this time). Note that these stubs are only necessary to run the Linux pulseaudio server ... they're not necessary for the pulseaudio client library so perhaps the best course of action is to skip them for the time being. Some example Linux capabilities are: CAP_MKNOD Create special files using mknod(2). CAP_NET_RAW Use RAW and PACKET sockets. CAP_SYS_BOOT Use reboot(2) and kexec_load(2). so by having capget return zero for the bits we're saying to the caller that the program does not have the necessary permissions to perform these tasks. The truth of course is the program may be able to do these things if it tries (depending on the results of the FreeBSD security checks) since currently Linux capabilities are not checked / enforced by the FreeBSD kernel. > Please add comments to document what they are supposed to do if set. Happy to add some comments to the code if you feel it's useful to continue with capget, capset, prctl PR_GET_KEEPCAPS, and prctl PR_SET_KEEPCAPS. If you feel that at this time you'd prefer to skip them due to potential security concerns, then that's fine too. >> + if (luch.version != _LINUX_CAPABILITY_VERSION) { >> + luch.version = _LINUX_CAPABILITY_VERSION; >> + error = copyout(&luch, args->hdrp, sizeof(luch)); > > Does linux do the same? I'm a little bit surprised that the kernel > does not answer in the same format as the userland requests The man page at: http://www.kernel.org/doc/man-pages/online/pages/man2/capget.2.html says: The calls will fail with the error EINVAL, and set the version field of hdrp to the kernel preferred value of _LINUX_CAPABILITY_VERSION_? when an unsupported version value is specified. In this way, one can probe what the current preferred capability revision is. so yes, I believe the implementation in the patch is correct in this regard. >> + if (lucd.effective || lucd.permitted || lucd.inheritable) >> + return (EPERM); > > How does the userland interprete EPERM? I'm not sure I understand your question. The manual page says: EPERM An attempt was made to add a capability to the Permitted set, or to set a capability in the Effective or Inheritable sets that is not in the Permitted set. Since the current implementation has minimum functionality we return EPERM if the program attempts to add or set any capabilities. >>> +184 AUE_CAPGET STD { int linux_capget(void *hdrp, void *datap); } >>> +185 AUE_CAPSET STD { int linux_capset(void *hdrp, const void *datap); } > > A quick search tells that the API is defined as cap_user_header_t and > cap_user_data_t and not "void *". It may be the case that in the end > those are defined to "void *" (I didn't check), but it would be nice > if you could use apropriate l_cap_user_header_t and l_cap_user_data_t. The first field in cap_user_header_t specifies a version number. I believe the rest of the layout of the header and data structures can depend on which version is requested. The manual page referenced above says: Not only are these system calls specific to Linux, but the kernel API is likely to change and use of these functions (in particular the format of the cap_user_*_t types) is subject to extension with each kernel revision, but old programs will keep working. Since conceptually the structures passed to the system call can vary depending on the version and it's possible for the kernel code to support more than one version, it seem appropriate to me to use void pointers which are cast as necessary in the kernel code. Granted the implementation I supplied only currently implements version 1 of the Linux API. I can add some comments to the capget / capset code mentioning the reason for the void pointers or if you feel strongly about it I can get rid of the void pointers. >> + case SCM_CREDS: >> + /* >> + * Currently LOCAL_CREDS is never in >> + * effect for Linux so no need to worry >> + * about sockcred >> + */ > > Is there a way to check this here and fail (either with an appropriate > error return or a kernel panic if this is more appropriate... I'm not > sure I have the big picture here)? I have a basic sanity check in the code which is: if (datalen != sizeof (*cmcred)) { error = EMSGSIZE; goto bad; } The problem is that unix domain sockets support two different ways of sending credentials (setsockopt LOCAL_CRED and sendmsg with SCM_CREDS). They both use the same msg type, however they each have their own unique structure. If setsockopt LOCAL_CRED is in effect for the Linux program, then the data supplied with the SCM_CREDS needs to be converted differently ... currently a nonissue since at the moment I believe the FreeBSD Linux layer doesn't support LOCAL_CRED (actually I don't know if Linux even supports LOCAL_CRED). I'm happy to take care of the style issues and add comments in some of the other places you mentioned. Let me know if there are any other questions and whether to keep the capabilities stuff as part of the patch. Once you tell me how to proceed I'll put together another patch. -- John ------------------------------------------------------------------------- | Feith Systems | Voice: 1-215-646-8000 | Email: john@feith.com | | John Wehle | Fax: 1-215-540-5495 | | -------------------------------------------------------------------------