interfaces: give priority to desktop-launch over desktop-legacy #13933
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sergio-costas
force-pushed
the
give-priority-to-desktop-launch-for-desktop-files
branch
from
3bb9b6a
to
0d9de14
Compare
|
Another possibility would be to create an specific interface to allow to read .desktop files, and give it priority instead of "desktop-launch". |
ZeyadYasser
added
the
Needs security review
zyga
requested changes
May 6, 2024
zyga
reviewed
May 6, 2024
zyga
approved these changes
May 6, 2024
ZeyadYasser
approved these changes
May 7, 2024
There was a problem hiding this comment.
I don't have strong opinions on how this interface conflict should be resolved, It is a corner case and a great catch, Thank you!
IMO, I think it might be worth exploring alternative interfaces that achieves what is required from desktop-legacy if possible instead of this approach.
ZeyadYasser
added
the
Needs Samuele review
|
The big problem is that using the gnome extension forces the use of desktop-legacy, which forces being able to read the list of files in the |
alexmurray
approved these changes
May 23, 2024
There was a problem hiding this comment.
LGTM for the stated purpose from a security point-of-view - although I agree the complexity seems a bit high and I don't love the idea of essentially hard-coding the 'desktop-launch' interface within utils.go - but this feels more like a question for @pedronis etc.
alexmurray
removed
the
Needs security review
|
I have to say that, yes, this approach is a bit "unorthodox" and "ugly", so maybe another way could be to add an specific API call, allowed only if the |
pedronis
reviewed
May 27, 2024
pedronis
requested changes
Jun 4, 2024
sergio-costas
force-pushed
the
give-priority-to-desktop-launch-for-desktop-files
branch
from
0ffa8a6
to
4c25630
Compare
pedronis
reviewed
Jun 5, 2024
pedronis
approved these changes
Jun 5, 2024
|
marking blocked just because I'm asking previous reviewers to review the new test |
ZeyadYasser
requested changes
Jun 5, 2024
zyga
reviewed
Jun 6, 2024
sergio-costas
added a commit
to sergio-costas/snapd
that referenced
this pull request
Jun 10, 2024
AppArmor rules that forbide access to a resource have more priority than rules that allow access to those same resources. This means that if an interface restricts access to an specific resource, it won't be possible to enable access to that same resource from another, more privileged, interface. An example of this is reading the .desktop files of all the installed snaps in the system: the superprivileged interface 'desktop-launch' enables access to these files, so any snap that has a connected plug for this interface should be able to read them. Unfortunately, the 'desktop-legacy' interface explicitly denies access to these files, and since it is connected automatically if a snap uses the 'desktop' or the 'unity7' interfaces, this mean that no graphical application will be able to read the .desktop files, even if the super- privileged interface 'desktop-launch' interface is connected. To allow this specific case, a temporary patch ( canonical#13933) was created and merged, but it is clearly an ugly and not-generic solution. For this reason, this new patch was created, following the specification https://docs.google.com/document/d/1K-1MYhp1RKSW_jzuuyX7TSVCg2rYplKZFdJbZAupP4Y/edit This patch allows to add "prioritized snippets". Each one has an UID and a priority. If no prioritized snippet with the same UID has been previously added, the new prioritized snippet will be added like any other normal snippet. But if there is already an added snippet with the same UID, then the priority of both the old and the new snippets are compared. If the new priority is lower than the old one, the new snippet is ignored; if the new priority is bigger than the old one, the new snippet fully replaces the old one. Finally, if both priorities are the same, the new snippet will be appended to the old snippet. This generic mechanism allows to give an interface priority over others if needed, like in the previous case.
sergio-costas
added a commit
to sergio-costas/snapd
that referenced
this pull request
Jun 10, 2024
AppArmor rules that forbid access to a resource have more priority than rules that allow access to those same resources. This means that if an interface restricts access to an specific resource, it won't be possible to enable access to that same resource from another, more privileged, interface. An example of this is reading the .desktop files of all the installed snaps in the system: the superprivileged interface 'desktop-launch' enables access to these files, so any snap that has a connected plug for this interface should be able to read them. Unfortunately, the 'desktop-legacy' interface explicitly denies access to these files, and since it is connected automatically if a snap uses the 'desktop' or the 'unity7' interfaces, this mean that no graphical application will be able to read the .desktop files, even if the super- privileged interface 'desktop-launch' interface is connected. To allow this specific case, a temporary patch ( canonical#13933) was created and merged, but it is clearly an ugly and not-generic solution. For this reason, this new patch was created, following the specification https://docs.google.com/document/d/1K-1MYhp1RKSW_jzuuyX7TSVCg2rYplKZFdJbZAupP4Y/edit This patch allows to add "prioritized snippets". Each one has an UID and a priority. If no prioritized snippet with the same UID has been previously added, the new prioritized snippet will be added like any other normal snippet. But if there is already an added snippet with the same UID, then the priority of both the old and the new snippets are compared. If the new priority is lower than the old one, the new snippet is ignored; if the new priority is bigger than the old one, the new snippet fully replaces the old one. Finally, if both priorities are the same, the new snippet will be appended to the old snippet. This generic mechanism allows to give an interface priority over others if needed, like in the previous case.
sergio-costas
added a commit
to sergio-costas/snapd
that referenced
this pull request
Jun 10, 2024
AppArmor rules that forbid access to a resource have more priority than rules that allow access to those same resources. This means that if an interface restricts access to an specific resource, it won't be possible to enable access to that same resource from another, more privileged, interface. An example of this is reading the .desktop files of all the installed snaps in the system: the superprivileged interface 'desktop-launch' enables access to these files, so any snap that has a connected plug for this interface should be able to read them. Unfortunately, the 'desktop-legacy' interface explicitly denies access to these files, and since it is connected automatically if a snap uses the 'desktop' or the 'unity7' interfaces, this mean that no graphical application will be able to read the .desktop files, even if the super- privileged interface 'desktop-launch' interface is connected. To allow this specific case, a temporary patch ( canonical#13933) was created and merged, but it is clearly an ugly and not-generic solution. For this reason, this new patch was created, following the specification https://docs.google.com/document/d/1K-1MYhp1RKSW_jzuuyX7TSVCg2rYplKZFdJbZAupP4Y/edit This patch allows to add "prioritized snippets". Each one has an UID and a priority. If no prioritized snippet with the same UID has been previously added, the new prioritized snippet will be added like any other normal snippet. But if there is already an added snippet with the same UID, then the priority of both the old and the new snippets are compared. If the new priority is lower than the old one, the new snippet is ignored; if the new priority is bigger than the old one, the new snippet fully replaces the old one. Finally, if both priorities are the same, the new snippet will be appended to the old snippet. This generic mechanism allows to give an interface priority over others if needed, like in the previous case.
sergio-costas
added a commit
to sergio-costas/snapd
that referenced
this pull request
Jun 11, 2024
AppArmor rules that forbid access to a resource have more priority than rules that allow access to those same resources. This means that if an interface restricts access to an specific resource, it won't be possible to enable access to that same resource from another, more privileged, interface. An example of this is reading the .desktop files of all the installed snaps in the system: the superprivileged interface 'desktop-launch' enables access to these files, so any snap that has a connected plug for this interface should be able to read them. Unfortunately, the 'desktop-legacy' interface explicitly denies access to these files, and since it is connected automatically if a snap uses the 'desktop' or the 'unity7' interfaces, this mean that no graphical application will be able to read the .desktop files, even if the super- privileged interface 'desktop-launch' interface is connected. To allow this specific case, a temporary patch ( canonical#13933) was created and merged, but it is clearly an ugly and not-generic solution. For this reason, this new patch was created, following the specification https://docs.google.com/document/d/1K-1MYhp1RKSW_jzuuyX7TSVCg2rYplKZFdJbZAupP4Y/edit This patch allows to add "prioritized snippets". Each one has an UID and a priority. If no prioritized snippet with the same UID has been previously added, the new prioritized snippet will be added like any other normal snippet. But if there is already an added snippet with the same UID, then the priority of both the old and the new snippets are compared. If the new priority is lower than the old one, the new snippet is ignored; if the new priority is bigger than the old one, the new snippet fully replaces the old one. Finally, if both priorities are the same, the new snippet will be appended to the old snippet. This generic mechanism allows to give an interface priority over others if needed, like in the previous case.
The interface 'desktop-legacy' (and 'unity7') specifically denies read access to the .desktop files, which means that any extension that requires it (like gnome or kde) won't be able to read them. Unfortunately, there are some specific cases where reading the .desktop files is mandatory, like when implementing the new Refresh Awareness specification. This specification requires to show the "visible name" of a snap, and its icon, and in order to have access to that, it is mandatory to be able to read the .desktop files. The 'desktop-launch' interface does include read access to the .desktop files. Although it is a very privileged interface, it is not a problem because the snaps that implement the Refresh Awareness specification are too, so using it to gain access to the .desktop files should be enough. Unfortunately, mixing it with 'desktop-legacy' interface (which happens when the snap implementing the Refresh Awareness specification also uses the gnome or the kde extension) results in not having access to the files, because the 'deny' rules set by the later have priority over any 'allow' rule set by the former. This PR adds a check when adding the specific .desktop rules in the 'desktop-legacy' interface: if the snap has a plug for the 'desktop-launch' interface, it won't apply the .desktop rules. This is not a problem, because without them, no access is granted by default (the rules added by 'desktop-legacy' allow to list the .desktop files, but not read them).
sergio-costas
force-pushed
the
give-priority-to-desktop-launch-for-desktop-files
branch
from
ad6d7bf
to
7e41181
Compare
|
Got go-ahead from @sergio-costas to merge |
sergio-costas
added a commit
to sergio-costas/snapd
that referenced
this pull request
Jul 2, 2024
AppArmor rules that forbid access to a resource have more priority than rules that allow access to those same resources. This means that if an interface restricts access to an specific resource, it won't be possible to enable access to that same resource from another, more privileged, interface. An example of this is reading the .desktop files of all the installed snaps in the system: the superprivileged interface 'desktop-launch' enables access to these files, so any snap that has a connected plug for this interface should be able to read them. Unfortunately, the 'desktop-legacy' interface explicitly denies access to these files, and since it is connected automatically if a snap uses the 'desktop' or the 'unity7' interfaces, this mean that no graphical application will be able to read the .desktop files, even if the super- privileged interface 'desktop-launch' interface is connected. To allow this specific case, a temporary patch ( canonical#13933) was created and merged, but it is clearly an ugly and not-generic solution. For this reason, this new patch was created, following the specification https://docs.google.com/document/d/1K-1MYhp1RKSW_jzuuyX7TSVCg2rYplKZFdJbZAupP4Y/edit This patch allows to add "prioritized snippets". Each one has an UID and a priority. If no prioritized snippet with the same UID has been previously added, the new prioritized snippet will be added like any other normal snippet. But if there is already an added snippet with the same UID, then the priority of both the old and the new snippets are compared. If the new priority is lower than the old one, the new snippet is ignored; if the new priority is bigger than the old one, the new snippet fully replaces the old one. Finally, if both priorities are the same, the new snippet will be appended to the old snippet. This generic mechanism allows to give an interface priority over others if needed, like in the previous case.
sergio-costas
added a commit
to sergio-costas/snapd
that referenced
this pull request
Jul 8, 2024
AppArmor rules that forbid access to a resource have more priority than rules that allow access to those same resources. This means that if an interface restricts access to an specific resource, it won't be possible to enable access to that same resource from another, more privileged, interface. An example of this is reading the .desktop files of all the installed snaps in the system: the superprivileged interface 'desktop-launch' enables access to these files, so any snap that has a connected plug for this interface should be able to read them. Unfortunately, the 'desktop-legacy' interface explicitly denies access to these files, and since it is connected automatically if a snap uses the 'desktop' or the 'unity7' interfaces, this mean that no graphical application will be able to read the .desktop files, even if the super- privileged interface 'desktop-launch' interface is connected. To allow this specific case, a temporary patch ( canonical#13933) was created and merged, but it is clearly an ugly and not-generic solution. For this reason, this new patch was created, following the specification https://docs.google.com/document/d/1K-1MYhp1RKSW_jzuuyX7TSVCg2rYplKZFdJbZAupP4Y/edit This patch allows to add "prioritized snippets". Each one has an UID and a priority. If no prioritized snippet with the same UID has been previously added, the new prioritized snippet will be added like any other normal snippet. But if there is already an added snippet with the same UID, then the priority of both the old and the new snippets are compared. If the new priority is lower than the old one, the new snippet is ignored; if the new priority is bigger than the old one, the new snippet fully replaces the old one. Finally, if both priorities are the same, the new snippet will be appended to the old snippet. This generic mechanism allows to give an interface priority over others if needed, like in the previous case.
ernestl
pushed a commit
that referenced
this pull request
Jul 8, 2024
* Add snippets with priorities AppArmor rules that forbid access to a resource have more priority than rules that allow access to those same resources. This means that if an interface restricts access to an specific resource, it won't be possible to enable access to that same resource from another, more privileged, interface. An example of this is reading the .desktop files of all the installed snaps in the system: the superprivileged interface 'desktop-launch' enables access to these files, so any snap that has a connected plug for this interface should be able to read them. Unfortunately, the 'desktop-legacy' interface explicitly denies access to these files, and since it is connected automatically if a snap uses the 'desktop' or the 'unity7' interfaces, this mean that no graphical application will be able to read the .desktop files, even if the super- privileged interface 'desktop-launch' interface is connected. To allow this specific case, a temporary patch ( #13933) was created and merged, but it is clearly an ugly and not-generic solution. For this reason, this new patch was created, following the specification https://docs.google.com/document/d/1K-1MYhp1RKSW_jzuuyX7TSVCg2rYplKZFdJbZAupP4Y/edit This patch allows to add "prioritized snippets". Each one has an UID and a priority. If no prioritized snippet with the same UID has been previously added, the new prioritized snippet will be added like any other normal snippet. But if there is already an added snippet with the same UID, then the priority of both the old and the new snippets are compared. If the new priority is lower than the old one, the new snippet is ignored; if the new priority is bigger than the old one, the new snippet fully replaces the old one. Finally, if both priorities are the same, the new snippet will be appended to the old snippet. This generic mechanism allows to give an interface priority over others if needed, like in the previous case. * Remove slices.Contains, since seems to be not supported * Update interfaces/apparmor/spec.go Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]> * Use testutils.Contains * Replace "uid" with "key" for clarity and sanity * Add specific type for priority keys and force registering them * Remove unneeded return * Use SnippetKey as type * Don't use "slice" since MacOS seems to not support it * Update interfaces/apparmor/spec.go Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]> * Update interfaces/apparmor/spec.go Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]> * Use String instead of GetValue * Use SnippetKey as key instead of the inner string * Update interfaces/connection.go Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]> * Several changes requested * Create the SnippetKeys inside Spec * Move key registration outside Spec This creates a centralized key registry inside apparmor module, so keys can be registered using top variables, and any duplicated key will produce a panic when snapd is launched, thus just panicking in any test too. * Added extra ways of working with SnippetKeys * Add extra check * Replace GetSnippetKey with GetSnippetKeys * Update the priority code use case A previous PR was merged with a Quick&Dirty(tm) solution to the priority problem between unity7 and desktop-legacy interfaces against desktop-launch interface. Now that it has been merged, that code must be updated to the new mechanism implemented in this PR. This is exactly what this commit does. * Add explanation and constants for prioritized snippets * Fix prioritized snippet key and add test in all_test * Several changes requested by Zygmunt Vazyli --------- Co-authored-by: Zygmunt Bazyli Krynicki <[email protected]>
This was referenced Jul 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The interface 'desktop-legacy' (and 'unity7') specifically denies read access to the .desktop files, which means that any extension that requires it (like gnome or kde) won't be able to read them.
Unfortunately, there are some specific cases where reading the .desktop files is mandatory, like when implementing the new Refresh Awareness specification. This specification requires to show the "visible name" of a snap, and its icon, and in order to have access to that, it is mandatory to be able to read the .desktop files.
The 'desktop-launch' interface does include read access to the .desktop files. Although it is a very privileged interface, it is not a problem because the snaps that implement the Refresh Awareness specification are too, so using it to gain access to the .desktop files should be enough. Unfortunately, mixing it with 'desktop-legacy' interface (which happens when the snap implementing the Refresh Awareness specification also uses the gnome or the kde extension) results in not having access to the files, because the 'deny' rules set by the later have priority over any 'allow' rule set by the former.
This PR adds a check when adding the specific .desktop rules in the 'desktop-legacy' interface: if the snap has a plug for the 'desktop-launch' interface, it won't apply the .desktop rules. This is not a problem, because without them, no access is granted by default (the rules added by 'desktop-legacy' allow to list the .desktop files, but not read them).
Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?