1263627 - It should not be possible to manually navigate to internal URLs

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

We have a number of 'internal' URLs that are hosted on localhost. It should not be possible to navigate to these URLs manually or through JavaScript.

Ideally all these URLs have some form of CSRF protection, like including a secret random token in the request. That will prevent people from being able to manually call these internal endpoints.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1725#attch-to-bugzilla

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1725

Comment 3

[Brian Nicholson (:bnicholson)]

Note that there's still a tricky case this bug doesn't handle: links from reader mode. Filed bug 1265115.

Comment 4

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

This looks good. I would just like to see a unit (or UI) test that shows that these loads are now failing.

Comment 5

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8741980 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1725

LGTM Just needs a test. If the test needs to run against an external site, maybe just move it to a separate LiveTests case so that that we can control when it runs?

Comment 6

[Brian Nicholson (:bnicholson)]

Added a test in the PR.

Unfortunately, I don't think this is ready yet after more testing. In addition to bug 1265115 mentioned above, there's another tricky exploit:
1) Use window.open() to go to some invalid URL.
2) This will load an error page.
3) With the reference to this opened window, set win.location.href to the malicious sessionrestore URL.

Like bug 1265115, this exploits the fact that requests from localhost pages are allowed to load other localhost pages.

To fix this properly, I think we'll want to remove that rule, meaning localhost pages will be treated the same as any other page. That means if we want to access local pages from other local pages (which we currently need for error pages, session restore, etc.), we'll have to route these requests through messages to Swift, where we can then create explicit PrivilegedRequests.

Comment 7

[Brian Nicholson (:bnicholson)]

PoC of exploit from comment 6: https://people.mozilla.org/~bnicholson/test/localhost-nav-test.html

Use the "window error + session test" button.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8741980 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1725

Updated with stricter requirements and tests.

Now, no pages (including local ones) will be able load other local pages directly; the only way to load anything on localhost is through PrivilegedRequest. That means if local pages want to load another URL or even reload the current page, they'll have to funnel that request through the Swift layer to make the request privileged. I created a shared LocalRequestHelper to handle these messages.

Comment 10

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8741980 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1725

This is a great patch. I left a few comments though.

Comment 11

[Brian Nicholson (:bnicholson)]

master: https://github.com/mozilla/firefox-ios/commit/78df359fd64aa7fc98bb2e1e7f65863c434fd3bb
v4.x: fe4bdfe

Comment 12

[Aaron Train [:aaronmt]]

Think this is causing bug https://bugzilla.mozilla.org/show_bug.cgi?id=1268871