996992 - nsAboutProtocolHandler::NewChannel possible bug

Status: RESOLVED INVALID

(Core :: Security, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

In the course of trying to create an about handler with limited privs for doing some loop stuff in bug 976109 I ran across this comment:

http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp?from=nsAboutProtocolHandler.cpp#143

It seems to assert that the principal should generally be based on the channel's originalURI.  Unfortunately, from what I can see, that's not always true.  

In particular, I _think_ that http://dxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#282 is where the case of the null channel owner is handled, specifically starting around line 299, in the call to NS_GetFinalChannelURI.  http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1772 defines that function, and it turns out that the originalURI is used ONLY in the case of the nsIChannel::LOAD_REPLACE flag being cleared. 

Some testing suggests to me that adding at least one particular handler to 
http://dxr.mozilla.org/mozilla-central/source/browser/components/about/AboutRedirector.cpp is causing it to have the LOAD_REPLACE set.

The first thing I'm trying to figure out is if this is a bug.  In particular, for what I'm trying to implement, I _want_ the originalURI behavior, but because of the behavior I've described a bug, it's not particularly straightforward to get.

More importantly is that if the behavior described here _is_ a bug, I could imagine that it might have negative security consequences, but I don't have a complete enough mental model of how these things all fit together to really guess.

Thoughts?



that setting the channel owner to null, as is done in that clause, is handled

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yes, if you end up redirecting to something that then redirects to something else with LOAD_REPLACE, you will get a principal based on the last thing loaded.  That's the meaning of LOAD_REPLACE...

I'd like to understand what the situation is here that creates a channel with LOAD_REPLACE.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

After chatting with bz in IRC and then poking at code, it now looks to me as though LOAD_REPLACE is not actually coming into play here, and I was misinterpreting some of the behavior I was seeing.

I still am not entirely convinced that the intent of the cited comment is actually how the code is behaving, but I'm going to need to do some more debugging to figure that out, which I probably won't get to until tomorrow.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

After doing a bunch more debugging with lldb, bz helped clear up a bunch of things about what's going on here.  The IRC log starts at <http://logs.glob.uno/?c=content#c203774>.

The short version is that the comment I thought was wrong is actually correct in terms of what it says about originalURI.  I suspect it's also correct in what it says about the owner semantics, but I'm less certain of that.  I'm going to mark this bug as INVALID, and if I do discover a problem with the owner semantics, I'll reopen this or file a new one.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

[If I'm mischaracterizing any conversations, feel free to correct]

After spending a bunch of time pawing through the in-tree hawk code and documentation as well as talking with abr, it looks to me like hawk adds enough complexity to the picture that pulling it into MLP is likely to slow us down rather than speeding us up.  It's possible that a Hawk expert would assert that I'm wrong about that, I could imagine.

So here are the current theories about approaches in what I currently believe is the order we should try implementing them between now and MLP:

After then chatting with sicking, it became fairly clear that tweaking the owner stuff is not great a way to go, because owner itself has some problems.  In particular, owner has the problem that it's effectively a hook for the nsScriptSecurityManager that makes it harder to reason about as a whole, because one has to go look at all the parts of the tree that use it to really understand how things work.  To that end, it sounds like it may actually go away in the future in the interest of making the security manager easier to reason about.  With that in mind, it sounds like the actually simplest thing to implement to get cookie origins working right in the Loop code is a simple hard-coded exception in or around nsScriptSecurityManager::GetChannelPrincipal that would look up URIs that need special owners, and set those owners there.

Further discussion with sicking and then myk suggests that it should, in theory, be possible to use the webapp stuff already baked into gecko for this.  From what I can tell, this would involve setting the mozapp and maybe mozbrowser attributes on the various iframes that we're using, and doing anything else implied by that (eg creating a webapp manifest, ...).  My understanding is that using these attributes will probably Just Work even in a normal desktop profile because of the way the code was designed to work in FirefoxOS.  We'll need to verify that, of course.

If we end up wanting to stop trying to change the same-origin stuff to get cookies to work, we'll need to do something requiring both client and server side changes.  Abr and I speculated that rather than doing full-fledged tokens for MLP, the simplest/fastest change we could likely make would be switching to the original architecture plan of having the client-side generate a UUID, rather than having it handed down from the server.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Hah.  I put this in the wrong bug.  Plz ignore the above.