interfaces/desktop: allow access to system prompter interface

[alexmurray]

This allows snaps to use pinentry-gnome3 via way of gpg-agent to unlock
private keys etc and to still integrate nicely with the user's existing
desktop.

[alexmurray]

(force pushed with correct email address in commit)

[zyga]

The code change looks good, thank you!

@alexmurray is there a bug related to this fix? Perhaps it can be referenced by the commit message
@alexmurray are those DBus methods safe to use from essentially untrusted software (the desktop interface is not privileged)?

[alexmurray]

@zyga - this is related to #7366 but I don't think there is a specific bug about it

Re safety of methods - I see these methods as akin to the existing DBus Notification methods we already allow - it allows an application to create a 'prompter' and get some input back from it - so this is similar to notifications IMO and so I don't see a safety issue in regards to providing this to untrusted applications. Users can easily dismiss any Prompt which is created using this interface as well.

[alexmurray]

@jdstrand I would be keen to get your review on this as well since you are more familiar with what things are generally inside the threat model for snapd or not

[alexmurray]

@jdstrand is this still on your radar?

[jdstrand]

Sorry for the delay on this (while there are only two rules, I knew it would take sometime to review gcr and pinentry-gnome3 in order to provide meaningful feedback).

For those unfamiliar, 'gcr' is 'GNOME crypto services' and it provides a DBus service as well as libgcr* APIs for programs to use. gnome-keyring and pinentry-gnome3 are two programs which use these APIs and specifically, pinentry-gnome3 uses the public gcr_prompt_password_async(), which calls the internal perform_prompt_async() which calls the gcr_secret_exchange_*() APIs (described in https://developer.gnome.org/gcr/unstable/GcrSecretExchange.html). These gcr public library APIs are what use the DBus calls for interface=org.gnome.keyring.internal.Prompter under the hood like we see in the PR. The gcr_secret_exchange_*() APIs are used by gcr_prompt_password_async() (for example) to encrypt passwords over DBus using GCR_SECRET_EXCHANGE_PROTOCOL_1 (ie, sx-aes-1; see https://developer.gnome.org/gcr/unstable/GcrSecretExchange.html#GCR-SECRET-EXCHANGE-PROTOCOL-1:CAPS).

Importantly, pinentry-gnome3 is the only pinentry binary that uses gcr (at least that I could find in Ubuntu 20.04). pinentry-gtk-2 does not use dbus, nor does pinentry-curses, pinentry-fltk, pinentry-tty, pinenttry-qt (white lie, it does use dbus for notifications, but not for secret exchange). I did see other applications that link against libgcr, so it is conceivable that these applications could make use of these accesses.

The upstream documentation states "this does not protect against active attacks like MITM attacks". Strict mode snaps are not able to sniff DBus (org.freedesktop.DBus.Monitoring is not allowed, nor "mask=eavesdrop") and are not able to bind to the the gcr DBus well-known names, so sniffing and MITM should not be possible. That said, the gcr DBus APIs have not been audited to ensure they are invulnerable to attack; the path=/org/gnome/keyring/Prompt/p[0-9]* is not snap-specific and the rule allows for receiving prompt calls for other applications.

All considered, I'd prefer this be moved to desktop-legacy instead of desktop. If the API can be proven to be invulnerable to attack by snaps, then we could consider also adding it to the desktop interface.

[jdstrand]

Please adjust this comment to be:

# Allow accessing the GNOME crypto services prompt APIs as used by
# applications using libgcr (such as pinentry-gnome3) for secure pin
# entry to unlock GPG keys etc. See:
# https://developer.gnome.org/gcr/unstable/GcrPrompt.html
# https://developer.gnome.org/gcr/unstable/GcrSecretExchange.html

[jdstrand]

This non-snap-specific rule and the upstream documentation that states that the GcrSecretExchange API "does not protect against active attacks like MITM attacks" makes me want this in desktop-legacy instead of desktop. If the API can be proven to be invulnerable to attack by snaps, then we could consider also adding it to the desktop interface.

[alexmurray]

The DBus API does not allow snooping or for other snaps to potentially receive input which was not destined for them - a prompt is initiated with the BeginPrompting method and this takes as an argument the path of the object to callback to - so the caller essentially says - start a prompt and call me back via this object which they provide the path to - this object is already registered via the DBus session bus - so they have full control over that - ie. the caller creates and pre-registers the object at /org/gnome/keyring/Prompt/p[0-9]* so they can receive the reply asynchronously via that object. So other than potentially allowing a denial-of-service via a snap continously spamming the interface I do not see that this a vulnerable attack surface to snaps.

[alexmurray]

I rebased this on current master and moved it to desktop-legacy for now (with updated comment) as per @jdstrand's request - this way we can land support for this via desktop-legacy and if / when we are satisfied it does not present an unreasonable attack surface then we can perhaps move it to desktop.

[jdstrand]

I rebased this on current master and moved it to desktop-legacy for now (with updated comment) as per @jdstrand's request - this way we can land support for this via desktop-legacy and if / when we are satisfied it does not present an unreasonable attack surface then we can perhaps move it to desktop.

If as a member of the security team you're confident after your review that the API is safe, then leaving it in desktop is ok with me.

[jdstrand]

Approving as is. If you prefer this in desktop after your gcr audit, please add a small comment above the 'receive' rule that while the dbus path is not snap-specific, the API is safe due to x, y, z.

[alexmurray]

After auditing gcr a bit more closely I am pretty sure this should be safe in terms of one snap being able to intercept credentials destined for another, but I believe there is the chance of a denial of service due to the way Gcr is coded - so the DBus interface itself is fine which I think is all we should really worry about in terms of this PR, but perhaps Gcr as a library could be a bit more defensive to try and avoid this DoS opportunity. The Gcr code is a bit convoluted due to the async nature of the APIs but I will try and summarise it here to explain how I believe this is safe from snooping on credentials:

1. Gcr uses this DBus interface in the GcrSystemPrompt object - this is in-turn used to implement the GcrPrompt interface which is what clients would actually call
2. This maintains the callback object path in a private variable prompt_path which is initialised when the object is constructed - at this point there is no checking whether the name is available via DBus - so it is possible that another snap etc may have already registered the name which the prompt wishes to use
3. perform_init_async maintains a state machine which is used to gradually initialise the object in an async manner - this is first called after the constructor has run via gcr_system_prompt_real_init_async - at this point since there is no connection yet to DBus at all, the object will attempt to connect to the session DBus.
4. Once connected to the session bus the on_bus_connected callback will be called - and this will proceed to try and register the prompt object on the session bus at prompt_path by calling register_prompt_object
5. If there was already another object registered with that name, this will return an error and this error will propagate back to the original caller who constructed the GcrSystemPrompt - ie. the primary interface to this is gcr_system_prompt_open_for_prompter_async which will propagate the error which occurs on registration of the path on dbus back to the caller.

So if a malicious snap where to go and register a heap of /org/gnome/keyring/Prompt/p[0-9]* object paths on the session bus this would stop other snaps being able to create and use this interface via Gcr (since when they go to create their prompts it would error out as the path would already exist) - however I am not sure we can protect against all DoS type attacks. Gcr could try and be smarter and when creating the prompt_path and check to see if this is available on the session bus and if not try and find one that does not exist already - but I don't think we should try and put in place snap interface policy to protect against poor interface implementations. So to my mind I feel we should allow this DBus interface via the desktop snap interface since the DBus API is safe, and if there is a possible issue with Gcr or gnome-shell or any other implementation of the interface that can be followed up separately if needed.

[alexmurray]

As per the above audit, I have moved this back to the desktop interface and added a comment in the AppArmor policy as to why this is safe.

[alexmurray]

The TravisCI failure https://travis-ci.org/snapcore/snapd/builds/656022977?utm_source=github_status appears to be a transient failure unrelated to this PR - is there any way I can retrigger that to run again?

[jdstrand]

Thanks for the PR, the follow-up research and extra commits!

Approving as is, but please see inline for a comment adjustment. Let me know if you continue to see unrelated testsuite failures and I can perform retries for you.

[alexmurray]

I notice the CI test failed again - but this doesn't look related to this change - is there anything I can do to help get this merged?

[jdstrand]

@alexmurray - this is the failure:

=== RUN   TestOverlord

----------------------------------------------------------------------
FAIL: overlord_test.go:694: overlordSuite.TestEnsureLoopPruneAbortsOld

overlord_test.go:748:
    // change was aborted
    c.Check(chg.Status(), Equals, state.HoldStatus)
... obtained state.Status = 3 ("Doing")
... expected state.Status = 1 ("Hold")

OOPS: 97 passed, 1 FAILED

This is in the unit tests and not a transient failure. I suggest merging from master (but not rebasing) and push here. That will trigger a new test run with updated master.

[alexmurray]

Thanks for the hint re merging from master @jdstrand - the unit tests at least have passed this time so 🤞

[mvo5]

Thank you for this PR