Bug 72580 - pulseaudio's use of sys/capability.h is non-POSIX
Summary: pulseaudio's use of sys/capability.h is non-POSIX
Status: RESOLVED FIXED
Alias: None
Product: PulseAudio
Classification: Unclassified
Component: daemon (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: pulseaudio-bugs
QA Contact: pulseaudio-bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-10 21:29 UTC by Allison Lortie (desrt)
Modified: 2014-01-26 14:45 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH] Add support for FreeBSD <sys/capability.h> (2.88 KB, patch)
2013-12-10 22:09 UTC, Allison Lortie (desrt)
Details | Splinter Review
Add support for FreeBSD <sys/capability.h> (3.83 KB, patch)
2013-12-15 23:35 UTC, Allison Lortie (desrt)
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Allison Lortie (desrt) 2013-12-10 21:29:18 UTC
The configure script for pulseaudio has this:

CAP_LIBS=''

AC_ARG_WITH([caps],
    AS_HELP_STRING([--without-caps],[Omit support for POSIX capabilities.]))

if test "x${with_caps}" != "xno"; then
    AC_SEARCH_LIBS([cap_init], [cap], [], [
                    if test "x${with_caps}" = "xyes" ; then
                        AC_MSG_ERROR([*** POSIX caps libraries not found])
                    fi])
    AC_CHECK_HEADERS([sys/capability.h], [], [
                    if test "x${with_caps}" = "xyes" ; then
                        AC_MSG_ERROR([*** POSIX caps headers not found])
                    fi])
fi

Then in the daemon's source:

void pa_drop_caps(void) {
#ifdef HAVE_SYS_CAPABILITY_H
    cap_t caps;
    pa_assert_se(caps = cap_init());
    pa_assert_se(cap_clear(caps) == 0);
    pa_assert_se(cap_set_proc(caps) == 0);
    pa_assert_se(cap_free(caps) == 0);
#else
    pa_log_warn("Normally all extra capabilities would be dropped now, but "
                "that's impossible because this Pulseaudio was built without "
                "libcap support.");
#endif
}



POSIX does not specify what such a file should contain -- the attempt to standardise it seems to have failed, as evidenced by the comment at the top of Linux's version of this file:

 * defunct POSIX.1e Standard: 25.2 Capabilities

Meanwhile, the combination of the two checks above produces the wrong behaviour.

Imagine a system where sys/capability.h exists, but not cap_init (FreeBSD is such a system, for example).

The first check fails due to missing cap_init, but because --with-caps=yes was not explicitly given, the failure is ignored.

The second check, which is independent of the first check then passes, because we do find sys/capability.h.  This results in HAVE_SYS_CAPABILITY_H being defined, and then the caps code gets enabled in pa_drop_caps().

Inside the #ifdef for HAVE_SYS_CAPABILITY_H there should probably also be an #ifdef __linux before using that Linux-style capabilities code.  There could then ideally be another branch for BSD-style sys/capability.h.
Comment 1 Allison Lortie (desrt) 2013-12-10 22:09:55 UTC
Created attachment 90593 [details] [review]
[PATCH] Add support for FreeBSD <sys/capability.h>

cap_init() and friends are Linux-specific, so only use them if we're on
Linux.

Add support for FreeBSD capabilities if we find <sys/capability.h> to be
available there.

Add an #else (not Linux or FreeBSD) case with an #error requesting
contributions for other platforms.

This patch keeps the cap_init check in configure.ac but removes the
error if it fails.  This will ensure we link to -lcap if needed, but
won't fail for the case that capabilities are part of the core system
(as on FreeBSD).

We do however, modify the header check to ensure we fail if there is no
<sys/capability.h> at all, forcing --without-caps to be explicitly
specified to make the failure go away.
Comment 2 Tanu Kaskinen 2013-12-13 13:30:44 UTC
Thanks for the patch. I have an improvement request: I think it would be better to not fail hard on systems that do not provide the capability functionality, unless --with-caps has been explicitly specified.
Comment 3 Allison Lortie (desrt) 2013-12-13 17:22:32 UTC
I consider it to be an automake "best practice" to fail instead of silently disabling a feature.  The reason for that is simple: the person who downloads and pulseaudio may simply not have the libcap development headers installed.  I think it's better to give them a chance to choose to install them or to explicitly specify --without-cap.

If I make your suggested modification then the build will just go ahead without capabilities support, which would be a reduction in functionality (or in this case security) just because the user happened not to have a particular package installed.

