Date: Sat, 12 Mar 2011 16:42:24 +0100 From: Alexander Leidinger <Alexander@Leidinger.net> To: John Wehle <john@feith.com> Cc: freebsd-emulation@FreeBSD.org Subject: Re: kern/149168: [linux] [patch] Linux sendmsg / recvmsg / etc fixes for pulseaudio Message-ID: <20110312164224.000010cd@unknown> In-Reply-To: <201103120747.p2C7lnRO011445@jwlab.FEITH.COM> References: <201103120747.p2C7lnRO011445@jwlab.FEITH.COM>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 12 Mar 2011 02:47:49 -0500 (EST) John Wehle <john@feith.com>
wrote:
> >> +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
Yes, but what does it mean? A NO-OP can be to tell "I did what you
requested" but in reality it was "I did nothing". It can also be that
the NO-OP result is an answer as in "you are not allowed to do that or
the action you requested did not succeed".
Your answer below looks like it is OK to proceed here.
> 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
This is what I needed to know. "You can not do anything" is a good
NO-OP implementation of this security related API.
> 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.
Yes, I think it is useful to commit this. It's a step forward and
permits those which "feel to scratch what's itching them in this area"
to proceed.
> 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.
I agree with you.
> >> + 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.
Good. Even better would be to print out that a program tried to set an
unsupported capability (like we do e.g. for unsupported ioctl's). If
you do not have the the spare time to handle this, I will add something
like this myself.
> >>> +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.
If you do not have the time to handle it, I am OK to commit it with the
void pointers, but I would prefer to have some real structures there
instead of 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).
OK.
> I'm happy to take care of the style issues and add comments in some of
> the other places you mentioned.
Great!
> 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.
With the capabilities please. Thanks a lot!
Bye,
Alexander.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110312164224.000010cd>