Closed Bug 1278804 Opened 8 years ago Closed 8 years ago

Remove, fix or explain OriginAttributes.mAddonId

Categories

(Core :: DOM: Security, defect, P3)

Product:
Core
Component:
DOM: Security
Version:
36 Branch
Type:
defect

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1314361

People

(Reporter: smaug, Unassigned)

References

Details

(Whiteboard: [domsecurity-backlog]) 

Currently OriginAttributes.mAddonId is rather strange. It does not behave like any other attribute and that has already caused issues when we're trying to assert hard that origin attribute handling is right everywhere. (And we really should have more assertions for all this stuff.)

I don't understand why addonid is special and whether it was bug 1209162 which broke it or whether it has been broken all the time.
https://bugzilla.mozilla.org/show_bug.cgi?id=1209162#c54
"broken" or inconsistent with everything else. But if there is some reason for this inconsistency, we should probably move it out from origin attributes and use something else for it.

Based on some comments addonid is just some flag computed from principal uri. And if so, it shouldn't live in origin attributes but in principal itself... though, the principal itself represents the url, so why it is needed at all? ... don't understand
Propagation isn't really the defining characteristic of OriginAttributes - each one can have different propagation rules. The defining characteristic is that each attribute creates an orthogonal dimension in origin space.

Given the above, a token is embedded in the canonical serialization of an origin if and only if it is an OriginAttribute. IIRC bill wanted the serialization, and didn't really care whether it created an orthogonal origin dimension or not. That's why things are the way they are.
That still doesn't explain why addonid is so special originattribute, and why we even need it when it is just computed from the url.
FWIW, I think we should aim for not having different kinds of origin attribute propagation models. Different models just make it a lot harder to ensure we get oa right. (we're missing still some strong assertions for oa.)
But I guess I could be convinced that different models give us some flexibility we need in some special cases. But use cases, use cases...
We designed the separate propagation models specifically for various use-cases that we had. Some of them have become less relevant with the effective abandonment of the b2g security models, but it's still likely to prove useful.

Anyway, sicking is more or less in the driver's seat for OAs now - I'd recommend chatting with him for any proposals you have.
My proposal is that someone explains why addonId works the way it works. It is totally unclear to me why it has so special propagation model right now.
We definitely need to document different propagation models somewhere, and assert hard that we propagate OA the way we want.
Though, I still argue that inconsistencies in OA propagation models can easily lead to bugs which may be privacy or security issues. So, we should try hard to not have different models.


We have still some basic issues with setting up OA, it tends to happen too late after docshell creation. But that is a separate bug.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(jonas)
I can only explain the original motivation for this origin attribute. We created it to solve a specific problem for WebExtensions. A WebExtension's manifest can include a permissions directive that lists a set of domains that it's allowed to send XHRs to. It sends these XHRs from two different kinds of contexts:

- Pages associated with the add-on. These pages have URLs like moz-extension://<uuid>/foobar. It's pretty easy to associate this URL with the add-on using the UUID, and we can check the permission that way.

- Content scripts modifying normal web pages. These content scripts run with expanded principals that don't have any link to the add-on aside from the origin attribute.

The latter case is where the origin attribute is useful since it allows us to mark the expanded principal as being tied to the add-on. Then we can look up the manifest and check the permission directive. We also attach the add-on ID to moz-extension URIs since it's easier to have a single check based on the origin attributes.

I don't understand what you guys mean by "propagation model", but hopefully this answers the question to some extent.
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #6)
> - Pages associated with the add-on. These pages have URLs like
> moz-extension://<uuid>/foobar. It's pretty easy to associate this URL with
> the add-on using the UUID, and we can check the permission that way.
> 
The unusual propagation means that addonID isn't propagated to iframes of that page. Is that expected?
See http://searchfox.org/mozilla-central/rev/ef24c234ed53b3ba50a1734f6b946942e4434b5b/dom/base/nsFrameLoader.cpp#2100 and http://searchfox.org/mozilla-central/source/caps/BasePrincipal.cpp#71
I don't quite understand what kind of principal for example an <iframe src="data:text/html,"> gets when
loaded in moz-extension://<uuid>/foobar page.


> - Content scripts modifying normal web pages. These content scripts run with
> expanded principals that don't have any link to the add-on aside from the
> origin attribute.
Right, this case just needs some principal and there isn't really any "propagation" happening.
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #7)
> (In reply to Bill McCloskey (:billm) from comment #6)
> > - Pages associated with the add-on. These pages have URLs like
> > moz-extension://<uuid>/foobar. It's pretty easy to associate this URL with
> > the add-on using the UUID, and we can check the permission that way.
> > 
> The unusual propagation means that addonID isn't propagated to iframes of
> that page. Is that expected?
> See
> http://searchfox.org/mozilla-central/rev/
> ef24c234ed53b3ba50a1734f6b946942e4434b5b/dom/base/nsFrameLoader.cpp#2100 and
> http://searchfox.org/mozilla-central/source/caps/BasePrincipal.cpp#71
> I don't quite understand what kind of principal for example an <iframe
> src="data:text/html,"> gets when
> loaded in moz-extension://<uuid>/foobar page.

Thanks, I understand the problem now. I just tested in Chrome and they do allow such an iframe to do a cross-origin XHR in that case.

I also tested in Firefox and I'm getting a lot of problems, so it's hard to tell if this works how it's supposed to or not. I'll need to look more tomorrow.
Flags: needinfo?(wmccloskey)
Component: Security → DOM: Security
The reason I had problems with testing is that the random website I tested is actually using CORS to let anyone download their data.

Anyway, it looks like everything actually works with data URIs. The correct principal comes from nsScriptSecurityManager::GetChannelURIPrincipal, which calls MaybeSetAddonIdFromURI. The URI in this case is the HTML page containing the IFRAME whose src is the data: URI.

I can provide more details if necessary. I'm pretty unfamiliar with the principal code, so it's hard to know what helps.

Anyway, I think it would be fine to copy the add-on ID the same way we copy the user context ID. In this case, though, it seems unnecessary. The comment is correct.
Flags: needinfo?(wmccloskey)
Whiteboard: [domsecurity-backlog]
It seems like what we want is to associate some information (addon-id) with a script. We're not looking to create cookie jars at all?

If that's the case, then I agree with Olli that the use of OriginAttributes is wrong here.

(In reply to Bill McCloskey (:billm) from comment #6)
> - Pages associated with the add-on. These pages have URLs like
> moz-extension://<uuid>/foobar. It's pretty easy to associate this URL with
> the add-on using the UUID, and we can check the permission that way.
> 
> - Content scripts modifying normal web pages. These content scripts run with
> expanded principals that don't have any link to the add-on aside from the
> origin attribute.

If these are expanded principals, why can't we make one of the principals in that expanded principal be a principal with a moz-extension url? Or otherwise enable creating a principal with the additional information about which addonid this principal belongs to?
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #10)
> It seems like what we want is to associate some information (addon-id) with
> a script. We're not looking to create cookie jars at all?

Correct.

> If that's the case, then I agree with Olli that the use of OriginAttributes
> is wrong here.

Can you explain more? See my comments below.

> (In reply to Bill McCloskey (:billm) from comment #6)
> > - Pages associated with the add-on. These pages have URLs like
> > moz-extension://<uuid>/foobar. It's pretty easy to associate this URL with
> > the add-on using the UUID, and we can check the permission that way.
> > 
> > - Content scripts modifying normal web pages. These content scripts run with
> > expanded principals that don't have any link to the add-on aside from the
> > origin attribute.
> 
> If these are expanded principals, why can't we make one of the principals in
> that expanded principal be a principal with a moz-extension url?

In the case where extension A is modifying a page that's part of extension B, then the expanded principal would contain two principals from different extensions. We would have no way of knowing which one is modifying the content and which one is being modified.

> Or
> otherwise enable creating a principal with the additional information about
> which addonid this principal belongs to?

OriginAttributes were explained to me in exactly this way: a mechanism for associating additional information with a principal that also caused it to be distinguished from principals with different data.

I don't see any concrete reason why using them for the addon ID is bad. So far it has worked very well for us. That said, I'm happy to change the code that Olli pointed to in comment 7 so that the add-on ID is propagated in the same way as the user context ID (although I don't think it will affect anything).
> OriginAttributes were explained to me in exactly this way: a mechanism for
> associating additional information with a principal that also caused it to
> be distinguished from principals with different data.
> 
> I don't see any concrete reason why using them for the addon ID is bad. So
> far it has worked very well for us.

Changing OriginAttributes changes which cookies a request uses, and which indexedDB data a webpage sees. So it's not just additional information which is stored in the principal.

It appears that you have managed to change the information in the OriginAttributes such that the changing cookies/indexedDB does not actually affect the addon.

In particular, since the addonId is not propagated to NeckoOriginAttributes it doesn't affect which cookies are used in network requests. And since the addonId is not propagated to DocShellOriginAttributes it doesn't affect child iframes indexedDB.

And I guess the principal which you stick in the ExpandedPrincipal is never used when we look up indexedDB data, so it ends up skirting the indexedDB separation there too.


However OriginAttributes were definitely designed to create separate indexedDB and cookie "jars". And you are having to walk a pretty narrow line to avoid having that affect you. So I would definitely be worried that this is going to be a problem in the future.

And OriginAttributes have been hard enough for people to understand, so the fact that they are used in an entirely different way here is likely to cause more confusion.
Priority: -- → P3
(In reply to Bill McCloskey (:billm) from comment #11)
> I don't see any concrete reason why using them for the addon ID is bad. So
> far it has worked very well for us. That said, I'm happy to change the code
> that Olli pointed to in comment 7 so that the add-on ID is propagated in the
> same way as the user context ID (although I don't think it will affect
> anything).
Well, it lets us to have more consistent assertions. Right now we need to special case addonid in assertions in various places because it isn't propagated the usual way.


But propagating the addonid the same way as what contextid does, would change cookie and storage handling in iframes of moz-extension://<uuid>/foobar pages, right? Since right now those iframes don't inherit the flag, so they get a principal without addonid, assuming one loads some random web page (so, not data: or about: or javascript urls).
I don't know which behavior is wanted here.
(In reply to Olli Pettay [:smaug] from comment #13)
> But propagating the addonid the same way as what contextid does, would
> change cookie and storage handling in iframes of
> moz-extension://<uuid>/foobar pages, right? Since right now those iframes
> don't inherit the flag, so they get a principal without addonid, assuming
> one loads some random web page (so, not data: or about: or javascript urls).
> I don't know which behavior is wanted here.

OK, now I understand. Yes, we don't want a special cookie jar for iframes inside moz-extension pages.

I'm not sure what to do here though. It seems like maybe we just need a special property on the principal that tells us what add-on ID something is associated with. For codebase principals this would check if it's a moz-extension URI and then look up the UUID. For expanded principals it would do something special. We probably would need to set the add-on ID when we create the codebase principal.

Bobby, what do you think?
Flags: needinfo?(bobbyholley)
(In reply to Bill McCloskey (:billm) from comment #14)
> (In reply to Olli Pettay [:smaug] from comment #13)
> > But propagating the addonid the same way as what contextid does, would
> > change cookie and storage handling in iframes of
> > moz-extension://<uuid>/foobar pages, right? Since right now those iframes
> > don't inherit the flag, so they get a principal without addonid, assuming
> > one loads some random web page (so, not data: or about: or javascript urls).
> > I don't know which behavior is wanted here.
> 
> OK, now I understand. Yes, we don't want a special cookie jar for iframes
> inside moz-extension pages.
> 
> I'm not sure what to do here though. It seems like maybe we just need a
> special property on the principal that tells us what add-on ID something is
> associated with. For codebase principals this would check if it's a
> moz-extension URI and then look up the UUID. For expanded principals it
> would do something special. We probably would need to set the add-on ID when
> we create the codebase principal.
> 
> Bobby, what do you think?

So, is there a specific reason why we can't just call into the service each time we want to know? If the issue is just that doing so adds slowness to the hot path, we could just put a Maybe<uint32_t> mAddonIdCache, which would allow us to cache the value the first time it's queried. Then we wouldn't need to sorry about all the annoying serialization stuff (which, IIRC, was one of the major motivators for using OAs here).
Flags: needinfo?(bobbyholley)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.