The same logic applies to people who are making distro packages as well...  I think that it's always better to let them know.
Comment 4 Tanu Kaskinen 2013-12-14 07:47:14 UTC
We treat all other dependencies in the same way: if the user didn't enable or disable an optional feature explicitly, then it's enabled or disabled depending on whether it's available in the system or not. While I didn't come up with this scheme, I think it's a good way to deal with optional dependencies. I want ./configure without arguments to just work, as long as all mandatory dependencies are satisfied.

At the end of configure, we print a list of features that are enabled or disabled. Capability support isn't currently in that list, so that could be fixed.

Since capabilities are a security feature, it might be a good idea to make an exception in this case, and require explicit --without-caps to make sure that the user really wants to build PulseAudio without capability support, but that should only be done on platforms on which capabilities are supported. We don't support capabilities on Windows, for example, and I don't want to require all Windows users to pass --without-caps.
Comment 5 Allison Lortie (desrt) 2013-12-14 19:49:57 UTC
That's a fair point.  I'll add a check for being on Linux or FreeBSD before giving the error and upload a new patch soon.
Comment 6 Allison Lortie (desrt) 2013-12-15 23:35:42 UTC
Created attachment 90814 [details] [review]
Add support for FreeBSD <sys/capability.h>

cap_init() and friends are Linux-specific, so only use them if we're on
Linux.

Add support for FreeBSD capabilities if we find <sys/capability.h> to be
available there.

Add an #else (not Linux or FreeBSD) case with an #error requesting
contributions for other platforms.

This patch keeps the cap_init check in configure.ac but removes the
error if it fails.  This will ensure we link to -lcap if needed, but
won't fail for the case that capabilities are part of the core system
(as on FreeBSD).

We do however, modify the header check to ensure we fail if there is no
<sys/capability.h> at all and we are on a system where it could be
installed.  The logic here is that it is better to give the user the
chance to install it than it is to proceed silently with a disabled
security feature on a system where it could easily be supported.
--without-caps remains an option if the user wants to force it.
Comment 7 Tanu Kaskinen 2013-12-20 11:44:45 UTC
Thanks again for the updated patch!

configure.ac looks good now. It looks like you forgot to remove the #error from caps.c, or was leaving it there intentional?
Comment 8 Allison Lortie (desrt) 2013-12-20 16:53:08 UTC
That's intentional.  It will catch that case that there is some other platform out there that has <sys/capability.h> but we don't know how to use it.  Hopefully they will then file a bug telling us how to do so.

It will not hit platforms that don't have <sys/capability.h> (Windows, etc.)

They can always workaround the issue with --without-caps.
Comment 9 Tanu Kaskinen 2013-12-20 19:52:38 UTC
OK, that makes sense. I had some misunderstanding of the logic.

Patch applied: cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=1da34e99b203de14a45c94ec768faf04d41eca5b
Comment 10 Allison Lortie (desrt) 2013-12-21 14:54:22 UTC
Thanks very much.
Comment 11 Allison Lortie (desrt) 2014-01-03 07:29:13 UTC
So this patch is not right.

I assumed from my reading of the Linux code ("cap_clear()...") that it was clearing all capabilities of the process when in fact it is only clearing the "special to root" capabilities.

The FreeBSD version of the code indeed clears _all_ capabilities beyond ones that the process already has (ie: cannot open any new files, create sockets, etc.)

This has a pretty obvious adverse effect on pulseaudio's ability to do what it needs to do -- indeed, it bombs out pretty quickly due to an inability to read its own config file.

We need to find out if we can drop the special-to-root caps on FreeBSD without the others.  My initial research into this makes it look like it may not be possible...
Comment 13 Allison Lortie (desrt) 2014-01-03 14:32:59 UTC
This is suboptimal because now we #error out on FreeBSD...

The crux of the problem here comes back down to this check in configure.ac that assumes that sys/capability.h means anything in particular.  It turns out that the presence of this file on FreeBSD is related to the existence of a very different feature than on Linux...
Comment 14 Allison Lortie (desrt) 2014-01-25 04:23:39 UTC
So it turns out that FreeBSD simply has no mechanism similar to Linux capabilities.  The correct fix is to add back the #elif case for FreeBSD and have it do nothing, perhaps with a comment about "known not to be supported".
Comment 15 Tanu Kaskinen 2014-01-26 09:31:19 UTC
Ok, I'll make that change soon.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.