Closed Bug 1182695 (android-intents) Opened 9 years ago Closed 4 years ago

[meta] Intent handling consolidation

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mcomella, Unassigned)

References

Details

(Keywords: meta, sec-other)

The Intent handling code and regressions are spaghetti - let's consolidate. This is intentionally security-restricted (see below). The original implementation of intent:// uris was in bug 851693. However, there are security risks to opening these directly so I fixed that in bug 1168998 - notably, this involved adding CATEGORY_BROWSABLE to the intent:// uris. After re-reading the documentation [1], I intepretted it as *all* Intents leaving the browser should have CATEGORY_BROWSABLE - I implemented this in part 1 of (mostly unrelated) bug 1168662. After speaking with Margaret, we realized the documentation means to say that CATEGORY_BROWSABLE should be added to any links clicked on in web content. There is also a question of whether or not users actions should also get this category (e.g. typing in a url - if a malicious app can type in an intent url, it has the same effect as clicking a link). We will investigate how feasible this is to implement. Adding CATEGORY_BROWSABLE to all Intents caused a regression with file:// uris (bug 1100100), specifically downloads and sharing via the action bar & reader view screen (bug 1175532). Notably, plain text shared via the selection handler should not get CATEGORY_BROWSABLE even though it's technically web content, the distinction being the content is not opening another Android app directly. My v1 fix was to be in bug 1182328 and only put CATEGORY_BROWSABLE on intent://, android-app://, and customUri:// schemes - this does not cover all web content and thus may not be 100% secure, but it covers the security issues outlined in the original security bug and covers the majority use case. The v2 fix is to explicitly find the links coming from web content (bug 1182140). Notes: * intent:// handling is in 41+ * p11 requested CATEGORY_BROWSABLE on custom URI schemes After talking with Margaret, we came up with the following (non-final) solution: 41: We need to make 41 work for our users, not p11, so we should ensure there are no regressions by adding CATEGORY_BROWSABLE only to intent:// and android-app uris, which adds a new intent handling code path and thus is unlikely to break anything. 42: We go for the ideal solution and add CATEGORY_BROWSABLE to all links from web content - bug 1182140. --- API 22 added a similar android-app:// uri, which I worked on in bug 1162182. I never landed it because I later found out that our custom intent handling code made android-app:// uris *just work*, rather than requiring Intent.parseUri as I had been using for intent:// uris, hence why I filed bug 1173625. --- Wes suggested we (unlike Chrome) prompt the user before opening an Android app through a uri - bug 1173147. --- We also don't correctly handle mimeTypes in custom uri schemes where web extensions do not open the Android app (bug 1173200). The issue was that an explicit mimeType was set and most apps do not expect this (outside of file:// uris). The quick solution was to only use mimeTypes on file:// uris but Wes pointed out that we can create Intents from multiple Intent resolutions (bug 1173200 comment 18) so the ideal solution is likely to create Intents for implicit and explicit mimeTypes - though I'm not sure if this should be for all external intents. [1]: http://developer.android.com/reference/android/content/Intent.html#CATEGORY_BROWSABLE
(In reply to Michael Comella (:mcomella) from comment #0) > 41: > We need to make 41 work for our users, not p11, so we should ensure there > are no regressions by adding CATEGORY_BROWSABLE only to intent:// and > android-app uris, which adds a new intent handling code path and thus is > unlikely to break anything. I'm going to use bug 1182328 for this.
I also have bug 1181698 to clean up getOpenURIIntentInner.
Depends on: 1181698
Then there's bug 1173626 about handling android-app uris on API < 22 devices.
Depends on: 1173626
Note that not specifying CATEGORY_BROWSABLE does not limit our app choices - an intent filter can specify unmatched categories.
I'm marking this sec-other because it sounds like you are filing separate bugs for the individual issues.
Keywords: sec-other
I fixed the issues on 42 by applying the non-comprehensive fix from 41 (bug 1182328) onto 42 (bug 1175532). This will ride the trains but we should apply the comprehensive fix (bug 1182140) in 43.
Group: core-security → firefox-core-security
Keywords: meta
Group: firefox-core-security → mobile-core-security

Agi/Petru would you have a look here and see if there is anything here that applies to Fenix?

Flags: needinfo?(petru.lingurar)
Flags: needinfo?(agi)

I think there are some changes to be made.
Filed https://github.com/mozilla-mobile/fenix/issues/16251 for Fenix.

Looking into this we saw that intent links [1] are already handled in Fenix with the help of AC - App-Links [2] which already adds CATEGORY_BROWSABLE to both the intent and the intent selector and additionally sets the intent as an implicit one by having the component set to null [3].

[1] https://developer.chrome.com/multidevice/android/intents
[2] https://github.com/mozilla-mobile/android-components/tree/master/components/feature/app-links
[3] https://github.com/mozilla-mobile/android-components/blob/06a2562cabbb7b2da722c491e8c127662fbcd5dc/components/feature/app-links/src/main/java/mozilla/components/feature/app/links/AppLinksUseCases.kt#L152-L159

As such, I think there's nothing else to be done on Fenix so I'll close the Fenix ticket.

Flags: needinfo?(petru.lingurar)
Flags: needinfo?(agi)

Michael, if you agree I think this meta can now be closed also.

Flags: needinfo?(michael.l.comella)

(In reply to Petru-Mugurel Lingurar [:petru] from comment #11)

Michael, if you agree I think this meta can now be closed also.

I don't have much context on this anymore but I'm not actively using it so that sounds okay to me.

Flags: needinfo?(michael.l.comella)
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Group: mobile-core-security → core-security-release
Product: Firefox for Android → Firefox for Android Graveyard
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.