Date: Sat, 12 Mar 2011 02:47:49 -0500 (EST) From: John Wehle <john@feith.com> To: alexander@leidinger.net Cc: freebsd-emulation@FreeBSD.org Subject: Re: kern/149168: [linux] [patch] Linux sendmsg / recvmsg / etc fixes for pulseaudio Message-ID: <201103120747.p2C7lnRO011445@jwlab.FEITH.COM>
next in thread | raw e-mail | index | archive | help
>> +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 | |
-------------------------------------------------------------------------
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201103120747.p2C7lnRO011445>