401535 - Bad separation of parameter- and presentation logic in register*Handler functions

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Jeff Walden [:Waldo]]

Currently navigator.registerContentHandler and navigator.registerProtocolHandler both do a minimal amount of work to get the window of the document and then pass that and the provided parameters off to the "web-content-handler-registrar".  None of the security checking for the API is done in the nsNavigator methods, and it's all done in the embedder-provided service.  This is all nice for the one JS implementation (because you can use JS), but it sucks more than a little for any other implementation, because then they have to assess even the security problems common to *all* implementations, rather than the ones specific to the way in which the info is presented.

While Firefox doesn't need this (it's already got everything implemented), extensions or embeddings which want to customize what these methods do will have to replicate all this validation logic, and I think it's likely the methods will accidentally include parameter-validation security holes in whatever they write, especially since the relevant standards are in flux.  We should move as much of the infrastructure as possible into the core C++ so everyone can securely use it.  We should make it *easy* to implement these APIs, and that means doing as much of the parameter validation as possible before they'd ever see such calls.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Dmose, how critical do you think this is? Doesn't sound like a blocker to me, but something that would be good to get fixed nevertheless.

Did anything like this come up in the recent security review?

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Waldo: I agree that having one implementation of this code is better than many.  But even if we pull the majority of this logic back into toolkit, why not keep it in JS?

sicking: Obviously, it's not critical to Firefox.  It looks to me as though for 1.9, we'll only be supporting web-based handlers for embedders who provide nsIBrowserDOMWindow, and, probably, only XUL-based embedders at that.  It is a real problem, though.  I'm not really sure how I can quantify it.

This did not come up in the security review, FWIW.

Comment 3

[Jeff Walden [:Waldo]]

(In reply to comment #2)
> Waldo: I agree that having one implementation of this code is better than many.
>  But even if we pull the majority of this logic back into toolkit, why not keep
> it in JS?

I don't mind it being in JS; my concern is that the current setup requires reimplementing the parameter-checking logic in every implementation of the registrar.  Unless you go through an additional layer of indirection (an intermediate service?  ugly, but then again, this isn't perf-critical) I don't see how you can do that.


> It looks to me as though for 1.9, we'll only be supporting web-based handlers
> for embedders who provide nsIBrowserDOMWindow, and, probably, only XUL-based
> embedders at that.

Hm, that's probably an even worse problem than having to reimplement all the security logic.  :-(

Comment 4

[Boris Zbarsky [:bzbarsky]]

> we'll only be supporting web-based handlers for embedders who provide
> nsIBrowserDOMWindow

That does basically mean XUL embedders only.  Is there a technical reason for this?  What do you use nsIBrowserDOMWindow for?

I do think we should try to fix this if at all possible.  It's really bad to have "Gecko" features (in the sense that this is an implementation of a proposed web spec) that only some Geckos have.  Makes it that much harder to convince webmasters that they should take the Mozilla Foundation's "sniff for Gecko, not Firefox" message seriously.

Comment 5

[Boris Zbarsky [:bzbarsky]]

One more question.  Is the validation being done mandated by the spec?  Or is it something we do for the sake of security, etc?

Comment 6

[Jeff Walden [:Waldo]]

A bit of both.  The spec mandates rejecting URL patterns without "%s" (which
doesn't seem insecure in and of itself at a glance, just nice to people who
mistype) and schemes the browser handles internally like http/https (which
would be a security problem), for starters.  We'll also prevent the given URL
from being a file: URL when bug 401343 is fixed, which isn't in the spec but
likely will be.  I could dig up more from skimming the spec or implementation
without much effort.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I feel pretty strongly that all the spec-compliance stuff should be in shared code....  (just factored out into a JS component is probably fine).

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #4)
> > we'll only be supporting web-based handlers for embedders who provide
> > nsIBrowserDOMWindow
> 
> That does basically mean XUL embedders only.  Is there a technical reason for
> this?  What do you use nsIBrowserDOMWindow for?

nsIBrowserDOMWindow is used to open a web-based protocol handler in a new tab or window, as appropriate.  In retrospect, I realize that I misspoke:  
What I was really referring to was that the feature won't support opening a web-based protocol handler in a new tab/window, which is likely to be the most typical case in a browser world.  However, as long as you pass in a windowContext parameter, it should work in any Gecko. 

Anyway, yeah, I totally agree that we should share the spec-compliance code.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> nsIBrowserDOMWindow is used to open a web-based protocol handler in a new tab
> or window, as appropriate.  

Could we switch to a window provider for that?  That's the embedding-friendly API for this functionality.  We'd need to make the API scriptable, right?  I think that should be fine, though I'll double-check.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Ooh, I wasn't aware of the existence of nsIWindowProvider.  It would need to be made scriptable.  Assuming the callers of providerWindow are guaranteed to be returned something that can be QIed (or whatever) to an interface that can either open the URI directly or be passed to the uri loader, it seems like that should work.

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK, looks like it was noscript just because I forgot to make it scriptable.  At least I see nothing that would keep us from making it scriptable.

provideWindow will either return a window or null if the caller should create a new window...

Come to think of which, why not just open the window via the window watcher, which will handle talking to the window provider for you?

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

windowwatcher has an API with the semantics "give me a new window or tab as per the user's pref"?  

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm..  Yes, that's exactly what OpenWindow and OpenWindowJS do if:

1) The parent is not a chrome window.
2) The window being opened is not a dialog.
3) The window being opened is not modal or being opened as chrome or being
   opened as a dialog.

Item 1 would be the biggest issue for you, but you'd need a treeowner for such a window anyway to use nsIWindowProvider (or rather to get an nsIWindowProvider)...  You might be able to use the window watcher for this....  Not sure it provides a good way to get the most recent window, though.

Right now I guess you use the window mediator, right?  That immediately makes you XUL-app-only, last I checked.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #13)
> Hmm..  Yes, that's exactly what OpenWindow and OpenWindowJS do if:
> 
> 1) The parent is not a chrome window.
> 2) The window being opened is not a dialog.
> 3) The window being opened is not modal or being opened as chrome or being
>    opened as a dialog.

Wow, I had no idea.  That's, uh, a little underdocumented.

> Item 1 would be the biggest issue for you, but you'd need a treeowner for such
> a window anyway to use nsIWindowProvider (or rather to get an
> nsIWindowProvider)...  You might be able to use the window watcher for this....
>  Not sure it provides a good way to get the most recent window, though.
> 
> Right now I guess you use the window mediator, right?  That immediately makes
> you XUL-app-only, last I checked.

Yes.  The code in question is at

http://mxr.mozilla.org/mozilla/source/uriloader/exthandler/nsWebHandlerApp.js#124

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pretty much all of window watcher is underdocumented.  It doesn't help that it's kinda frozen and all either. 

Not really sure whether there's an embedding-friendly way to do the window mediator recent window thing.  :(  Benjamin, do you know one?

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

With a few exceptions, I'm mostly focused on MailCo-related hacking now.  Reassigning a bunch of bugs to default component owners.  I'm happy to help with brainstorming/advice as needed, however.  

Search for the string MAILMONKEY to delete any bugmail generated by this change.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I filed bug 452164 to make nsIWindowProvider scriptable, for what it's worth.