Closed Bug 1480757 Opened Last year Closed Last year

Add intent URI filtering for onLoadRequest

Categories

(GeckoView :: General, defect)

51 Branch
All
Android
defect
Not set

Tracking

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 wontfix, firefox62 fixed, firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- wontfix
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: esawin, Assigned: esawin)

References

Details

(Whiteboard: [adv-main62-])

Attachments

(4 files)

Currently, all (non-redirect) load requests are forwarded to onLoadRequest, including some that may be unsafe to load.

We should block unsafe URIs like intent URI containing file data schemes, see bug 1356893.
Group: firefox-core-security, mozilla-employee-confidential
Group: mozilla-employee-confidential
Let's make some of the intent handling utils available for GeckoView.
Assignee: nobody → esawin
Attachment #8997415 - Flags: review?(nchen)
Block unsafe intent URI (or other unsafe URIs) from onLoadRequest dispatch.

Do we want to signal onLoadError in this case?
Attachment #8997417 - Flags: review?(snorp)
Blocks: 1478171
Comment on attachment 8997417 [details] [diff] [review]
0002-Bug-1480757-2.0-Block-unsafe-URI-intent-load-request.patch

Review of attachment 8997417 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +303,5 @@
>                      final String uri = message.getString("uri");
>                      final int where = convertGeckoTarget(message.getInt("where"));
>                      final int flags = filterFlags(message.getInt("flags"));
>  
> +                    if (!IntentUtils.isUriSafeForScheme(uri)) {

This looks to be in GeckoAppShell and also does not do anything for intent:// URIs.
Attachment #8997417 - Flags: review?(snorp) → review-
(In reply to Eugen Sawin [:esawin] from comment #2)
> Created attachment 8997417 [details] [diff] [review]
> 0002-Bug-1480757-2.0-Block-unsafe-URI-intent-load-request.patch
> 
> Block unsafe intent URI (or other unsafe URIs) from onLoadRequest dispatch.
> 
> Do we want to signal onLoadError in this case?

Yeah, I think that would be appropriate. We should be careful adding new errors on top of Gecko's, though. Maybe we can use an existing one in this case.
Comment on attachment 8997417 [details] [diff] [review]
0002-Bug-1480757-2.0-Block-unsafe-URI-intent-load-request.patch

Review of attachment 8997417 [details] [diff] [review]:
-----------------------------------------------------------------

Oops I see the previous patch now, sorry!
Attachment #8997417 - Flags: review- → review+
Comment on attachment 8997415 [details] [diff] [review]
0001-Bug-1480757-1.0-Move-safe-intent-handling-utilities-.patch

Review of attachment 8997415 [details] [diff] [review]:
-----------------------------------------------------------------

It would be great if we had unit tests for these.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4)
> (In reply to Eugen Sawin [:esawin] from comment #2)
> > Created attachment 8997417 [details] [diff] [review]
> > 0002-Bug-1480757-2.0-Block-unsafe-URI-intent-load-request.patch
> > 
> > Block unsafe intent URI (or other unsafe URIs) from onLoadRequest dispatch.
> > 
> > Do we want to signal onLoadError in this case?
> 
> Yeah, I think that would be appropriate. We should be careful adding new
> errors on top of Gecko's, though. Maybe we can use an existing one in this
> case.

Which error would be appropriate, maybe ERROR_FILE_ACCESS_DENIED (since it's an URI issue) or ERROR_UNSAFE_CONTENT_TYPE (if we consider the intent's data as content)?
Comment on attachment 8997415 [details] [diff] [review]
0001-Bug-1480757-1.0-Move-safe-intent-handling-utilities-.patch

Review of attachment 8997415 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah a local unit test (similar to GeckoBundleTest) would be nice.
Attachment #8997415 - Flags: review?(nchen) → review+
I'm still not sure if MALFORMED_URI is the best error, but it seems to be the most general from the URI category.
Attachment #8998033 - Flags: review?(snorp)
Comment on attachment 8997415 [details] [diff] [review]
0001-Bug-1480757-1.0-Move-safe-intent-handling-utilities-.patch

Approval Request Comment
[Feature/Bug causing the regression]: N/A.
[User impact if declined]: A potential GeckoView exploit like bug 1356893.
[Is this code covered by automated tests?]: Local tests only, included in this bug.
[Has the fix been verified in Nightly?]: It's not in Nightly yet.
[Needs manual test from QE? If yes, steps to reproduce]: STR see bug 1356893.
[List of other uplifts needed for the feature/fix]: All patches in this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It only affects GeckoView URI filtering/blocking.
[String changes made/needed]: None.
Attachment #8997415 - Flags: approval-mozilla-beta?
This probably should have had a security rating and sec approval before landing. Also, usually best not to land tests for security-sensitive issues until after they are public.
Flags: needinfo?(esawin)
Flags: needinfo?(dveditz)
The exploit is known and public (bug 1356893) and given that no official Mozilla product is currently affected by this issue (Focus has its own checks for intents), I haven't moved it through sec review.

I've marked it sec-critical for not bringing extra attention to the exploit for potential third-party apps based on GeckoView (that we are not aware of).

I'm sorry if that was not the adequate process to handle this, we haven't intended to mark this issue sec-critical originally.
Flags: needinfo?(esawin)
Comment on attachment 8997415 [details] [diff] [review]
0001-Bug-1480757-1.0-Move-safe-intent-handling-utilities-.patch

Geckoview-only fix, let's uplift. 
Thanks for the explanation!
Attachment #8997415 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This had to be backed out for build bustages in GeckoSession.java:

https://hg.mozilla.org/releases/mozilla-beta/rev/7bcf460f41d7a40d20ef268b5e158c0dd23edf76

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=e8db9f7d2225366699c4c2c73d1d4ab465700253&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=194043783&repo=mozilla-beta

[task 2018-08-15T08:41:59.375Z] 08:41:59     INFO -  :geckoview:compileOfficialWithGeckoBinariesNoMinApiDebugJavaWithJavac/builds/worker/workspace/build/src/mobile/android/thirdparty/ch/boye/httpclientandroidlib/conn/HttpHostConnectException.java:56: warning: non-varargs call of varargs method with inexact argument type for last parameter;
[task 2018-08-15T08:41:59.375Z] 08:41:59     INFO -          this(cause, host, null);
[task 2018-08-15T08:41:59.375Z] 08:41:59     INFO -                            ^
[task 2018-08-15T08:41:59.375Z] 08:41:59     INFO -    cast to InetAddress for a varargs call
[task 2018-08-15T08:41:59.376Z] 08:41:59     INFO -    cast to InetAddress[] for a non-varargs call and to suppress this warning
[task 2018-08-15T08:41:59.376Z] 08:41:59     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:210: error: cannot find symbol
[task 2018-08-15T08:41:59.376Z] 08:41:59     INFO -                                               NavigationDelegate.ERROR_CATEGORY_URI,
[task 2018-08-15T08:41:59.376Z] 08:41:59     INFO -                                                                 ^
[task 2018-08-15T08:41:59.376Z] 08:41:59     INFO -    symbol:   variable ERROR_CATEGORY_URI
[task 2018-08-15T08:41:59.377Z] 08:41:59     INFO -    location: interface NavigationDelegate
[task 2018-08-15T08:41:59.377Z] 08:41:59     INFO -  /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:211: error: cannot find symbol
[task 2018-08-15T08:41:59.377Z] 08:41:59     INFO -                                               NavigationDelegate.ERROR_MALFORMED_URI);
[task 2018-08-15T08:41:59.377Z] 08:41:59     INFO -                                                                 ^
[task 2018-08-15T08:41:59.377Z] 08:41:59     INFO -    symbol:   variable ERROR_MALFORMED_URI
[task 2018-08-15T08:41:59.378Z] 08:41:59     INFO -    location: interface NavigationDelegate

Those are from bug 1451476 which didn't get uplifted to 62.
Flags: needinfo?(esawin)
That's correct the page load error API will be riding the 63 train.
For beta, we can ignore patch 4 from this bug, it's non-critical and is the one causing the conflict.
Flags: needinfo?(esawin)
Minusing for advisories since it doesn't affect a shipped product per comment 14.
Whiteboard: [adv-main62-]
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 63 → mozilla63
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.