apparmor dbus denial for org.freedesktop.Accounts and make Other vibrations work

[Oliver Grawert]

This affects vivid and (somewhat recently?) 14.09.

At some point, apps started to request access to org.freedesktop.Accounts for something, but I'm not sure what. It has been conjectured in this bug that it is due to vibration settings. Filing against ubuntu-system-settings for now, but please feel free to move to the correct package.

This happens with webapps:
Apr 7 08:42:17 ubuntu-phablet dbus[797]: apparmor="DENIED" operation="dbus_method_call" bus="system" path="/org/freedesktop/Accounts" interface="org.freedesktop.DBus.Introspectable" member="Introspect" mask="send" name="org.freedesktop.Accounts" pid=2632 profile="com.ubuntu.developer.webapps.webapp-facebook_webapp-facebook_1.0.26" peer_pid=1596 peer_profile="unconfined"
Apr 7 08:42:17 ubuntu-phablet dbus[797]: apparmor="DENIED" operation="dbus_method_call" bus="system" path="/org/freedesktop/Accounts" interface="org.freedesktop.Accounts" member="FindUserById" mask="send" name="org.freedesktop.Accounts" pid=2632 profile="com.ubuntu.developer.webapps.webapp-facebook_webapp-facebook_1.0.26" peer_pid=1596 peer_profile="unconfined"

and QML apps:
Apr 7 08:43:40 ubuntu-phablet dbus[797]: apparmor="DENIED" operation="dbus_method_call" bus="system" path="/org/freedesktop/Accounts" interface="org.freedesktop.DBus.Introspectable" member="Introspect" mask="send" name="org.freedesktop.Accounts" pid=3377 profile="com.ubuntu.calculator_calculator_1.3.339" peer_pid=1596 peer_profile="unconfined"
Apr 7 08:43:40 ubuntu-phablet dbus[797]: apparmor="DENIED" operation="dbus_method_call" bus="system" path="/org/freedesktop/Accounts" interface="org.freedesktop.Accounts" member="FindUserById" mask="send" name="org.freedesktop.Accounts" pid=3377 profile="com.ubuntu.calculator_calculator_1.3.339" peer_pid=1596 peer_profile="unconfined"

The following rules allow the requested access:
  dbus (send)
       bus=system
       path="/org/freedesktop/Accounts"
       interface="org.freedesktop.DBus.{Introspectable,Properties}"
       member=Introspect
       peer=(name=org.freedesktop.Accounts,label=unconfined),
  dbus (send)
       bus=system
       path="/org/freedesktop/Accounts"
       interface="org.freedesktop.Accounts"
       member=FindUserById
       peer=(name=org.freedesktop.Accounts,label=unconfined),
  dbus (send)
       bus=system
       path="/org/freedesktop/Accounts/User[0-9]*"
       interface="org.freedesktop.DBus.Properties"
       member=Get
       peer=(name=org.freedesktop.Accounts,label=unconfined),

However, the above is too lenient and constitutes a privacy leak for apps. FindUserById could be used by a malicious app to enumerate usernames on multiuser systems and because we can't mediate method data with apparmor, the Get() method can be used to obtain any information provided by this interface.

The following can be used to see what can be leaked to a malicious app:
gdbus introspect --system -d org.freedesktop.Accounts -o /org/freedesktop/Accounts/User`id -u phablet`

This can be solved in a couple of ways:
1. add whatever information the app is trying to access to a new helper service that only exposes things that the app needs. This could be a single standalone service, perhaps something from ubuntu-system-settings, that could expose any number of things-- the current locale, if the locale changed, if the grid units changed, the vibration settings, etc. Since this service wouldn't have any sensitive information, you could use standard dbus properties/Get()/etc
2. add a new dbus API to an existing service such that apparmor rules can then be used to allow by method (eg, GetVibration() or something)

I won't dictate the implementation except to mention that '1' seems like something generally useful and I believe that it was something the ubuntu-system-settings devs were already looking at for detecting locale changes without rebooting.

Original description
starting an app in vivid (image 135 on arale currently)

produces a bunch of dbus denials in syslog ... (there is also a /dev/tty one but i think this is just because soemthing tries to write an error to console ... so transient)

http://paste.ubuntu.com/10620834/

[Oliver Grawert (ogra)]

after a discussion on IRC it seems the vibration settings are at fault here ...

http://bazaar.launchpad.net/~system-settings-touch/gsettings-ubuntu-touch-schemas/trunk/changes?filter_file_id=com.ubuntu.touch.acc-20140113175130-tlkp5n9obvl0wg6c-1

[Jamie Strandboge (jdstrand)]

