Closed
Bug 1480757
Opened 7 years ago
Closed 7 years ago
Add intent URI filtering for onLoadRequest
Categories
(GeckoView :: General, defect)
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)
12.38 KB,
patch
|
jchen
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.51 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
2.11 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
1.62 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•7 years ago
|
Group: firefox-core-security, mozilla-employee-confidential
Assignee | ||
Updated•7 years ago
|
Group: mozilla-employee-confidential
Assignee | ||
Comment 1•7 years ago
|
||
Let's make some of the intent handling utils available for GeckoView.
Assignee: nobody → esawin
Attachment #8997415 -
Flags: review?(nchen)
Assignee | ||
Comment 2•7 years ago
|
||
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)
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.
Assignee | ||
Comment 7•7 years ago
|
||
(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 8•7 years ago
|
||
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+
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8998031 -
Flags: review?(snorp)
Assignee | ||
Comment 10•7 years ago
|
||
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)
Attachment #8998031 -
Flags: review?(snorp) → review+
Attachment #8998033 -
Flags: review?(snorp) → review+
Assignee | ||
Comment 11•7 years ago
|
||
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?
![]() |
||
Comment 12•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a9bb2b62b72ce22b21520451dba85ecc5e1626e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5e0ddc5ce11c9e7b538b1928e3c899f5dc4efa
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e1dfd6ceee22640b124c312cb8bda4e916d3e09
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7fa66a761c242203eddb68a3f713ada4a1a8a5
https://hg.mozilla.org/mozilla-central/rev/4a9bb2b62b72
https://hg.mozilla.org/mozilla-central/rev/0f5e0ddc5ce1
https://hg.mozilla.org/mozilla-central/rev/9e1dfd6ceee2
https://hg.mozilla.org/mozilla-central/rev/0e7fa66a761c
Group: firefox-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Updated•7 years ago
|
status-firefox61:
--- → wontfix
status-firefox62:
--- → affected
status-firefox-esr52:
--- → unaffected
status-firefox-esr60:
--- → unaffected
Comment 13•7 years ago
|
||
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)
Assignee | ||
Comment 14•7 years ago
|
||
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 15•7 years ago
|
||
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+
![]() |
||
Comment 16•7 years ago
|
||
uplift |
![]() |
||
Comment 17•7 years ago
|
||
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)
Assignee | ||
Comment 18•7 years ago
|
||
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)
Comment 19•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/da46fa4975d8
https://hg.mozilla.org/releases/mozilla-beta/rev/10611ef867f7
https://hg.mozilla.org/releases/mozilla-beta/rev/f3a9f10f699e
Flags: needinfo?(dveditz)
Comment 20•7 years ago
|
||
Minusing for advisories since it doesn't affect a shipped product per comment 14.
Whiteboard: [adv-main62-]
Updated•6 years ago
|
Product: Firefox for Android → GeckoView
Updated•6 years ago
|
Target Milestone: Firefox 63 → mozilla63
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•