Deny audio recording for all snap applications

Bug #1583057 reported by Simon Fels on 2016-05-18
18
This bug affects 3 people
Affects Status Importance Assigned to Milestone
pulseaudio (Ubuntu)
High
Luke Yelavich
Xenial
High
Simon Fels
Yakkety
High
Luke Yelavich

Bug Description

[Impact]
Currently snaps on Ubuntu Classic may declare in their snap.yaml that they want access to pulseaudio. When installed, snapd will auto-connect the pulseaudio interface giving the snap access to the pulseaudio server for playback and recording. Because recording is allowed, snaps are allowed to eavesdrop on users without the user knowing. Phase 1 of the pulseaudio interface should block recording for snaps while the details of phase 2 (which combines pulseaudio/snappy interfaces and trust-store) are worked out.

[Test Case]
First, install pulseaudio then reboot (alternatively can 'killall pulseaudio' from within your session or logout then killall pulseaudio from a vt and then log back in). pulseaudio needs to be restarted for the changes to be in effect and a reboot is the easiest way to achieve that.

1. unconfined can play audio
2. unconfined can record audio
3. non-snap confined can play audio
4. non-snap confined can record audio
5. snap confined can play audio
6. snap confined cannot record audio
7. snap confined devmode can record audio
8. indicator-sound and 'Sound Settings... works'
9. click can record audio if trust-store allows (eg, 'SnapRecorder' from the store)
10. click can play audio (eg, playback of recording from 'SnapRecorder' from the store)

Currently '6' is not implemented and all snaps may record audio. When this bug is fixed, no snaps should be able to record audio (until phase 2 is implemented which will be in a different bug).

The attached script tests 1-7. 9 and 10 require testing on a device and using

[Regression Potential]
The patch is quite small and easy to understand and is implemented to only affect processes that want to record and are running with a security label that starts with 'snap.' Unconfined processes and process running under other security labels should not be affected.

Original description:
Until we have a proper trust-store implementation with snappy and on the desktop/ubuntu core we want pulseaudio to simply deny any audio recording request coming from an app shipped as part of a snap.

The implementation adds a module-snappy-policy module to pulseaudio which adds a hook for audio recording requests and checks on connection if the apparmor security label of the connecting peer starts with "snap." which will identify it as a snap application.

Pulseaudio with the patch is available as part of the landing request at https://requests.ci-train.ubuntu.com/#/ticket/1428

Sebastien Bacher (seb128) wrote :

Hey Luke, could you work with Simon to help landing that to yakkety and probably SRU to xenial (needed for snappy)?

Changed in pulseaudio (Ubuntu):
assignee: nobody → Luke Yelavich (themuso)
importance: Undecided → High
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package pulseaudio - 1:8.0-2ubuntu2