From irc (#phablet) on Wed Mar 18 2015:
08:37 < ogra> bzoltan, ^^^^ is anything in the sdk querying the account service on startup ?
08:37 < ogra> (on the phone that is)
08:37 < kenvandine> ogra, there is
08:37 < ogra> oh
08:37 < kenvandine> the other vibrate setting is stored there
08:38 < kenvandine> and the sdk uses that
08:38 < ogra> jdstrand, so i guess we need to allow that somehow
08:39 < jdstrand> I thought we had a special place for things like that
08:39 < jdstrand> and that Accounts gave away too much
08:39 < jdstrand> mdeslaur: do you recall something about that? ^ (see backscroll from 13 minutes ago)
08:40 < jdstrand> ah
08:41 < jdstrand> that should be exposed via usensord, no?
08:41 < mdeslaur> jdstrand: nope, no recollection of that
08:41 < jdstrand> kenvandine: ? ^ (usensord)
08:42 < mdeslaur> the vibrate setting is stored in user accounts?
08:42 < mdeslaur> that's is quite weird
08:43 < mdeslaur> wouldn't volume and vibrate be system-wide settings?
08:44 < ogra> until you have per-user settings
08:44 < ogra> to override the system defaults
08:44 < mdeslaur> if it's per-user, how do you handle the boot screen?
08:44 < ogra> we dont yet, seems someone was a bit to proactive :)
08:45 < mdeslaur> if it's system-wide, it doesn't belong in accounts. If it's per-user, it doesn't need to go in accounts
08:45 < ogra> once we have multiuser we will need a way to override system defaults ... i guess someone thought of this when initially implementing this bit
08:46 < mdeslaur> and giving apps access to accounts doesn't really make sense
08:47 < ogra> right, we need to find who/why it was added
08:48 < kenvandine> jdstrand, no idea
08:49 < kenvandine> all the vibrate/silent mode settings are in accounts service
08:56 < ogra> kenvandine, any idea who put them there ?
08:57 < kenvandine> jgdx, ^^ was that you?
08:57 < kenvandine> i know he did the UI for the setting
08:58 < kenvandine> all the other vibrate/volume related settings are in accounts service
08:58 < kenvandine> but perhaps this one should be user specific
08:59 < kenvandine> however, the greeter needs the setting too... not sure what's the right answer
08:59 < jgdx> kenvandine, 'them', no.
08:59 < ogra> a separate dbus service perhaps
09:00 < kenvandine> jgdx, i meant really just the other vibrate setting
09:00 < kenvandine> i'm not sure how much discussion we really had on where to store that
09:00 < jgdx> kenvandine, that was me
09:00 < kenvandine> i would have assumed accounts service as well
09:01 < jgdx> http://bazaar.launchpad.net/~system-settings-touch/gsettings-ubuntu-touch-schemas/trunk/changes?filter_file_id=com.ubuntu.touch.acc-20140113175130-tlkp5n9obvl0wg6c-1
09:02 < kenvandine> yeah, i think they make sense
09:02 < kenvandine> i guess we could debate the other vibrate

[Jamie Strandboge (jdstrand)]

Since this is causing confusion for developers and users, I am going to add an explicit deny for the denials.

[Marc Deslauriers (mdeslaur)]

Why do apps need to access the vibrate setting at all?
Since vibrating consumes power, the platform should be able to enforce whether or not an app can do it, so the API should probably be implemented in a service that is running under a different process.

[Sebastien Bacher (seb128)]

it's the uitk using the setting and generating the warnings, reassigning there. We could probably have a settings part for the eventual new dbus service discussed as approach 1) but I don't think it's in any plan to work on that at the moment

[Launchpad Janitor (janitor)]

This bug was fixed in the package apparmor-easyprof-ubuntu - 1.3.10

