Merge lp:~thomas-voss/trust-store/fix-1504022 into lp:trust-store/15.04
| Status: | Superseded | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Proposed branch: | lp:~thomas-voss/trust-store/fix-1504022 | ||||||||
| Merge into: | lp:trust-store/15.04 | ||||||||
| Diff against target: |
1644 lines (+1211/-92) 20 files modified
3rd_party/xdg/CMakeLists.txt (+22/-0) 3rd_party/xdg/LICENSE (+166/-0) 3rd_party/xdg/README.md (+87/-0) 3rd_party/xdg/xdg.cpp (+203/-0) 3rd_party/xdg/xdg.h (+118/-0) 3rd_party/xdg/xdg_test.cpp (+130/-0) CMakeLists.txt (+4/-1) debian/control (+3/-0) debian/source/format (+1/-1) src/CMakeLists.txt (+8/-0) src/core/trust/impl/sqlite3/store.cpp (+2/-8) src/core/trust/mir/agent.cpp (+11/-38) src/core/trust/mir/agent.h (+29/-14) src/core/trust/mir/click_desktop_entry_app_name_resolver.cpp (+106/-0) src/core/trust/mir/click_desktop_entry_app_name_resolver.h (+49/-0) tests/CMakeLists.txt (+29/-8) tests/click_desktop_entry_app_name_resolver_test.cpp (+60/-0) tests/mir_agent_test.cpp (+123/-22) tests/share/applications/valid.pkg_app_0.0.0.desktop (+55/-0) tests/test_data.h.in (+5/-0) |
||||||||
| To merge this branch: | bzr merge lp:~thomas-voss/trust-store/fix-1504022 | ||||||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Tyler Hicks | Needs Information on 2015-11-20 | ||
| Alberto Mardegan (community) | 2015-11-11 | Approve on 2015-11-12 | |
|
Review via email:
|
|||
This proposal supersedes a proposal from 2015-11-11.
This proposal has been superseded by a proposal from 2015-11-15.
Commit Message
Add interface mir::AppNameRes
mir::AppNameRes
to localized application names.
Description of the Change
Add interface mir::AppNameRes
mir::AppNameRes
to localized application names.
- 138. By Thomas Voß on 2015-11-12
-
Robustify desktop entry lookup, relying on xdg dirs to search for matching entries.
- 139. By Thomas Voß on 2015-11-12
-
Rename test for mir::ClickDeskt
opEntryAppNameR esolver.
- 140. By Thomas Voß on 2015-11-14
-
Add missing , in debian/control.
- 141. By Thomas Voß on 2015-11-15
-
rely on xdg:: for querying xdg base spec directories.
| Tyler Hicks (tyhicks) wrote : | # |
Hello - Thomas had asked for a security review and I just took a look at the code.
The code itself looks nearly perfect. I have a couple questions below but they're minor.
I'm mainly concerned about the concept of using the localized Name of the desktop file in a trust prompt. For something like a click app that comes from an untrusted developer, we have no idea if the localized name in their provided desktop file is an accurate translation or a malicious translation intended to trick the user into granting their app access. I don't know of any logic that could be used in the click reviewers tools to verify that the translations are non-malicious. How do we handle such a situation?
As for the code, I have a couple questions/comments:
1) What UID does resolve_
2) The external XDG stuff that you wrote is a nice idea. I'd recommend packaging it up as a library so that it can be reused in a controllable fashion instead of copying and pasting the code into all of the projects that want to use it.
| Tyler Hicks (tyhicks) wrote : | # |
After speaking with jdstrand, he explained why the click reviewers tools don't currently verify the Name field of a desktop file. The desktop file name field can't be forced to match the click package name because the desktop file name should be "pretty" while the click package name is alphanumerics and '-'.
Because of this, I don't see how the desktop file name field can be trusted enough to be used in trust store prompts. :/
| Thomas Voß (thomas-voss) wrote : | # |
Hey Tyler,
thanks for the detailed review and your comments. Let me first try to clarify on the use of the localized name in trust prompts. I actually raised the same issue as you, and it turns out that we are "vulnerable" to applications faking their identity in a couple of places throughout the system. First and foremost: Unity itself relies on the .desktop file entry for displaying the name. We might want to fix the default approach for handling localized application names meant for human consumption in snappy, though.
One possible way to resolve the issue is by altering the prompt to show the localized name alongside the actual application id. Ideally, we would have an expendable field "Details" as part of the prompt containing in-depth information about the request, at the very least the actual app ID together with the UID (and the human-readable user name).
(@1:) The function always runs under the actual user. I will however add a check to make sure that the current user id matches the user id of the request. If not, we will return early, and deny the request.
(@2:) Sure, happy to. I mainly added the external code here to guarantee a swift landing. I will do the packaging and carry out the requesting asap, but would like to make sure that a fix for this bug is not blocked by package/MIR review processes.
| Thomas Voß (thomas-voss) wrote : | # |
One other idea to further clarify the identity of the app might be to include its official icon. At the very least, this would make it easier for the user to associate the source of the trust request with what is visible in the dash and in the launcher.
- 142. By Thomas Voß on 2015-11-26
-
Address comments in review and implement new prompt design.
- 143. By Thomas Voß on 2015-11-27
-
Ensure that rendering is consistent with app icon rendering on launcher/dash.
- 144. By Thomas Voß on 2015-11-27
-
Update pot to account for string changes.
- 145. By Thomas Voß on 2015-11-30
-
Change color of negative action to neutral, see:
https://developer. ubuntu. com/api/ apps/qml/ sdk-14. 10/Ubuntu. Components. UbuntuColors/ #lightGrey- prop
Fix apostrophe to be a real typographic apostrophe. - 146. By Thomas Voß on 2015-11-30
-
Align with app scope display of icons, see:
https://code.launchpad .net/~cimi/ unity8/ new-shadows- 1.3/+merge/ 271611

A couple of inline comments, I'm not convinced that this way of finding the desktop files is reliable.