1449843 - Provide finer grained access model for nsAboutCapabilities

Status: RESOLVED INVALID

(Core :: DOM: Security, enhancement, P1)

Description

[Christoph Kerschbaumer [:ckerschb]]

Currently, all about: pages have access to every method within nsAboutCapabilities.  It seems to be necessary to fine grain that model so that each function can only be accessed by a pre-defined whitelisted set of about URIs.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1449843_fine_grained_access_for_aboutcap.patch

Attaching a POC patch here;

In summary: We can add AboutUtils.h/cpp which can perform the principal vetting and allows fine grained control over which capability is exposed to which about URI.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1449843_fine_grained_access_for_aboutcap.patch

Gijs, as discussed last week we would like a finer grained access control for the various functions within AboutCapabilities. Please note that this patch needs tweaking in various ways, in particular how we are going to provide the whiteList. Anyway, the patch highlights what would need to be done to provide that access control.

I think it makes sense to add AboutUtils.h/.cpp which also allows to abstract the access control away from nsDocument.cpp, but that's just as a nice side effect.

Now it gets interesting: If we want to provide a finer access control mechanism we basically have to provide a [Func="Allow*"] to every exposed function within the webidl. In other words, if someone wants to use a function then that someone would have to extend the whitelist with that particular about URI so one can access the function.

While I like the security guarantees of the provided mechanism, that approach feels a little clumsy and I was wondering if we could do better for what we want to accomplish here.

Any thoughts?

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Now it gets interesting: If we want to provide a finer access control
> mechanism we basically have to provide a [Func="Allow*"] to every exposed
> function within the webidl. In other words, if someone wants to use a
> function then that someone would have to extend the whitelist with that
> particular about URI so one can access the function.
> 
> While I like the security guarantees of the provided mechanism, that
> approach feels a little clumsy and I was wondering if we could do better for
> what we want to accomplish here.
> 
> Any thoughts?

I think this would mean extending the webidl parser / code generator so we could annotate the urls for which particular APIs get exposed more directly in the webidl (based on origin principals and/or documentURI for pages with nullprincipal), and/or via a pref that we annotate in the webidl (that last one feels like a potential for privesc though, so less sure that's a good idea). I don't know if that's easy/hard and/or if we should actually want to do that. I'm reasonably interested in the sense that I think some of our non-about: page things may also want this (e.g. we expose add-on manager APIs to some pages, but not most). Perhaps :mccr8 can comment on feasibility and/or if this is a good idea (he seems to have r='d various codegen bits for webidl before).

More generally though, I'm a little bit hesitant about putting too much effort into this particular solution for the "I have a content-privileged page in a content process and I want to do more-privileged things" before we / Mossop / browser-arch come to some kind of conclusion on what we want the mechanism to be. It feels like that could easily end up being wasted effort. I don't know what the plan is there (or if there is one yet!). Mossop ?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs (he/him) from comment #3)
> More generally though, I'm a little bit hesitant about putting too much
> effort into this particular solution for the "I have a content-privileged
> page in a content process and I want to do more-privileged things" before we
> / Mossop / browser-arch come to some kind of conclusion on what we want the
> mechanism to be. It feels like that could easily end up being wasted effort.
> I don't know what the plan is there (or if there is one yet!). Mossop ?

There is no clear path forward yet. I am currently generating a list and comparing pros and cons of RemotePageManager and AboutCapabilities. Anyway, I wanted to put that WIP patch up though to see how feasible it is to fine grain the access model of AboutCapabilities.

Comment 5

[Dave Townsend [:mossop]]

Yeah I'm wary of diving in too much before we've got the larger plan in place.

That said the patch as soon seems like it's going to create a lot of busy work and I wonder if there are simpler solutions that don't compromise security.

Can we just have each method do a principal check at the start against the permission manager or something rather than having to write one function for each method? It would mean the method would still be visible to every page but just throw an exception if called from a page that isn't allowed to use it. That might actually be clearer than methods not being present if you forget to update some whitelist somewhere.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Dave Townsend [:mossop] from comment #5)
> Can we just have each method do a principal check at the start against the
> permission manager or something rather than having to write one function for
> each method? It would mean the method would still be visible to every page
> but just throw an exception if called from a page that isn't allowed to use
> it. That might actually be clearer than methods not being present if you
> forget to update some whitelist somewhere.

I am personally fine with that approach. In particular, aboutCapabilities is only exposed to trusted about pages, so that by itself already minimizes the attack vector in my opinion. Doing additional checks add the beginning of every function within nsAboutCapabilities hence sounds reasonable to me.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to :Gijs (he/him) from comment #3)
> Perhaps :mccr8 can comment on feasibility
> and/or if this is a good idea (he seems to have r='d various codegen bits
> for webidl before).

I haven't done anything with WebIDL except review some simple local changes in a long time, so I'm not really sure about the specifics of your case. Maybe Boris could give some advice. It wasn't too hard to figure out how to add stuff to WebIDL, once you know where in the gigantic code gen file the things you need to change actually are.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Just answering the question in comment 3, that's doable.  We used to have things like that for b2g before we ripped out the complexity.  Fundamentally, under the hood, you'd codegen a function, use it as a [Func] automatically, etc.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

My understanding of the unprivileged scope is very limited, but I was pointed to this bug in context of enabling Fluent (l10n system).

For localization of the unprivileged pages like about:* pages, we'll need to expose the l10n.js file [0] which requires access to DOMLocalization.jsm[1].

If I understand correctly we'll want to get some flag that allows us to declare that a page like `about:rights` can load l10n.js and DOMLocalization.jsm.

Is this bug the right time to evaluate how the proposed API will enable this model?


[0] https://searchfox.org/mozilla-central/source/intl/l10n/l10n.js
[1] https://searchfox.org/mozilla-central/source/intl/l10n/DOMLocalization.jsm

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #8)
> Just answering the question in comment 3, that's doable.  We used to have
> things like that for b2g before we ripped out the complexity. 
> Fundamentally, under the hood, you'd codegen a function, use it as a [Func]
> automatically, etc.

Boris, I would like to get a feeling how hard it would be to update that code generator. Whenever you got some time, could you provide some pointers what would need to be updated and how we would do that in case we go down that route? Thanks!

Comment 11

[Boris Zbarsky [:bzbarsky]]

So...

Changing exposure of entire interfaces is not that bad.  See the getConditionList() function in Codegen.py.

Changing exposure of single methods is a bit more complicated, because it's fairly optimized for both time and space, and even then it's not small enough yet...  See the getControllingCondition() bit in Codegen.py.

I suspect the simplest path to victory if this is only going to be used in this one API is to turn whatever the new annotation here is into a code-generated function and effectively synthesize a Func pointing to that function.  We could reintroduce more per-property metadata like we used to have before https://hg.mozilla.org/mozilla-central/diff/20a6fd076505/dom/bindings/Codegen.py removed the b2g permission bits, but I'd really rather not go down that route....

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> So...

Thanks for the info. First we have to figure out how to move forward with AboutCapabilities anyway because it seems we are going to deprecate JS implemented WebIDLS (see See 1450827 comment 11).

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Update on this bug: By marking Bug 1457458 as a duplicate of Bug 1459204 I guess all the changes within this bug became invalid too. To find more inforamtion, please see Bug 1459204.