---------------
apparmor-easyprof-ubuntu (1.3.10) vivid; urgency=medium

  * templates/*: explicitly deny noisy access to accountsservice
    (LP: #1433590)
 -- Jamie Strandboge <email address hidden> Tue, 07 Apr 2015 11:29:08 -0500

[Charles Kerr (charlesk)]

IMO solving this at the ui-toolkit level is not correct, as that still leaves the problem for system services e.g. indicator-datetime.

seb128, looks to me like a lot of this traffic is coming from system-settings. src/accountsservice.cpp AccountsService::setUpInterface() is the source of the FindUserById() call, and the Get calls are coming from when the code pulls the properties for IncomingMessageVibrate, IncomingMessageVibrateSilentMode, OtherVibrate, DialpadSoundsEnabled, IncomingCallVibrate, IncomingCallVibrateSilentMode.

Two suggestions:

1. It's not clear to me why these are being stored in AccountsService instead of in gsettings-ubuntu-touch-schemas. The latter seems the more logical place for it and would be easy to implement; e.g. if the properties mentioned above were simple properties in com.ubuntu.touch.sound. Indeed, it looks like there's already some overlap, as there's a silent-mode property both there and on AccountsService. I'd be happy to write g-u-t-s and u-s-s patches for these if everyone's agreeable on this being moved to touch-schemas. (This would benefit indicator-datetime too :-)

2. For client applications, I agree with mdeslaur, that it would be good to have a service handle this instead of relying on client apps to monitor the setting and do the right thing; e.g. adding an intent argument to usensord's "VibratePattern" method specifying whether the vibration is due to an incoming call, a received message, or other. That still wouldn't solve the problem for system-settings, which still has to get/set the fields.

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Zsombor Egri (zsombi)]

Ok, guys, is anybody working on any service that toolkit can use?

[Sebastien Bacher (seb128)]

Yes, the desktop team started working on the service needed for gsettings isolation

[Zsombor Egri (zsombi)]

Pat, seams we are not going to have this for wk21. See below.

[15:31:55] <seb128> zsombi, hey, yes, we started working on that
[15:45:49] <zsombi> seb128: any estimate on the availability?
[15:45:58] <seb128> zsombi, not yet, weeks at least
[15:46:22] <zsombi> seb128: ouh, so it may fall out of wk21 milestone
[15:46:45] <seb128> zsombi, yeah for sure, I doubt that service is going to be easy or available before june

[Pat McGowan (pat-mcgowan)]

can we do a short term fix specific to this setting? for example a dbus api on the sound-service

[Zsombor Egri (zsombi)]

We discussed yesterday with Jamie and Christian, and the best would be to get this implemented in the usensord service. So the idea is that service would expose a property the settings application can change. Toolkit will not read any setting, but will call the Haptics as it did before. On the other hand, sensors service must check who was the caller of the vibration, to differentiate whether the call came from OSK or the app itself. It will then decide to vibrate or not, based on the setting.

So changes would need to be done on
1. usensord - expose the property system settings app can change, identify the caller and do or do not vibrate depending on the setting
2. system settings app should read/write this property
3. toolkit calls the haptics (vibrate) as it did before, without checking the enabled flag

Now, how does this sound?

[Pat McGowan (pat-mcgowan)]

great lets do it

[Zsombor Egri (zsombi)]

Ok, then I've added usensord as affected, no idea who can pick up the task on that front...

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Christian Dywan (kalikiana)]

Any news on the usensord work? More apps are hitting this.

[Pat McGowan (pat-mcgowan)]

@penk any update?

[Penk Chen (penk)]

@pat, I have exposed otherVibrate property in usensord from my local branch, and am working on the haptic logic. Should be able to finish it soon.

[Pat McGowan (pat-mcgowan)]

Pushing for 3rd time, is this still being worked?

[Zhang Enwei (zhangew401)]

Hi Pat,
I have exposed the property OtherVibrate from Object Path: com.canonical.usensord.haptic.
For remaining logic part about when to do vibration, I am contacting Zsombor. May be ready soon.
https://code.launchpad.net/~zhangew401/usensord/fix-lp-1433590

[Zhang Enwei (zhangew401)]

Thanks Zsombor explaining a lot about this bug to me.
According to comment #13, I am now blocked because in usensord, based on currently info we could get from dbus message, we don't have a method to judge if the peer(caller） is OSK or the app.
The call flow is app--->toolkit---->Qt Haptics-->uSensord
If the app or OSK don't send info for usensord to make the differentiation, I cannot achieve the goal.

[Zhang Enwei (zhangew401)]

I am now investigating if org.freedesktop.DBus.GetConnectionUnixProcessID is supported in go-dbus.
If it is supported, we can use pid to get the name of the process or path of the binary.

[Zsombor Egri (zsombi)]

On Wed, Jul 13, 2016 at 10:41 AM, Zhang Enwei <email address hidden>
wrote:

> I am now investigating if org.freedesktop.DBus.GetConnectionUnixProcessID
> is supported in go-dbus.
> If it is supported, we can use pid to get the name of the process or path
> of the binary.
>
> --
> You received this bug notification because you are a bug assignee.
> https://bugs.launchpad.net/bugs/1433590
>
> Title:
> apparmor dbus denial for org.freedesktop.Accounts and make Other
> vibrations work
>
> Status in Canonical System Image:
> Triaged
> Status in apparmor-easyprof-ubuntu package in Ubuntu:
> Fix Released
> Status in ubuntu-system-settings package in Ubuntu:
> Confirmed
> Status in ubuntu-ui-toolkit package in Ubuntu:
> Confirmed
> Status in usensord package in Ubuntu:
> Confirmed
>
> Bug description:
> This affects vivid and (somewhat recently?) 14.09.
>
> At some point, apps started to request access to
> org.freedesktop.Accounts for something, but I'm not sure what. It has
> been conjectured in this bug that it is due to vibration settings.
> Filing against ubuntu-system-settings for now, but please feel free to
> move to the correct package.
>
> This happens with webapps:
> Apr 7 08:42:17 ubuntu-phablet dbus[797]: apparmor="DENIED"
> operation="dbus_method_call" bus="system" path="/org/freedesktop/Accounts"
> interface="org.freedesktop.DBus.Introspectable" member="Introspect"
> mask="send" name="org.freedesktop.Accounts" pid=2632
> profile="com.ubuntu.developer.webapps.webapp-facebook_webapp-facebook_1.0.26"
> peer_pid=1596 peer_profile="unconfined"
> Apr 7 08:42:17 ubuntu-phablet dbus[797]: apparmor="DENIED"
> operation="dbus_method_call" bus="system" path="/org/freedesktop/Accounts"
> interface="org.freedesktop.Accounts" member="FindUserById" mask="send"
> name="org.freedesktop.Accounts" pid=2632
> profile="com.ubuntu.developer.webapps.webapp-facebook_webapp-facebook_1.0.26"
> peer_pid=1596 peer_profile="unconfined"
>
> and QML apps:
> Apr 7 08:43:40 ubuntu-phablet dbus[797]: apparmor="DENIED"
> operation="dbus_method_call" bus="system" path="/org/freedesktop/Accounts"
> interface="org.freedesktop.DBus.Introspectable" member="Introspect"
> mask="send" name="org.freedesktop.Accounts" pid=3377
> profile="com.ubuntu.calculator_calculator_1.3.339" peer_pid=1596
> peer_profile="unconfined"
> Apr 7 08:43:40 ubuntu-phablet dbus[797]: apparmor="DENIED"
> operation="dbus_method_call" bus="system" path="/org/freedesktop/Accounts"
> interface="org.freedesktop.Accounts" member="FindUserById" mask="send"
> name="org.freedesktop.Accounts" pid=3377
> profile="com.ubuntu.calculator_calculator_1.3.339" peer_pid=1596
> peer_profile="unconfined"
>
> The following rules allow the requested access:
> dbus (send)
> bus=system
> path="/org/freedesktop/Accounts"
> interface="org.freedesktop.DBus.{Introspectable,Properties}"
> member=Introspect
> peer=(name=org.freedesktop.Accounts,label=unconfined),
> dbus (send)
> bus=system
> path="/org/freedesktop/Accounts"
> interface="org.freedesktop.Accounts"
> member=FindUserById
> ...

[Zhang Enwei (zhangew401)]

I have compiled the usensord binary based on current solution.
I did some tests and it works well.
Since settings part is not ready, I am using below command to change the property whose default value is 0(means not vibrate).
dbus-send --session --print-reply --type=method_call --dest='com.canonical.usensord' /com/canonical/usensord/haptic org.freedesktop.DBus.Properties.Set string:"com.canonical.usensord.haptic" string:"OtherVibrate" uint32:1

Code is here: https://code.launchpad.net/~zhangew401/usensord/fix-lp-1433590
and the logic is
do vibration for OSK always.
do vibration for others only when the property is 1(means enabled in settings)

The property is saved in file, /home/phablet/.config/usensord/prop so that it will be restored after factory reset.

[Seth Arnold (seth-arnold)]

A pid alone does not suffice to uniquely identify a process. pid+process start time can uniquely identify a process. Without looking deeply this probably needs to be reworked.

Thanks

[Tyler Hicks (tyhicks)]

Seth is right about PIDs not being sufficient for identifying a process. Additionally, you cannot trust a /proc/PID/comm to identify what binary was used to start the process. See the proc(5) man page for details on how to trivially change your own comm to something that would trick the check added in the merge proposal.

[Zhang Enwei (zhangew401)]

Thanks Seth and Tyler.
IMHO the start time of one process may always be different value. So... this solution is not correct.

[Zhang Enwei (zhangew401)]

Proposition from James Henstridge (jamesh)
"If you want to identify the executable, calling os.Readlink() on /proc/$PID/exe would be more appropriate:

    $ ps x | grep maliit
     5823 ? Ssl 2:38 maliit-server
    25788 pts/16 S+ 0:00 grep --color=auto maliit
    $ ls -l /proc/5823/exe
    lrwxrwxrwx 1 phablet phablet 0 Jul 7 11:47 /proc/5823/exe -> /usr/bin/maliit-server

I'd combine that with the a check that the security label is "unconfined" as Tyler suggested (which you can do using the code fragment I gave via mail). That should be enough to ensure you aren't being faked out by an untrusted application, and are talking to the expected system service."

[Seth Arnold (seth-arnold)]

/proc/pid/exe is also not suitable for security use.

Thanks

[Zhang Enwei (zhangew401)]

Thanks Seth.
Do you have any suggestion what we can use in this case?

[Zhang Enwei (zhangew401)]

Hi Seth,
Since Tyler is on vacation, could you please help review?
https://code.launchpad.net/~zhangew401/usensord/fix-lp-1433590/+merge/299959
Thanks.

[Zhang Enwei (zhangew401)]

Hi Tyler and Seth,
Could you please help review current solution again? This bug is tagged ota13 so I don't have much time left. Thanks a lot.
Hi Zsombor,
Have you verified the binary I sent to you? Thank you.

[Zhang Enwei (zhangew401)]

Hi Zsombor，
Here is the latest usensord binary. Could you please help verify?
I verified unconfined apps, I cannot verify confined apps since this needs code change from Toolkit.
Thanks a lot.

[Pat McGowan (pat-mcgowan)]

@zheng could you request a landing for this or do you need some help?
@zsombor any progress

[Zhang Enwei (zhangew401)]

Hi Pat,
I think yes because I don't know how to request a landing. Thanks.

[Zhang Enwei (zhangew401)]

Hi Pat,
I have got the permission to work on Bileto. Will learn how to make a landing.
Thanks.

[Zhang Enwei (zhangew401)]

Hi Pat,
The ci-train ticket is created and package built successfully(https://requests.ci-train.ubuntu.com/#/silo/017).
@Zsombor, you could use silo17 for testing if you like. Thanks.

[Zsombor Egri (zsombi)]

@zhang wohoooooooo!!! It works!!! Thanks for that! We can finally proceed on UITK and System Settings app side then.

[Launchpad Janitor (janitor)]

This bug was fixed in the package ubuntu-system-settings - 0.4+16.10.20160824.1-0ubuntu1

---------------
ubuntu-system-settings (0.4+16.10.20160824.1-0ubuntu1) yakkety; urgency=medium

  [ Florian Boucault ]
  * Crossbuilding packaging fixes.

  [ Pat McGowan ]
  * allows uss to be built by crossbuilder (LP: #1433590, #1614637)

  [ jonas-drange ]
  * use python3 in mock click command
  * force python3 to run by refering to it in the shebang of
    test_code.py

 -- <email address hidden> (Jonas G. Drange) Wed, 24 Aug 2016 13:52:14 +0000

[Launchpad Janitor (janitor)]

This bug was fixed in the package ubuntu-ui-toolkit - 1.3.2073+16.10.20160824

---------------
ubuntu-ui-toolkit (1.3.2073+16.10.20160824) yakkety; urgency=medium

  [ Zsombor Egri ]
  * No need to check the setting anymore when vibrate. Fixes LP: #1433590.

  [ Albert Astals Cid ]
  * AbstractButton: Bind mouse area signals earlier. Otherwise it can happen that
    someone clicks on button and the binding is still not done, i.e. 22911213 .
    Fixes LP: #1610165.

  [ Christian Dywan ]
  * Fix build warnings.
  * Remove unused Q_Q(UCAbtractButton) declaration.

  [ Larry Price ]
  * Address documentation issue in UbuntuListView - pullToRefresh has "enabled"
    property. Fixes LP: #1387679

  [ Loïc Molinari ]
  * Added new C++ UbuntuMetrics library.
    This library allows the monitoring of various metrics in QtQuick 2
    applications. It allows the logging of different metrics as well as
    overlaying these metrics on top of each QtQuick windows.
  * Added LTTng support to UbuntuMetrics.
  * Added support for logging generic events. Made use of it to log predefined
    ApplicationMonitor events. Predefined events only define UserInterfaceReady
    for now, as it's needed to track start-up time.
  * Wrapped libUbuntuMetrics to QML.

  [ Florian Boucault ]
  * Icon, Switch and ProgressBar: enable use of texture atlas.
  * ActivityIndicator: do not load the image until the ActivityIndicator is
    made visible. Load it asynchronously.

  [ Tim Peeters ]
  * Remove unused CHANGES, CHANGES.syntax and NEWS files.

 -- Zoltán Balogh <email address hidden> Wed, 24 Aug 2016 11:35:29 +0000