Snap policy module fails to identify snaps if SCM_CREDENTIALS are missing from PA_COMMAND_AUTH request

I should note that aa_getpeercon() is currently a wrapper around

 rc = getsockopt(fd, SOL_SOCKET, SO_PEERSEC, buf, &optlen);

it checks that apparmor is enabled before hitting the interface (other LSMs could be using it), and then splits the context that is returned into a label and mode.

Once the LSM stacking replacement interface is properly defined it will use that if available instead.

You can hit the interface directly, but if you do you should perform similar checks and processing.

I had a look through how upstream Pulse Audio uses SCM_CREDENTIALS, and I don't believe this bug extends to anything there: all uses grant privileges based on matching uid or gid, so this attack would result in less privilege.

It's a problem for us because we were reducing privilege on a match rather than increasing it.

Attached is a test snap based on the example program from bug 1886854. I've turned it into a standalone snap with strict confinement an audio-playback plug, and a launcher script that sets environment variables to let it find the Pulse Audio socket and cookie file.

The program attempts to load the "module-null-sink" plugin, and then remove it. The expected output is something like this:

    2020/09/22 15:46:21 PulseAudio connection created successfully
    2020/09/22 15:46:21 Couldn't load module, error message: PulseAudio error: commandLoadModule -> Access denied

However, on repeated runs, it will occasionally produce output like the following:

    2020/09/22 15:46:33 PulseAudio connection created successfully
    Loaded Module sucessfully at index: 27

... indicating that it has not been detected as a snap. These occasions will be paired with Pulse Audio logging "[pulseaudio] module-snap-policy.c: AppArmor profile could not be retrieved."

Snapcraft project for pa-race snap.

Here is a draft fix for the bug as a patch against 20.04's Pulse Audio. If this looks okay, it should be fairly easy to port to xenial, bionic, and groovy.

With these changes, we use aa_getpeercon() to retrieve the peer's AppArmor label at connection time and store it in the pa_client struct. The policy module then uses this label rather than the "SCM_CREDENTIALS -> process ID -> aa_gettaskcon" method it currently does.

The pa-race test snap consistently returns access denied with these changes applied.

I had a go at trying to slot in a fix for bug 1886854 (allow classic snaps to load modules), but I'm getting protocol errors when switching those hooks to async mode. So this is just a fix for the SCM_CREDENTIALS security issue.

The problems with the protocol error problems were due to me dropping too much of the 0409 patch: in addition to adding the pa_creds fields to the pa_client struct, it was also fixing a bug in the command argument parsing after continuing from an asynchronous hook invocation.

This version includes the fix for bug 1886854, to allow classic snaps to invoke module and daemon control related commands again.

This has been assigned CVE-2020-16123.

The patch in comment #6 looks good - we will just need to edit the changelog entry to list the actual assigned CVE ID from above and then backport to xenial, bionic and groovy.

As far as testing goes, here are a few things to help test the updated package:

1. After installing the test snap, running "pa-race" repeatedly should always fail. This is in comparison with the old package where it will occasionally succeed.

2. Running "/snap/pa-race/current/bin/bug" directly (i.e. the same program outside of confinement) should always succeed.

3. Running "snap run --shell some-classic-snap" and then "/snap/pa-race/current/bin/bug" from that shell should always succeed.

Some of the tests from the plan in bug 1781428 would be worth verifying too.

Hello James,

Thank you so much for the patch, test snap and testing information! There are issues applying the patch in Xenial because there are some remaining references to the creds and creds_valid variables (which are removed from the pa_client struct) in src/modules/trust-store/module-trust-store.c, which contains functionality for Ubuntu Touch. Should the changes to remove the variables be backported to that file as well?

Thank you,
Avital

This is a bit tricky due to divergence in the package after we removed Trust Store post-xenial. I _think_ this is the best way to integrate it:

1. Keep xenial's 0409 patch as is: the current version in focal and from my debdiff represents the parts of that original patch that were still needed after removing the Trust Store specific parts.

2. Add the 0410 patch from my debdiff. This is a new patch rather than a replacement for xenial's 0410, so may need renumbering. I don't think it should conflict with the other truststore patches.

3. Take the 0700 patch from my debdiff as a replacement for xenial's 0450 patch.

This bug was fixed in the package pulseaudio - 1:13.99.2-1ubuntu2.1

---------------
pulseaudio (1:13.99.2-1ubuntu2.1) groovy-security; urgency=medium

  * SECURITY UPDATE: don't rely on SCM_CREDENTIALS to detect snap confined
    clients (LP: #1895928)
    - d/p/0409-pa-client-peer-credentials.patch: drop patch
    - d/p/0409-fix-arg-parsing-after-async-hook.patch: remains of old 0409
      patch not related to pa_creds.
    - d/p/0410-pa-client-peer-apparmor-label.patch: new patch, records
      AppArmor label in pa_client struct for native connections using
      aa_getpeercon.
    - d/p/0702-add-snappy-policy-module.patch: use the AppArmor
      label in the pa_client rather than looking it up via the process ID
      from SCM_CREDENTIALS.
    - CVE-2020-16123
   * Don't block classic snaps from module loading/unloading (LP: #1886854)
    - d/p/0702-add-snappy-policy-module.patch: replace
      deny_to_snaps_hook with a version that allows classic snaps.

 -- James Henstridge <email address hidden> Thu, 05 Nov 2020 16:46:59 -0500