---------------
pulseaudio (1:8.0-2ubuntu2) yakkety; urgency=medium

  [ Simon Fels ]
  * debian/patches/0700-modules-add-snappy-policy-module.patch:
    - Add initial support for a snappy specific policy manager
      which will deny all audio recording from snaps for now
      until real integration with the trust-store is available. (LP: #1583057)
  * debian/rules:
    - Build with snappy support
  * debian/pulseaudio.install:
    - Include new snappy policy module

 -- Luke Yelavich <email address hidden> Wed, 01 Jun 2016 12:04:14 +1000

Changed in pulseaudio (Ubuntu):
status: New → Fix Released
Simon Fels (morphis) wrote :

@Luke: How did you test the change before landing it?

I was under the impression that this had been tested given its waiting to be landed in overlays etc.

If it has not beentested, I am happy to back it out, since I am not in a position to test this right now.

Jamie Strandboge (jdstrand) wrote :

Thanks for working on this! Per the snappy team, this will also need a SRU for xenial.

tags: added: snapd-interface
Jamie Strandboge (jdstrand) wrote :

Adding xenial task and marking triaged since a fix is available in yakkety. Who will be providing this update?

Changed in pulseaudio (Ubuntu Xenial):
status: New → Triaged
Jamie Strandboge (jdstrand) wrote :

Ping, who will be providing this update to xenial?

Simon Fels (morphis) wrote :

@Jamie: Attached is a debdiff to update the pulse pacakge with snappy support.

Zygmunt Krynicki (zyga) wrote :

Can we please try to upstream this patch? This will help with making other distributions share the security features and advantages of snaps.

Jamie Strandboge (jdstrand) wrote :

@Simon, thanks, I'll work on sponsoring this.

@Zygmunt, I'm not sure this is the patch to upstream-- it is the phase 1 approach and the phase 2 approach is pulseaudion/trust-store/snappy interfaces which we will be discussing this week.

Changed in pulseaudio (Ubuntu Xenial):
status: Triaged → In Progress
Jamie Strandboge (jdstrand) wrote :

@Simon, couple of small things:
* you should use 8.0-0ubuntu3.1 as the version instead of 8.0-0ubuntu4
* the changelog has a date of 'Tue, 17 May 2016 17:59:58 +0200' which is quite old, yet the diff was only recently uploaded. You can use 'dch -r' to update the date

More importantly:
 * the patch introduces compiler warnings. These should be cleaned up:
+modules/module-snappy-policy.c: In function ‘connect_record_hook’:
+modules/module-snappy-policy.c:57:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
+ char *label = NULL;
+ ^
+modules/module-snappy-policy.c:65:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
+ pa_hook_result_t decision = PA_HOOK_OK;
+ ^

* configure shows:
+ Enable Snappy support: no
+ Enable Apparmor: yes

Can you comment on why snappy support was added to configure in this patch if you aren't going to use it? It seems that snappy support is compiled though, cause I see '-DHAVE_SNAPPY=1' in the build logs and see usr/lib/pulse-8.0/modules/module-snappy-policy.so is now shipped.

Changed in pulseaudio (Ubuntu Xenial):
importance: Undecided → High
assignee: nobody → Simon Fels (morphis)
Jamie Strandboge (jdstrand) wrote :

The functionality does not work as expected and I am able to record when running parecord under an apparmor profile that starts with 'snap.' (see attached).

Jamie Strandboge (jdstrand) wrote :

I should mention that when testing this installed test packages then logged out of my session, killed my user's pulseaudio then logged back in. I suppose I could have also done 'killall pulseaudio' and have it restart automatically instead.

Jamie Strandboge (jdstrand) wrote :
description: updated
Jamie Strandboge (jdstrand) wrote :

@Simon, finally, in reading the patch this will affect both strict and devmode and so the patch should "if startswith 'snap.' and process is in enforce mode ; then block recording".

This will be needed for the phase 2 implementation as well, so it is not wasted effort. I've asked the apparmor devs to comment in the trello card if there is an apparmor API for this. Be cognizant of TOUCTOU here but also understand that a snap is not able to change its enforcement mode so a TOCTOU is not security relevant for this particular change.

Jamie Strandboge (jdstrand) wrote :

@Simon, per the SRU process, I've done the paperwork to pursue the SRU but leaving this as 'In Progress' due to my comments. Please attach an updated debdiff and I'll review and adjust the bug as appropriate.

description: updated
Simon Fels (morphis) wrote :

@Jamie: Works fine here for me. Using a simple snap

name: pulseaudio-clients
version: 8.0-1
summary: Clients for PulseAudio
description: |
 Contains PulseAudio client utilities

apps:
  pactl:
    command: usr/bin/pactl
    plugs: [pulseaudio]
  paplay:
    command: usr/bin/paplay
    plugs: [pulseaudio]
  parec:
    command: usr/bin/parec
    plugs: [pulseaudio]

parts:
  packages:
    plugin: nil
    stage-packages:
      - pulseaudio

to test.

Make sure that the module is loaded:

simon@nirvana ~/Work/ubuntu/snappy/paplay-snap $ pactl list modules | grep snappy
 Name: module-snappy-policy

If not you can load it with

$ pactl load-module module-snappy-policy

If you now install the snap from above and run

$ pulseaudio-clients.parec

you will see the client hangs. If you unload the module

$ pactl unload-module module-snappy-policy

you will see the client can record now and doesn't hang any longer.

The only thing which might have went wrong with the package debdiff is that the module for the snappy policy wasn't added to /etc/pulse/daemon.pa. Will check that now.

Simon Fels (morphis) wrote :

Updated the patch. We now add snappy-policy always to default.pa. I couldn't figure out what I have to change to make the ifelse statement work properly to add the snappy policy module only conditionally to default.pa

Also fixed the compiler warnings and other small things.

Jamie Strandboge (jdstrand) wrote :

@Simon, thanks for the updates. It looks I did not have the module-snappy-policy module loaded and appreciate the update to default.pa and the updated patch that addresses the other issues.

The only remaining issue is making sure that recording continues to work in devmode. I think you will want to use aa_getcon() for this. Please discuss with Tyler before implementing however.

Changed in pulseaudio (Ubuntu Xenial):
status: In Progress → Won't Fix
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Bug attachments