Closed Bug 852848 Opened 12 years ago Closed 12 years ago

Facebook oauth integration relies on content loaded over http (http://intense-tundra-4122.herokuapp.com/fbowd/oauth2_new/flow.html)

Categories

(Core Graveyard :: DOM: Apps, defect)

Product:
Core Graveyard
Component:
DOM: Apps
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:tef+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, firefox-esr17 unaffected, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Status:
RESOLVED FIXED
Milestone:
1.0.1 IOT1 (10may)
Project Flags:
blocking-b2g tef+
Tracking Flags:
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
firefox-esr17 --- unaffected
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: pauljt, Assigned: fabrice)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, sec-high, Whiteboard: [madrid][adv-main24-])

Attachments

(2 files, 6 obsolete files)

Gaia modifications to use this patch
356 bytes, text/html
jmcf
: feedback+
Details
patch
9.74 KB, patch
ferjm
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
I think this is actually reasonably well-known (see https://groups.google.com/forum/#!msg/mozilla.dev.webapps/EeflBoDrYfk/q12HPoDdFocJ ), but I couldn't find a bug, so I am raising this to make sure. As far as I understand the situation: - oauth doesnt support the app:// protocol - so we have a shim to redirect to after a user authenticates with facebook - this shim just forwards the URL params to the parent app frame using postMessage One obvious issue is that shim is not SSL: http://intense-tundra-4122.herokuapp.com/fbowd/oauth2_new/flow.html This needs to be SSL at least, and seems like it should be hosted somewhere more legit. Am I missing something here? Does this get configured somehow by the carrier at build time? It also seems quite dangerous to be loading content over the network at all (can we prepopulate an app-cache or something ? (what if intense-tundra-4122.herokuapp.com gets owned? If nothing else, you can phish all phones )
See bug 851760. :)
oh yes, intense-tundra is only a temporary hosting solution for dev
I think facebook happily redirects to any custom url scheme btw. This is common practice for iOS app. You register a protocol handler like "myapp-facebook-oauth-handler://" and then you don't need the server part anymore. We can probably get rid of the server bits by allowing our 'webviews' to intercept requests or allow custom url schemes per app. This is also useful for many other kind of apps. It is widely used on both iOS and Android in apps that need to talk to 'webby' stuff.
(In reply to Stefan Arentz [:st3fan] from comment #3) > I think facebook happily redirects to any custom url scheme btw. This is > common practice for iOS app. You register a protocol handler like > "myapp-facebook-oauth-handler://" and then you don't need the server part > anymore. That's not true. The configuration page for the redirection URI does not allow to set protocols different than http(s). > > We can probably get rid of the server bits by allowing our 'webviews' to > intercept requests or allow custom url schemes per app. Yes, but that would require platform support. Nonetheless, would be great to get rid of server side > > This is also useful for many other kind of apps. It is widely used on both > iOS and Android in apps that need to talk to 'webby' stuff.
Hi, bug 851760 is already open to get ride of the heroku domain and host this on mozilla servers. Like the idea of the custom handler, but as well some services won't allow you to specify a handler different than http(s), as Jose Manuel commented and I can confirm as well with Google OAuth services. Cheers!
blocking-b2g: tef? → tef+
Depends on: 851760
(In reply to Francisco Jordano [:arcturus] from comment #5) > Hi, > > bug 851760 is already open to get ride of the heroku domain and host this on > mozilla servers. > > Like the idea of the custom handler, but as well some services won't allow > you to specify a handler different than http(s), as Jose Manuel commented > and I can confirm as well with Google OAuth services. Yeah I think you are right. What I have probably done in the past with iOS apps that I have worked on is simply intercept the (http/https) redirect in the webview. Then you don't need a special url scheme. But, this is something that our web views (mozbrowser iframe) cannot do yet I think.
For the record, I have a patch that intercepts the http(s) redirection requests, cancel them and can then do something with the parameters that are passed to the redirection URL. What I have not figured out is how to send these parameters back to gaia where we want them.
(In reply to Fabrice Desré [:fabrice] from comment #7) > For the record, I have a patch that intercepts the http(s) redirection > requests, cancel them and can then do something with the parameters that are > passed to the redirection URL. What I have not figured out is how to send > these parameters back to gaia where we want them. Oh that is awesome Fabrice. How did you do the redirect intercept? I think all you need to do is postMessage() the parameters back from the mozbrowser iframe to its parent.
Well, I did it on the chrome side with an http-on-modify-request observer. But at this point I don't have a reference to the window that started the redirection, so I can't do much more that firing a mozChromeEvent to the system app. Maybe I should try to see if I can get that in the mozbrowser api implementation instead. Stay tuned...
(coming to this party late, sorry if I'm misunderstanding) Should this URL be a pref or be customizable like the boot splash screen images?
(In reply to Andrew Overholt [:overholt] from comment #10) > (coming to this party late, sorry if I'm misunderstanding) Should this URL > be a pref or be customizable like the boot splash screen images? yes, we should not hardcode that at all. Given that it's on the Gaia side, we'll need to use either a setting (if it needs to be shared by different apps) or have it in some json config file.
Attached patch gecko patch (obsolete) — — Splinter Review
I'm attaching this patch here just because we discussed that with Stefan, but if we want to go in this direction I'll open a new bug. With this path, we can declare through preferences the oauth redirection urls that we want to serve ourselves without hitting the server. Redirecting to app:// pages works like a charm afaict. In addition to the gecko patch, you need this small gaia modification, and of course we'll have to add the currently hosted pages as in the communication app. diff --git a/apps/system/js/popup_manager.js b/apps/system/js/popup_manager.js index 7e1ed84..dcb1e5c 100644 --- a/apps/system/js/popup_manager.js +++ b/apps/system/js/popup_manager.js @@ -77,6 +77,7 @@ var PopupManager = { popup.addEventListener('mozbrowserloadend', this); popup.addEventListener('mozbrowserloadstart', this); popup.addEventListener('mozbrowserlocationchange', this); + popup.addEventListener('mozbrowseroauthredirect', this); }, close: function pm_close(evt) { @@ -144,6 +145,10 @@ var PopupManager = { this.throbber.classList.remove('loading'); break; + case 'mozbrowseroauthredirect': + console.log("Got OAuth redirect!"); + evt.target.src = evt.detail.to; + break; case 'mozbrowserlocationchange': evt.target.dataset.url = evt.detail;
Attached patch gecko patch v2 (obsolete) — — Splinter Review
Better patch for gecko, simpler and which doesn't need the gaia patch anymore (except adding the new pages of course).
Attachment #727493 - Attachment is obsolete: true
(In reply to Fabrice Desré [:fabrice] from comment #13) > Created attachment 727509 [details] [diff] [review] > gecko patch v2 > > Better patch for gecko, simpler and which doesn't need the gaia patch > anymore (except adding the new pages of course). the redirection pages are already on the Gaia tree, see https://github.com/mozilla-b2g/gaia/tree/master/apps/communications/contacts/oauth2
Fabrice said he'd like Jonas to weigh in here.
Flags: needinfo?(jonas)
(In reply to Fabrice Desré [:fabrice] from comment #13) > Created attachment 727509 [details] [diff] [review] > gecko patch v2 > > Better patch for gecko, simpler and which doesn't need the gaia patch > anymore (except adding the new pages of course). Hi Fabrice, If I understood the patch, basically we need to setup in the preferences which url's can listen to the redirections and where to send the redirection back. We will need to modify the current code to align with this proposal, which I like a lot ;) But still have a question regarding how 3rd party packaged apps would be able to do oauth authentication. Cheers and thanks!!
Note I filed a PR to just use https:// for now... Not sure of the right process to get somebody to look at it. https://github.com/mozilla-b2g/gaia/pull/8790
(In reply to Francisco Jordano [:arcturus] (on PTO till 2nd April) from comment #16) > Hi Fabrice, > > If I understood the patch, basically we need to setup in the preferences > which url's can listen to the redirections and where to send the redirection > back. Yes, that's correct. > We will need to modify the current code to align with this proposal, which I > like a lot ;) > > But still have a question regarding how 3rd party packaged apps would be > able to do oauth authentication. That doesn't solve anything for 3rd party apps unfortunately. From what I read about oauth, it's not well enough defined to be sure that several services could use the same redirection URI, but I may be wrong.
Jonas asked me to comment. I'm wondering if a solution lies in Facebook's recommendation for "desktop apps", which is effectively what a packaged app is: https://developers.facebook.com/docs/howtos/login/login-for-desktop/ The key idea is that (a) you don't need an app secret, (b) the redirect URL is a special Facebook-hosted URL which the client app is expected to notice, and from which it can extract the access token located in the fragment identifier, specifically: https://www.facebook.com/connect/login_success.html# access_token=USER_ACCESS_TOKEN... Since there is no inherent need to prevent that SSL request from being made, only a need to eventually detect that URL and extract the fragment identifier, you might be able to get by with a less-invasive patch, and you definitely don't need a relay server.
Flags: needinfo?(jonas)
Detecting a redirect is actually what this patch does, along with relaying the parameters to the app:// url that we want to actually use.
(In reply to Fabrice Desré [:fabrice] from comment #20) > Detecting a redirect is actually what this patch does, along with relaying > the parameters to the app:// url that we want to actually use. OK, great, does special-casing just that Facebook URL make sense?
(In reply to Ben Adida [:benadida] from comment #21) > (In reply to Fabrice Desré [:fabrice] from comment #20) > > Detecting a redirect is actually what this patch does, along with relaying > > the parameters to the app:// url that we want to actually use. > > OK, great, does special-casing just that Facebook URL make sense? That I'm not sure, since we also have integration with other services like gmail and windows live that will need this kind of mechanism.
(In reply to Fabrice Desré [:fabrice] from comment #22) > That I'm not sure, since we also have integration with other services like > gmail and windows live that will need this kind of mechanism. Makes sense. Do we have a complete list of these services? I can dig back into their protocols and suggest approaches that limit the use of a network mediator.
Currently we support importing contacts from facebook, gmail and windows live. I don't know if more are planned. The relevant code is in the communications app in gaia: https://github.com/mozilla-b2g/gaia/tree/master/apps/communications
Great, so google OAuth supports a similar scheme to Facebook's: https://developers.google.com/accounts/docs/OAuth2InstalledApp#choosingredirecturi It looks to me like using the urn redirection will provide exactly the same hook as the Facebook suggestion: a special URL that doesn't require a new server, that can be detected by your patch and then directed back to the app. I'll keep looking for something similar from Microsoft.
Since unassigned tef+ bugs look weird and it looks like you're working on this, Fabrice, I'm assigning it to you. Feel free to unassign if I'm incorrect.
Assignee: nobody → fabrice
Attachment #727509 - Flags: feedback?(justin.lebar+bug)
Fabrice explained to me on IRC what's going on here. To explain what I understand: At the end of the oauth process, FB will redirect us to any http(s) site. But we want it to redirect us to an app:// site, because we don't want to rely on the http(s) site being up. We can't ask FB to redirect us directly to an app:// site, because FB doesn't support that scheme. And anyway even if we got them to change it, we'd have to ask other oauth providers to change their stuff as well, and that's not going to happen in general. So my suggestion over IRC was: It sounds like what apps want is a way to load app:// content with an http(s) url. It seems like we should be able to provide that in Gecko. If we did this, we wouldn't need any provider-specific hacks. To be concrete, suppose the app's manifest is http://foo.app.com/manifest. The app might specify in its manifest that all requests to http://foo.app.com/app_magic get transformed into app://foo/. Then the app could ask FB to redirect to http://foo.app.com/app_magic/oauth_complete.html, and we'd actually load app://foo/oauth_complete.html. We could require that the aliased URL is same-origin as the manifest URL. I think that would eliminate most of the security issues here.
Attachment #727509 - Flags: feedback?(justin.lebar+bug)
Hi all, I cannot explain the happiness that invades me when seeing the solution proposed here. Getting ride of the external server will be a win for the platform. With that said just have a couple of questions: - When we talk about transform a url into another one, we are talking about sending the headers as well isnt? - Also are we talking about having this solution for 1.0.1 or will be out of the scope? - Taking into account bug 852720 (allowing packaged apps to define an origin), will us provide a way of apps registering this redirection via manifest? Or is that too much to ask for :S Thanks a lot to everyone for the efforts!!
(In reply to Justin Lebar [:jlebar] from comment #27) > Fabrice explained to me on IRC what's going on here. To explain what I > understand: > > At the end of the oauth process, FB will redirect us to any http(s) site. > But we want it to redirect us to an app:// site, because we don't want to > rely on the http(s) site being up. > > We can't ask FB to redirect us directly to an app:// site, because FB > doesn't support that scheme. And anyway even if we got them to change it, > we'd have to ask other oauth providers to change their stuff as well, and > that's not going to happen in general. > > So my suggestion over IRC was: It sounds like what apps want is a way to > load app:// content with an http(s) url. It seems like we should be able to > provide that in Gecko. If we did this, we wouldn't need any > provider-specific hacks. > > To be concrete, suppose the app's manifest is http://foo.app.com/manifest. > The app might specify in its manifest that all requests to > http://foo.app.com/app_magic get transformed into app://foo/. Then the app > could ask FB to redirect to > http://foo.app.com/app_magic/oauth_complete.html, and we'd actually load > app://foo/oauth_complete.html. > > We could require that the aliased URL is same-origin as the manifest URL. I > think that would eliminate most of the security issues here. I am against this idea because it opens the door to DNS based attacks. As an attacker that can control your DNS, for example when you are on my WiFi access point, I would simply resolve foo.app.com to point to my own server and do all kinds of nasty stuff. I think a simpler solution would be if a mozbrowser iframe would be able to intercept and allow/deny requests. This is similar functionality that both iOS and Android have in their WebViews and it is super useful for integration like this and some other scenarios.
> I am against this idea because it opens the door to DNS based attacks. I don't understand what you mean. The point of this proposal is to avoid loading anything from the app. We never load anything from foo.app.com during the oauth flow, so we have no reason to ask DNS to resolve that domain. > I think a simpler solution would be if a mozbrowser iframe would be able to intercept and allow/deny > requests. We could do this, but then the mozbrowser embedder (that is, Gaia) would have to know which requests to deny and redirect. As far as I could tell, this required having a hardcoded list of oauth providers in Gaia.
(In reply to Justin Lebar [:jlebar] from comment #30) > > I am against this idea because it opens the door to DNS based attacks. > > I don't understand what you mean. The point of this proposal is to avoid > loading anything from the app. We never load anything from foo.app.com > during the oauth flow, so we have no reason to ask DNS to resolve that > domain. Ok maybe I misunderstood this. What would be the implication of aliasing https://twitter.com to app://twitter.com? Would it mean that the real https://twitter.com would become unreachable? > > I think a simpler solution would be if a mozbrowser iframe would be able to intercept and allow/deny > > requests. > > We could do this, but then the mozbrowser embedder (that is, Gaia) would > have to know which requests to deny and redirect. As far as I could tell, > this required having a hardcoded list of oauth providers in Gaia. No it would simply be an addition to the BrowserAPI https://wiki.mozilla.org/WebAPI/BrowserAPI - Like an extra event handler that would fire before loading content and would give the *owner* of that mozbrowser iframe a change to cancel it. No global list is required.
> What would be the implication of aliasing https://twitter.com to app://twitter.com? Would it mean > that the real https://twitter.com would become unreachable? In my model, yes. > Like an extra event handler that would fire before loading content and would give the *owner* of > that mozbrowser iframe a change to cancel it. Gaia is the owner (our jargon is "embedder") of the mozbrowser iframe. How does Gaia know, given a requested URL, whether to cancel/redirect the request without a list of some sort? I don't think we could consider the window which started the oauth flow to be the "owner" of the window which contains the oauth flow, if that's what you mean. /That/ would open up some interesting security issues.
(In reply to Justin Lebar [:jlebar] from comment #27) > To be concrete, suppose the app's manifest is http://foo.app.com/manifest. > The app might specify in its manifest that all requests to > http://foo.app.com/app_magic get transformed into app://foo/. Then the app > could ask FB to redirect to > http://foo.app.com/app_magic/oauth_complete.html, and we'd actually load > app://foo/oauth_complete.html. > > We could require that the aliased URL is same-origin as the manifest URL. I > think that would eliminate most of the security issues here. Rewriting all requests to http://foo.app.com/app_magic may potentially allow a device to be fingerprinted. A malicious site could make requests for http://foo.app.com/well_known_file and look for differences between the install vs uninstalled app page. Same origin prevents most introspection by the malicious page. Sourcing a script file and comparing window variables should work though. I'm also curious as to how the rewrite would interact with extended principals. Would http://foo.app.com have the principal of foo.app.com, the packaged app GUID or something else? What about if foo.app.com is a packaged browser app? I think the frames within the browser app should be okay since they have inBrowserElement set.
I'll restate what I mentioned above because it seems it got lost in the ensuing conversation: Facebook provides a mechanism for this already. You can request that the redirect URL be a special URL *at Facebook*, with the OAuth token placed in the fragment identifier. The client's job is to detect this special URL, extract the token, and voila. This is very similar to Google's approach, which indicates to me that this is a pattern that's emerging from OAuth providers: redirect to a special URL that only a full web client with chrome privileges (not just content) can access.
(In reply to Stefan Arentz [:st3fan] from comment #31) > > We could do this, but then the mozbrowser embedder (that is, Gaia) would > > have to know which requests to deny and redirect. As far as I could tell, > > this required having a hardcoded list of oauth providers in Gaia. > > No it would simply be an addition to the BrowserAPI > https://wiki.mozilla.org/WebAPI/BrowserAPI - Like an extra event handler > that would fire before loading content and would give the *owner* of that > mozbrowser iframe a change to cancel it. No global list is required. That would work, but just for privileged and certified apps and it would leave 'normal' apps out of luck. I have a proposal similar to what's been thrown around by Fabrice and Justin. Instead of allowing a one-on-one redirect from a fake URL to an app URL (so each fake URL matches one app URL), can't we just define ONE loopback URL that just strips the protocol, host (and port) and loads the rest of the URL as a relative URL to the app root. With an example. Let's assume we define the loopback URL as: https://loopback.ffos This URL is a constant (can be defined on a pref). So, as an app developer, when I want Facebook or whatever to load the url /here/goes/my/oauth.html on my app, what I do is say Facebook to load: https://loopback.ffos/here/goes/my/oauth.html and what Gecko should do is load app://whateverTheLocalAppIs/here/goes/my/oauth.html. If we can do this, it should work for all kind of apps, it doesn't introduce any new risk that I can see (since an evil app developer cannot capture any real page URL that way) and it doesn't require adding new configuration options for each app. Hmm I lied, there's one risk I can see here, come to think about it: a malicious web app could use redirects to https://loopback.ffos/url to load random app pages. But that can be easily fixed by introducing a 'loopbackAllowedFrom' property on the app manifest: loopbackAllowedFrom: [ 'https://www.facebook.com' ] or something like that.
> You can request that the redirect URL be a special URL *at Facebook*, with the OAuth token placed in > the fragment identifier. In order for this to work, Gaia would need to be able to recognize when the oauth frame loaded this special URL. Fabrice solves this problem in his patch by hardcoding this URL into Gecko. The essential problem I was trying to work around with my proposal was the need to do this. Do you have an alternative way to avoid hardcoding these URLs?
> Let's assume we define the loopback URL as: > > https://loopback.ffos There is no guarantee that all OAuth providers will let you provide "https://loopback.ffos" as a redirect URL, because it's not a valid URL today (.ffos is not a TLD). With the changes ICANN is making, perhaps it will be in the future, which causes its own set of problems! You could imagine using a valid URL like https://mozilla.org/ffos/loopback or something like that, but I think using a URL associated with the app is probably better, since then we don't bake into the spec a reliance on any one party to maintain control of a particular domain.
(In reply to Justin Lebar [:jlebar] from comment #32) > > What would be the implication of aliasing https://twitter.com to app://twitter.com? Would it mean > > that the real https://twitter.com would become unreachable? > > In my model, yes. > > > Like an extra event handler that would fire before loading content and would give the *owner* of > > that mozbrowser iframe a change to cancel it. > > Gaia is the owner (our jargon is "embedder") of the mozbrowser iframe. How > does Gaia know, given a requested URL, whether to cancel/redirect the > request without a list of some sort? > > I don't think we could consider the window which started the oauth flow to > be the "owner" of the window which contains the oauth flow, if that's what > you mean. /That/ would open up some interesting security issues. I don't understand this comment. The facebook auth stuff runs in an app. In case of B2G it runs inside the communications app. The communications app is responsible for opening and owning the mozbrowser iframe in which the facebook oauth runs. So the app can install these handlers on that iframe. Then when facebook is done and returns the redirect, the communications app can intercept it and deal with it. This is really not a system issue.
(In reply to Justin Lebar [:jlebar] from comment #37) > > Let's assume we define the loopback URL as: > > > > https://loopback.ffos > > There is no guarantee that all OAuth providers will let you provide > "https://loopback.ffos" as a redirect URL, because it's not a valid URL > today (.ffos is not a TLD). With the changes ICANN is making, perhaps it > will be in the future, which causes its own set of problems! > > You could imagine using a valid URL like https://mozilla.org/ffos/loopback > or something like that, but I think using a URL associated with the app is > probably better, since then we don't bake into the spec a reliance on any > one party to maintain control of a particular domain. The idea behind using a fake URL is that it doesn't matter if someone registers it or not... since it won't be loaded from the net ever. It is a loopback address, and it works the same way as 127.0.0.1. Which BTW was my first idea for this special URL :) https://127.0.0.1/myURL Only I didn't think that would work for most (if any) commercial OAuth providers would let you enter that. By the way, do Facebook actually check that the URL is a valid, Internet reachable address? Cause that would mean that I can't make Intranet-only oauth apps either (redirecting to an internal URL).
> The communications app is responsible for opening and owning the mozbrowser iframe in which the > facebook oauth runs. Ah, I'd asked Fabrice, and he said that FB oauth was opened via window.open from within the app. If the oauth is instead run inside a mozbrowser created the app itself, then that completely changes things. The app can observe mozbrowserlocationchange and do exactly what Ben suggests in comment 34, with zero changes to the platform. > The idea behind using a fake URL is that it doesn't matter if someone registers it or not I understand, but I don't think it's OK for us to write a spec which takes a legitimate URL and says "nobody can ever access this URL from within an app."
(In reply to Antonio Manuel Amaya Calvo from comment #39) > > By the way, do Facebook actually check that the URL is a valid, Internet > reachable address? Cause that would mean that I can't make Intranet-only > oauth apps either (redirecting to an internal URL). This will depend on the service provider. Some services check if the address is reachable, but, AFAIK, is something that is not standardised. F.
(In reply to Francisco Jordano [:arcturus] from comment #41) > (In reply to Antonio Manuel Amaya Calvo from comment #39) > > > > By the way, do Facebook actually check that the URL is a valid, Internet > > reachable address? Cause that would mean that I can't make Intranet-only > > oauth apps either (redirecting to an internal URL). > > This will depend on the service provider. Some services check if the address > is reachable, but, AFAIK, is something that is not standardised. > > F. I've been checking and it seems that twitter and Instagram (according to google searches) allow either localhost or 127.0.0.1 as the URL. And that would works almost as any dev would expect (except for the change of protocol): it would load a local page.
(In reply to Justin Lebar [:jlebar] from comment #37) > You could imagine using a valid URL like https://mozilla.org/ffos/loopback > or something like that, but I think using a URL associated with the app is > probably better, since then we don't bake into the spec a reliance on any > one party to maintain control of a particular domain. This might create same-origin issues on the Facebook side if multiple (different) apps use the same domain for their oauth callbacks. I don't think we can assume that is not going to cause troubles. We need a more structural solution. Ideally something that has nothing to do with OAuth specifically but more with how applications can interact with content that they load inside mozbrowser iframes. S.
Which same-origin issues? And the browser API is privileged only. And on a privileged app I can do the oauth flows currently by using sustenxhr
(In reply to Antonio Manuel Amaya Calvo from comment #44) > Which same-origin issues? > > And the browser API is privileged only. And on a privileged app I can do the > oauth flows currently by using sustenxhr There is no guarantee that all OAuth providers deal well with multiple apps from possibly different vendors using the same callback host.
Hmm that would be a very particular read of the standard. That parameter is there to protect against third parties, not against the app developer or the user. That's why even localhost is a valid (if a little bit particular) value. Oh, btw, allowing redirecting localhost (for example) to app://currentapp doesn't work only for oauth. In fact it would allow any app developer to load something from his app to his server and back, safely.
(In reply to Justin Lebar [:jlebar] from comment #40) > > The communications app is responsible for opening and owning the mozbrowser iframe in which the > > facebook oauth runs. > > Ah, I'd asked Fabrice, and he said that FB oauth was opened via window.open > from within the app. > > If the oauth is instead run inside a mozbrowser created the app itself, then > that completely changes things. The app can observe > mozbrowserlocationchange and do exactly what Ben suggests in comment 34, > with zero changes to the platform. I don't like that particular solution as we will need to duplicate all the logic and UI that is already in place for window.open. My proposal here is to provide a special kind of window that will manage the OAuth redirections as per a white list passed as parameter. Using this approach we will enable any kind of app (with the corresponding permission) to implement seamlessly OAuth flows. window.open('xxx','oauth, redirectURI=redirectURI')
(In reply to Jose M. Cantera from comment #47) > (In reply to Justin Lebar [:jlebar] from comment #40) > > > The communications app is responsible for opening and owning the mozbrowser iframe in which the > > > facebook oauth runs. > > > > Ah, I'd asked Fabrice, and he said that FB oauth was opened via window.open > > from within the app. > > > > If the oauth is instead run inside a mozbrowser created the app itself, then > > that completely changes things. The app can observe > > mozbrowserlocationchange and do exactly what Ben suggests in comment 34, > > with zero changes to the platform. > > I don't like that particular solution as we will need to duplicate all the > logic and UI that is already in place for window.open. My proposal here is > to provide a special kind of window that will manage the OAuth redirections > as per a white list passed as parameter. Using this approach we will enable > any kind of app (with the corresponding permission) to implement seamlessly > OAuth flows. > > window.open('xxx','oauth, redirectURI=redirectURI') Wouldn't be enough to just have w=window.open('xxx','mozbrowser') that opens a window like window.open but the object returned implements the Browser API? (that is, it's an <iframe mozbrowser> instead of a plain <iframe))
window.open(..., 'mozbrowser') is problematic. window.open() returns a window. But the window inside a mozbrowser is not always accessible, because a mozbrowser may run inside a different process than its parent. So window.open would have to return null, which would not be useful. Even if it was technically feasible to hack window.open('mozbrowser') to return an iframe instead of a window, I think that would be a bad idea because it would be inconsistent and expose implementation details about mozbrowser.
(In reply to Justin Lebar [:jlebar] from comment #49) > window.open(..., 'mozbrowser') is problematic. > > window.open() returns a window. But the window inside a mozbrowser is not > always accessible, because a mozbrowser may run inside a different process > than its parent. So window.open would have to return null, which would not > be useful. Even if it was technically feasible to hack > window.open('mozbrowser') to return an iframe instead of a window, I think > that would be a bad idea because it would be inconsistent and expose > implementation details about mozbrowser. yes, that's a good reason to support my proposal: var w = window.open('xxx','oauth, redirectURI=redirectURI') w.onoauthredirect = function() { }
> yes, that's a good reason to support my proposal: > > var w = window.open('xxx','oauth, redirectURI=redirectURI') > w.onoauthredirect = function() { > } Except that this gives pages the ability to redirect an arbitrary page to their content, which is not safe. I think the proposal from comment 27 is better, because it's much safer. The main objection to this proposal was "you should use mozbrowser instead," but if using mozbrowser is not acceptable or possible, then this is an alternative. I kind of agree that using mozbrowser for this use-case is heavy-handed.
(In reply to Justin Lebar [:jlebar] from comment #51) > > yes, that's a good reason to support my proposal: > > > > var w = window.open('xxx','oauth, redirectURI=redirectURI') > > w.onoauthredirect = function() { > > } > > Except that this gives pages the ability to redirect an arbitrary page to > their content, which is not safe. > Yes, that's why I'm proposing to add a new permission to allow an app to do that only if that permission is given, i.e. that would only be allowed for certified and trusted content > I think the proposal from comment 27 is better, because it's much safer. > The main objection to this proposal was "you should use mozbrowser instead," > but if using mozbrowser is not acceptable or possible, then this is an > alternative. I kind of agree that using mozbrowser for this use-case is > heavy-handed.
Our solution to everything in B2G these days seems to be "add a permission, make it available only to trusted content". Sometimes that's necessary, but it essentially fragments the Web, so it's bad. If we have a solution that's safer, I think we should do that.
(In reply to Justin Lebar [:jlebar] from comment #53) > Our solution to everything in B2G these days seems to be "add a permission, > make it available only to trusted content". Sometimes that's necessary, but > it essentially fragments the Web, so it's bad. If we have a solution that's > safer, I think we should do that. if a solution allows us to make a quick and safe delivery we should go for that solution, IMHO. We should not forget that this functionality is needed for v1.0.1 and the proposed solution in comment #50 would have a minimal impact in the v1.0.1 code
I think the faster way to fix this would be just to replace the window.open with the <iframe mozbrowser>, although it has the problem of either having to reimplement the window dressing that window.open currently does (the location bar basically) or giving less perceived security/trust. That would not require any change on the platform nor on any other part of gaia beside the apps that use oauth. Still, I fail to see how the fact that window.open returns a window is a problem since it's currently returning a window which is really an iframe. Are all iframe mozbrowser OOPs? Even if it is, the parent has to get an object for the iframe somehow or it would not be able to set event managers on it, can it? Wouldn't it be possible just to add the mozbrowser option checker to window_manager.js on Gaia? If I'm not mistaken that's the one actually doing the opening of the 'windows', is it not? I don't like much Jose Manuel's approach because it's too ad hoc.
Fabrice, any thoughts on the two approaches?
(In reply to Justin Lebar [:jlebar] from comment #40) > > The communications app is responsible for opening and owning the mozbrowser iframe in which the > > facebook oauth runs. > > Ah, I'd asked Fabrice, and he said that FB oauth was opened via window.open > from within the app. I checked the contacts app code, and it's using window.open() to start the oauth flow (at https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/oauth2/js/flow.js#L93) That let us with a gecko side solution. I'll write a new patch using a manifest-based declaration of redirects. One of the issues to implement Justin's idea from comment 27 is that gaia's apps have no http facing manifest urls (they have app:// manifest urls) so that may not be that clean.
Fabrice, is the manifest-declared resirect going to be restricted just to certified apps? I think the same problems that Justin raised against using a fixed, fake, host for redirects apply here. What will happen if or when the app developer lose control of the redirected domain? Since https://localhost is a valid URL for redirects according to the standard and it's in fact supported, I still think that it would be cleaner to just allow manifests to specify something like: allowLocalhostRedirects: true to mean they want http[s]://localhost to redirect to app://currentapp.
Oh, also for v1.0.1 this can be fixed without any gecko changes by changing the window.open to an iframe mozbrowser, can't it?
(In reply to Antonio Manuel Amaya Calvo from comment #59) > Oh, also for v1.0.1 this can be fixed without any gecko changes by changing > the window.open to an iframe mozbrowser, can't it? As you comment 55, my main concern about this is having to mock not just the location bar but basically the whole window (allowing the user to cancel/close the action and so on). Don't like this approach at all cause: - Using a 3rd party credentials is quite sensitive for users, so will prefer to use the standard tools from the phone, giving the user the feedback that he is visiting the original site. - Doing this on the gaia side will fix this just for the original problem (contacts integration with fb, google and hotmail), but if we think about 3rd party developers, asking them to mimic this doesn't seem a good experience. The idea of using a manifest based solution (perhaps restricting it to privileged apps) sounds good to me, and we could kill two birds with a stone (our current problem and having a platform that is oauth friendly for devs). Also regarding the dns based attack, perhaps I'm wrong, but that can also happen on a desktop browser isn't? Cheers! F.
As I keep trying to emphasize, none of us can guarantee that every single oauth provider will allow http[s]://localhost. It's a valid URL, but an oauth provider might filter it, because it's an odd URL to provide. For example, the oauth provider might require the URL to end with one of the known TLDs (.com, .org, etc). I think using localhost is painting us into a corner, and I emphasize that no amount of testing can prove that it will work on all providers now and in the future. > What will happen if or when the app developer lose control of the redirected domain? Then the app developer can change the manifest.
(In reply to Justin Lebar [:jlebar] from comment #61) > As I keep trying to emphasize, none of us can guarantee that every single > oauth provider will allow http[s]://localhost. It's a valid URL, but an > oauth provider might filter it, because it's an odd URL to provide. For > example, the oauth provider might require the URL to end with one of the > known TLDs (.com, .org, etc). > > I think using localhost is painting us into a corner, and I emphasize that > no amount of testing can prove that it will work on all providers now and in > the future. Actually providers can randomly restrict anything they wish. No amount of testing can prove that any given domain will work on all providers. The best we can do is go by the standard, and by something that's currently enforced or supported by the biggest providers. And even that isn't something stable since today Facebook support .org (for example) and tomorrow they might decide they're going to support only .com, or .facebook, or .whatever, it's their call to make. And I don't think we should design based on what they could or could not do that isn't on the standard. > > > What will happen if or when the app developer lose control of the redirected domain? > > Then the app developer can change the manifest. And why would he do that? He loses nothing by leaving it as it... the only thing that will happen will be that the user will not be able to access something that is now a valid URL (as opposed to a fake-ish one use to internally redirect to the app). Which is exactly the same problem he would have if we used localhost.mozilla.org, or localhost.gaiamobile.org, or something like that for ALL apps instead of having a per-app redirect and at some point down the line Mozilla loses control of that domain. Which, again, could be solved with a simple OS patch... So I don't see what we win by allowing the app to specify what URLs are redirectable, or having to specify it on a preference. And I see what we lose though: the ability for this to work for all kind of apps, and not just certified and (maybe, depending on how the redirect is specified) privileged ones. I haven't seen any other proposal here that would work securely for not reviewed apps.
> Actually providers can randomly restrict anything they wish. It's true that providers can randomly restrict anything they wish. I think it's also true that providers are more likely to restrict localhost than they are to restrict https://foo.org. > The best we can do is go by the standard Does the OAuth standard explicitly say that localhost is allowed? Francisco said in comment 41 that the criteria for acceptable URIs is not standardized. I think we're making this much harder than it is.
Attached patch gecko patch v3 (obsolete) — — Splinter Review
Another try at making progress... Since I don't think we can reasonably know what the providers allow/disallow and how this can fire back at us if restrict too much what we can do, I choose to allow redirections to be declared in the manifests like this: diff --git a/apps/communications/manifest.webapp b/apps/communications/manifest.webapp index 529b0bb..d98ae19 100644 --- a/apps/communications/manifest.webapp +++ b/apps/communications/manifest.webapp @@ -135,5 +135,9 @@ { "notification": "/dialer/index.html#keyboard-view" }, { "telephony-new-call": "/dialer/index.html#keyboard-view" }, { "ussd-received": "/dialer/index.html#keyboard-view" } + ], + "redirects": [ + { "from": "http://intense-tundra-4122.herokuapp.com/fbowd/oauth2_new/flow.html", + "to": "/contacts/oauth2/flow.html" } ] } The "from" property is the prefix we want to redirect. The "to" property is relative to the app origin. Redirections app app-specific, which means that even if an app decides to redirect http://foo.org/bar.html to an internal URI, loading http://foo.org/bar.html in the browser or any other app won't result in a redirect. I think this gives us enough protection.
Attachment #727509 - Attachment is obsolete: true
(In reply to Fabrice Desré [:fabrice] from comment #64) > Created attachment 736005 [details] [diff] [review] > Redirections app app-specific, which means that even if an app decides to > redirect http://foo.org/bar.html to an internal URI, loading > http://foo.org/bar.html in the browser or any other app won't result in a > redirect. I think this gives us enough protection. Hmm no it doesn't. If we go this way then this has to be restricted to certified and privileged apps (optionally with an associated permission so it's consistent with the rest of the OS). Otherwise a random app could do: "redirects": [ { "from": "https://twitter.com/sessions", "to": "/shamelessPirate/soLongAndThanksForYourPassword.html" } ] } And show inside of the app the twitter logging page (for oauth or whatever). A cautious user could check that the app was indeed using OAuth for the loging page (and we would tell him the page shown is indeed https://twitter.com). But when he hits the login button we'll happily pass his credentials to the app to do with then whatever it wishes. Either we restrict the valid values for 'from' or we restrict the app types that can use it, I don't think we can safely allow any unverified app to redirect the page(s) it chooses to to itself.
Nothing prevents an app to mock the twitter login page in its UI, even unprivileged ones. I'm sure that most users won't understand/realize what's going on. Anyway I'm fine with restricting redirects to privileged or certified apps.
(In reply to Fabrice Desré [:fabrice] from comment #66) > Nothing prevents an app to mock the twitter login page in its UI, even > unprivileged ones. I'm sure that most users won't understand/realize what's > going on. Yep, I agree completely on the 'most users' part. And OAuth for those users is a act of faith anyway. But the problem with this is we're fooling the rest of the users also. And defeating any kind of countermeasure that a service provider could include to ensure the 'authenticity' of the page (like showing the profile photo of the user as Facebook does when you enter the email address correctly with an incorrect password on a computer where previously you have authenticated correctly). Whatever the service provides add as an aid so non-technical users can know they're on the correct place goes down the drain with this. > > Anyway I'm fine with restricting redirects to privileged or certified apps. That'll be great for this version, thanks.
Attached patch patch v4 (obsolete) — — Splinter Review
Now with redirections limited to privileged and packaged apps. Justin, can you review the dom/browser-element parts ? and Fernando the Webapps.jsm ones?
Attachment #736005 - Attachment is obsolete: true
Attachment #736425 - Flags: review?(justin.lebar+bug)
Attachment #736425 - Flags: review?(ferjmoreno)
Keywords: dev-doc-needed
Comment on attachment 736425 [details] [diff] [review] patch v4 Review of attachment 736425 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks Fabrice! ::: dom/apps/src/AppsUtils.jsm @@ +90,5 @@ > packageHash: aApp.packageHash, > staged: aApp.staged, > installerAppId: aApp.installerAppId || Ci.nsIScriptSecurityManager.NO_APP_ID, > + installerIsBrowser: !!aApp.installerIsBrowser, > + redirects: aApp.redirects Can we add a check for the 'redirects.to' parameter so only relative URLs are allowed? ::: dom/apps/src/Webapps.jsm @@ +1292,5 @@ > debug("updateAppHandlers: old=" + aOldManifest + " new=" + aNewManifest); > this.notifyAppsRegistryStart(); > + if (aApp.appStatus >= Ci.nsIPrincipal.APP_STATUS_PRIVILEGED) { > + aApp.redirects = aNewManifest.redirects; > + } nit: update the comment also, please.
Attachment #736425 - Flags: review?(ferjmoreno) → review+
Hi, just compiled the patch and tried. The redirection works as expected, great job Fabrice! Then decided to do a simple modification to use our current code with this new redirection mechanism. You can find it here: https://github.com/arcturus/gaia/tree/simplified-oaut-redirect In our process we do a window.open, go to the service, log in, and when the service redirects to the callback url, we hit our app (the document specified in the manifest), fetch the parameters (the access token) and we do a postMessage to the window opener to finally close the popup. Well there is something (sorry for not being able to identify what is it) that crash when we try to do the postMessage to the parent window. I'm attaching the log that I got from gecko. Despite the proper redirection works perfectly so we are able to handle the access token from the app itself, so now is a matter or either fixing why that postMessage is not working, or just use another way to pass the access token to the opener (that is the app itself :)) Thanks a lot for your work!
Francisco, I checked postMessage() and everything is fine there. I added some log in oauth20.js, we fail because of the origin change: E/GeckoConsole(10499): Content JS LOG at app://communications.gaiamobile.org/contacts/oauth2/js/oauth20.js:102 in tokenDataReady: {"access_token":"BAAEmVyLJ6WYBAKEzr1bpBp1enuHiuD3fjfpVZA8Fm8HAmPZBmKXn2n3M0CmrE8rSfmGMUUbg5E9ke8lbqTEfXtDmirkdzACuZCSNZBlAE346QjLp6uCv","expires_in":"5172403","state":"friends"} E/GeckoConsole(10499): Content JS LOG at app://communications.gaiamobile.org/contacts/oauth2/js/oauth20.js:110 in tokenDataReady: allowedOrigin=http://intense-tundra-4122.herokuapp.com origin=app://communications.gaiamobile.org I'm confident this is easy to fix!
Is it possible to get a mochitest here?
> + let app = DOMApplicationRegistry.getAppByLocalId(appId); When we were profiling app startup, I recall that we established that this was an expensive sync method that shouldn't be on the critical path. Did we make it faster somehow? Otherwise I'm kind of afraid of doing this on every load. Given the choice between 1. letting the app redirect any URL, but then exposing this functionality only to privileged and certified apps, and 2. letting the app redirect same-origin URLs, and exposing this functionality to all apps, shouldn't we go with (2)? We can talk IRL about this if you'd like. I'd also like to see a mochitest for this. We have a pretty good framework set up for mozbrowser mochitests, in dom/browser-element/mochitest. I'm not sure that loadstart is the right event to implement a redirect at. We may have already started network requests by then. But I'm not sure. We should find bz and ask.
Attachment #736425 - Flags: review?(justin.lebar+bug)
(In reply to Justin Lebar [:jlebar] from comment #73) > > + let app = DOMApplicationRegistry.getAppByLocalId(appId); > > When we were profiling app startup, I recall that we established that this > was an expensive sync method that shouldn't be on the critical path. Did we > make it faster somehow? Otherwise I'm kind of afraid of doing this on every > load. We optimized this when doing the startup improvements. It's not that bad anymore. > Given the choice between > > 1. letting the app redirect any URL, but then exposing this functionality > only to privileged and certified apps, and > > 2. letting the app redirect same-origin URLs, and exposing this > functionality to all apps, > > shouldn't we go with (2)? We can talk IRL about this if you'd like. I'm not sure what same-origin means there, but let's discuss this IRL. > I'd also like to see a mochitest for this. We have a pretty good framework > set up for mozbrowser mochitests, in dom/browser-element/mochitest. sure, I will add some. > I'm not sure that loadstart is the right event to implement a redirect at. > We may have already started network requests by then. But I'm not sure. We > should find bz and ask. ok.
(In reply to Justin Lebar [:jlebar] from comment #73) > > + let app = DOMApplicationRegistry.getAppByLocalId(appId); > > When we were profiling app startup, I recall that we established that this > was an expensive sync method that shouldn't be on the critical path. Did we > make it faster somehow? Otherwise I'm kind of afraid of doing this on every > load. > > Given the choice between > > 1. letting the app redirect any URL, but then exposing this functionality > only to privileged and certified apps, and > > 2. letting the app redirect same-origin URLs, and exposing this > functionality to all apps, > > shouldn't we go with (2)? We can talk IRL about this if you'd like. "same-origin" for unsigned apps is a fuzzy concept at the moment. And something kinda hairy to be included/fixed/considered/secured on the 1.0.1 time frame, and this bug is a tef+ blocker. Anything that's just included on the manifest for an unreviewed/unsigned app cannot be trusted at all. And even using the install_from as an origin has some problems (what happens when installing apps from a server I don't actually control? Installing from dropbox or any other file server machine?)
Thanks Fabrice, as you comment was a simple fix in the gaia side. I'm attaching a PR with the changes we will need in gaia to use this patch. I've tried it and works perfectly (for the 3 services, facebook, gmail and hotmail). Asking Jose Manuel for feedback on the gaia side. Also will need a look from Vivien cause we needed to modify the popup manager in the system to check for some null references. Cheers!
Attachment #736921 - Attachment is obsolete: true
Attachment #737446 - Flags: feedback?(jmcf)
Comment on attachment 737446 [details] Gaia modifications to use this patch positive feedback, provided the comments in GH are satisfied
Attachment #737446 - Flags: feedback?(jmcf) → feedback+
Whiteboard: [status: has review, maybe needs final gaia patch?]
I r-cleared'ed this patch earlier; see comment 73. We established that we are going to stay with the current design wrt security, but there are still kinks to work out here.
Whiteboard: [status: has review, maybe needs final gaia patch?] → [status: waiting for new Gecko patch]
Whiteboard: [status: waiting for new Gecko patch] → [status: waiting for new Gecko patch][madrid]
Target Milestone: --- → Madrid (19apr)
No material updates in a couple of days. What needs to happen next to fix this?
> No material updates in a couple of days. I think Fabrice has been at a work-week or something...
Should this bug be Security-Sensitive? I don't think it should?
Hi, I've been invited into this bug because I'm facing the same problem (as a 3rd party developer) I'm using the Foursquare API in a privileged app and the API require an oauth_token. I'looked at the discussion and here are some comments on this :) Most part of the Foursquare API are working without systemXHR so I started developing an hosted app. On an hosted app, I have an origin so I didn't have a problem with this. That's why I think it's okay to allow this only for privileged and certified app (comment 65). The only app that would not be able to do oauth would be packaged app without type: privileged or type: certified (I'm not sure who would be concerned: surely an hosted app with app cache is more comfortable to work with) An alternative might be to not include headers (especially cookies) to the app page. I don't have much knowledge of how oauth provider transmit the token. is it always on the url fragment? if yes that could be enough for oauth purpose I also prefer to use https://my.domain.tld than https://localhost as a redirect url because that allow me to ship the same code in my hosted webapp and my firefox packaged app hs: I didn't thought about postMessage to talk from the redirect page to the main one. I'm currently storing the token in localStorage and then caling window.close() in the redirected page and in the origin page i have an ugly setInterval checking if the popup is still open...
FWIW, is it worth splitting out this bug separately for the Gaia and Gecko work so we can land and test each part independently of one another? I'll likely be testing the gecko changes, while someone from TEF will be testing the Gaia pieces here.
Group: b2g-core-security
Marking a target deadline of 5/10, since we want more than one round of certification with this change.
Target Milestone: 1.0.1 Madrid (19apr) → 1.0.1 IOT1 (10may)
What's blocking landing this?
Fabrice needs to write a new Gecko patch. IRL he told me that he spoke with bz and they decided that mozbrowserloadstart is not the right point in time to do a redirect. There may be other things that need to be fixed up here, but off the top of my head that's what I remember.
I also talked with fabrice about how this needs ability to redirect from an https:// uri (which facebook gives us) to an app:// uri. Pointed him at bug 765934 which added the redirectTo() api (not on b2g18 branch but easy to uplift), but last I heard he was running into CSP policy vetoing the redirect.
It's actually not the CSP vetoing the redirect, but by the check in https://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#1313 since the app:// protocol has the DANGEROUS_TO_LOAD flag. I'm not sure if we can change these flags to something that let us redirect while not causing security issues.
In the mean time, can we *please* just change the current URL to use https:// ? It's obviously an incremental step towards getting rid of Heroku completely, but it's better than what we have right now. I have this PR to do it, but not sure what the right steps are: https://github.com/mozilla-b2g/gaia/pull/8790
(In reply to Fabrice Desré [:fabrice] from comment #88) > It's actually not the CSP vetoing the redirect, but by the check in > https://mxr.mozilla.org/mozilla-central/source/caps/src/ > nsScriptSecurityManager.cpp#1313 since the app:// protocol has the > DANGEROUS_TO_LOAD flag. > I'm not sure if we can change these flags to something that let us redirect > while not causing security issues. Yep, that was introduced in bug 773886 IIRC along with forbidding to load resources from other apps unless you get the app.manage permission (or whatever is called). And I don't think we can safely just remove that. I had assumed that what you were introducing would just add an special case or override that validation completely for the 'internal' redirects.
Talked to fabrice--plan is to allow nsIScriptSecurityManager.checkLoadURIWithPrincipal() to not fail if it's given an app:// URI that maps to the appID of the principal passed to it. AFAICT this shouldn't create any security hole, but maybe someone who knows nsIScriptSecurityManager can fill me in. (The description for checkLoadURIWithPrincipal says "Check that content with principal aPrincipal can load "uri". And an app should be able to load its own app:// uris...)
bholley thinks plan in comment 91 may work, but wants to see patch. Suggests that mounir and/or sicking may have opinions too.
That doesn't work, because facebook is loaded in window.open() and thus get an appId of 0.
We need somebody who can figure out how to connect the dots here. We know at some level which app is trying to do the oauth, and we need to allow https->app:// redirects if the app:// belongs to that app. I don't know the window/docshell/etc code well enough to know the best way to do that. Ideas? fabrice mentioned tryingt to get the docshell in the on-modify-request observer, but it's not present on the parent obviously, and we currently don't fire OMR on the child. We could change that, but for various reasons bsmith has been encouraging us not to fire OMR on the child. But if it's the best way forward here, I can do it.
Flags: needinfo?(bzbarsky)
I think I'm missing a lot of context here, but.... In CheckLoadURI we have a principal. Does that not have an app id associated with it? I'm not sure what the exact situation comment 93 is about is and how it fits in with the rest of this stuff...
Flags: needinfo?(bzbarsky)
I checked again the app id and I was wrong in comment 93. CheckLoadURIWithPrincipal in the child is always called with the correct app id. It's only in the parent that we get an app id of NO_APP_ID. Here's a backtrace (in the child) when we check the url that we would like to redirect: (gdb) bt #0 nsScriptSecurityManager::CheckLoadURIWithPrincipal (this=0x4218b6a0, aPrincipal=0x44281ec0, aTargetURI=0x44b9ced0, aFlags=9) at /home/fabrice/dev/birch/caps/src/nsScriptSecurityManager.cpp:1259 #1 0x40900fbe in nsScriptSecurityManager::AsyncOnChannelRedirect (this=0x4218b6a0, oldChannel=0x4430cc50, newChannel=<value optimized out>, redirFlags=<value optimized out>, cb=0x42fa9ca4) at /home/fabrice/dev/birch/caps/src/nsScriptSecurityManager.cpp:2359 #2 0x404696f8 in nsAsyncRedirectVerifyHelper::DelegateOnChannelRedirect (this=0x42fa9ca0, sink=0x4218b6a4, oldChannel=0x4430cc50, newChannel=0x4543c450, flags=1) at /home/fabrice/dev/birch/netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp:153 #3 0x404712fc in nsIOService::AsyncOnChannelRedirect (this=0x42107400, oldChan=0x4430cc50, newChan=0x4543c450, flags=1, helper=0x42fa9ca0) at /home/fabrice/dev/birch/netwerk/base/src/nsIOService.cpp:323 #4 0x40469894 in nsAsyncRedirectVerifyHelper::Run (this=0x42fa9ca0) at /home/fabrice/dev/birch/netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp:236 #5 0x40db81d2 in nsThread::ProcessNextEvent (this=0x421068e0, mayWait=<value optimized out>, result=0xbeea6007) at /home/fabrice/dev/birch/xpcom/threads/nsThread.cpp:627 #6 0x40d986d6 in NS_ProcessNextEvent (thread=0x1, mayWait=false) at /home/fabrice/dev/b2g/B2G/objdir-gecko/xpcom/build/nsThreadUtils.cpp:238 #7 0x40b904ac in mozilla::ipc::MessagePump::Run (this=0x42102340, aDelegate=0xbeea6910) at /home/fabrice/dev/birch/ipc/glue/MessagePump.cpp:82 #8 0x40b9055e in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x42102340, aDelegate=0xbeea6910) at /home/fabrice/dev/birch/ipc/glue/MessagePump.cpp:231 #9 0x40de63bc in MessageLoop::RunInternal (this=0x1000000) at /home/fabrice/dev/birch/ipc/chromium/src/base/message_loop.cc:219 #10 0x40de6472 in MessageLoop::RunHandler (this=0xbeea6910) at /home/fabrice/dev/birch/ipc/chromium/src/base/message_loop.cc:212 #11 MessageLoop::Run (this=0xbeea6910) at /home/fabrice/dev/birch/ipc/chromium/src/base/message_loop.cc:186 #12 0x40b13cc0 in nsBaseAppShell::Run (this=0x43d24820) at /home/fabrice/dev/birch/widget/xpwidgets/nsBaseAppShell.cpp:163 #13 0x40433f6e in XRE_RunAppShell () at /home/fabrice/dev/birch/toolkit/xre/nsEmbedFunctions.cpp:659 #14 0x40b9052c in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x42102340, aDelegate=0xbeea6910) at /home/fabrice/dev/birch/ipc/glue/MessagePump.cpp:198 #15 0x40de63bc in MessageLoop::RunInternal (this=0x43d24820) at /home/fabrice/dev/birch/ipc/chromium/src/base/message_loop.cc:219 #16 0x40de6472 in MessageLoop::RunHandler (this=0xbeea6910) at /home/fabrice/dev/birch/ipc/chromium/src/base/message_loop.cc:212 #17 MessageLoop::Run (this=0xbeea6910) at /home/fabrice/dev/birch/ipc/chromium/src/base/message_loop.cc:186 #18 0x4043445e in XRE_InitChildProcess (aArgc=-1091933548, aArgv=0xbeea6a18, aProcess=1108456960) at /home/fabrice/dev/birch/toolkit/xre/nsEmbedFunctions.cpp:496 #19 0x00008552 in main (argc=7, argv=0xbeea6a94) at /home/fabrice/dev/birch/ipc/app/MozillaRuntimeMain.cpp:60
(In reply to Fabrice Desré [:fabrice] from comment #96) > I checked again the app id and I was wrong in comment 93. > CheckLoadURIWithPrincipal in the child is always called with the correct app > id. It's only in the parent that we get an app id of NO_APP_ID. > Hmm IIRC that check is done also on the parent to verify that only childs with app:manage (or whatever the permission is actually called) load resources from zips other than their own. I guess the parent will use the appID of the child process to do the check then?
re: comment 97: yes the app:// checks on the parent should have the correct appID--we get it from the TabParent. It's a different codepath from CheckURIWithPrincipal.
I've been reading the code and I have a question/proposal/doubt. Can we just cancel the request and create a new request originating from the system principal instead of the original principal? That would remove the problem with the checks and in fact is more representative of what is actually happening.
Attached patch another try (obsolete) — — Splinter Review
This patch tries to hook up the url change in nsDocShell::OnRedirectStateChange(), by canceling the request and calling nsDocShell::LoadURI() with the new url. LoadURI() succeeds, but no content is actually loaded... I'm running out of time on this one, so if someone has cycles, feel free to steal.
Attachment #736425 - Attachment is obsolete: true
jdm says he can help dig in here.
Assignee: fabrice → josh
I've tried a build with Fabrice's latest patch and Arcturus' gaia changes on my Buri, and I can't actually see a problem. The Facebook contact sync page comes up, and gdb shows the network request for http://intense-tundra-4122.herokuapp.com/fbowd/oauth2_new/flow.html being cancelled, and no other network request outside of facebook occurring.
Is the redirected-to page being loaded, shown and/or executed also?
I've confirmed through gdb that we're loading redirects/redirect.html and all of its resources before the Facebook contact sync. I don't see anything in particular besides the page with the progress bar that waits on Facebook, but I'm not sure what I'm supposed to see.
I see the exact same behaviour on an unagi: the redirect is cancelled, and we load the correct page from the jar, after which the facebook contact sync page appears.
Just in case it was a timing thing, I tried without any debugger attached, and the contact sync page appeared without any problems.
If you can import the contacts correctly and heroku isn't being contacted, I would say this works then :)
All of my testing so far has been against b2g18 or v1.0.1, so I'm going to test against mozilla-central as well.
mozilla-central shows me the Facebook contact selection page as well, so I'm not sure what failures you were seeing, Fabrice.
(In reply to Josh Matthews [:jdm] from comment #109) > mozilla-central shows me the Facebook contact selection page as well, so I'm > not sure what failures you were seeing, Fabrice. I just tried again, with the last patch I attached (https://bugzilla.mozilla.org/attachment.cgi?id=747213) and still don't get that to work. I'm puzzled...
I'm going to unassign myself for now. If there's any more work I can do, let me know.
Assignee: josh → nobody
I've prepared a simple app test to check the redirect. The app just opens a window on another domain which then tries to load the controlled URL by three ways: 1. Linking directly to the URL (<a href="controlled_url">) 2. Doing a document.location.href="controlled_url" 3. Loading another page which returns a 302 with the controlled_url. The third case works correctly (it captures the controlled url and shows instead a local page). There are some quirks with Gaia (the opened window 'location bar' shows the wrong domain and doesn't show the redirected to URL) but it works. The other two cases don't work. So if we assume than the oauth redirection is going to be via a 302 (and I guess Facebook must be) this solution is enough. Otherwise, it isn't.
Antonio, this patch only deals with 30x redirects, so what you are seeing is expected. Can you send me your app? I still don't understand why this is failing for me...
So, it appears that this is actually working, and the failure I saw was in my frankenstein gaia build. My apologies to everyone that lost time on that. I'm gonna clean up the patch.
Assignee: nobody → fabrice
Attached patch patch — — Splinter Review
Cleaned up version of the latest patch. Boris, can you look at the docshell part? When we are about to redirect, we check if we have an application specific override for this redirect and load the new url instead. We only need that for 30X redirects since this is what the oauth flows are using. We don't want to turn that into a generic uri override feature.
Attachment #747213 - Attachment is obsolete: true
Attachment #749040 - Flags: review?(ferjmoreno)
Attachment #749040 - Flags: review?(bzbarsky)
> The [redirect[ case works correctly... There are some quirks with Gaia (the > opened window 'location bar' shows the wrong domain and doesn't show the > redirected to URL) but it works. Does the new patch show the correct domain/URI? Do we care?
Comment on attachment 749040 [details] [diff] [review] patch Hmm. You control the set of URIs sufficiently that you know it's OK to throw out their POST data and whatnot? I guess this is OK given that...
Attachment #749040 - Flags: review?(bzbarsky) → review+
Comment on attachment 749040 [details] [diff] [review] patch Review of attachment 749040 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/AppsUtils.jsm @@ +92,5 @@ > installerAppId: aApp.installerAppId || Ci.nsIScriptSecurityManager.NO_APP_ID, > installerIsBrowser: !!aApp.installerIsBrowser, > storeId: aApp.storeId || "", > + storeVersion: aApp.storeVersion || 0, > + redirects: aApp.redirects Same comment as my previous review :), can we add a check in AppsUtils.checkManifest for the 'redirects.to' parameter so only relative URLs are allowed for this field? We are prepending the app origin to 'redirects.to', so only relative URLS would work. ::: dom/apps/src/Webapps.jsm @@ +1334,4 @@ > return [toHexString(hash.charCodeAt(i)) for (i in hash)].join(""); > }, > > // Updates the activities and system message handlers. Also same nit as before, update the comment, please. ::: dom/interfaces/apps/nsIAppsService.idl @@ +65,5 @@ > jsval getAppInfo(in DOMString appId); > > /** > + * Returns a URI to redirect to when we get a redirection to 'uri'. > + * Returns null if no redirection is declared for this uri. nit: "...or the app is not privileged".
Attachment #749040 - Flags: review?(ferjmoreno) → review+
Whiteboard: [status: waiting for new Gecko patch][madrid] → [status: needs uplift][madrid]
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e6219c376cdf https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/0b6bcb1f4175 Note that under normal circumstances, I would have never seen this bug for uplift because 1) it's filed under Gaia and 2) it's hidden in a security group I'm not a member of (so it wouldn't show up even in the times I do look at the Gaia bugs). Please needinfo? me on these in the future so they don't slip between the cracks (or better yet, add me to the b2g security group).
status-b2g18: affected → fixed
status-b2g18-v1.0.1: affected → fixed
Whiteboard: [status: needs uplift][madrid] → [madrid]
Group: b2g-core-security
Component: Gaia::Contacts → DOM: Apps
Product: Boot2Gecko → Core
Version: unspecified → Trunk
Keywords: verifyme
QA Contact: jsmith
I assume ESR17 is unaffected here. Is that true?
(In reply to Al Billings [:abillings] from comment #122) > I assume ESR17 is unaffected here. Is that true? That's correct.
Keywords: verifyme
What needs to be documented on MDN about this bug?
Does this bug only affect b2g even though we fixed it in trunk?
(In reply to Mark Giffin [:markg] from comment #124) > What needs to be documented on MDN about this bug? I think the documentation is already there, this is the redirect manifest property. (In reply to Al Billings [:abillings] from comment #125) > Does this bug only affect b2g even though we fixed it in trunk? The root cause was b2g only, yes. The fix involved platform code that can be exercised by any web runtime.
I'm trying to figure out if this needs to be a Desktop Firefox advisory or if this is actually a Firefox OS issue that we fixed in platform.
(In reply to Al Billings [:abillings] from comment #127) > I'm trying to figure out if this needs to be a Desktop Firefox advisory or > if this is actually a Firefox OS issue that we fixed in platform. That's not a Desktop Firefox issue at all.
Thanks. Marking it for no advisory.
Whiteboard: [madrid] → [madrid][adv-main24-]
Group: core-security
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: