1300833 - Cu.getWebIDLCallerPrincipal() shouldn't return an expanded principal

Status: RESOLVED INVALID

(Core :: XPConnect, defect)

Description

[(no longer active)]

We currently return the subject principal, if one is present, which can be an expanded principal <http://searchfox.org/mozilla-central/source/dom/bindings/CallbackObject.cpp#90>.  We should avoid returning expanded principals here.

Comment 1

[(no longer active)]

Boris, thoughts?  I'm planning to set webIDLCallerPrincipal to null if it's expanded.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Why would we want to do that?  What's wrong with returning an expanded principal here?

The point of this function is to allow JS-implemented WebIDL to do exactly what C++-implemented stuff would do with SubjectPrincipal().  If we allow SubjectPrincipal() to return an expanded principal, we should allow it here too.

Comment 3

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Why would we want to do that?  What's wrong with returning an expanded
> principal here?

Getting the origin attributes from an expanded principal will do the right thing.  Over in bug 1297687 we'd need to add assertions guaranteeing that cannot happen in practice.

> The point of this function is to allow JS-implemented WebIDL to do exactly
> what C++-implemented stuff would do with SubjectPrincipal().  If we allow
> SubjectPrincipal() to return an expanded principal, we should allow it here
> too.

The difference is that I have now audited how the OA from subject principals are used to ensure that we're not relying on the correctness of the OA of an expanded principal.

If getting an expanded principal here is indeed correct, I'd need to additionally audit Cu.getWebIDLCallerPrincipal() callers to at the very least.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I think we need to audit getWebIDLCallerPrincipal callers, yes.  :(  This function really is the moral equivalent of GetSubjectPrincipal() and is meant to be used the same exact way.

What are we doing about new code introducing new GetSubjectPrincipal() calls?

Comment 5

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> What are we doing about new code introducing new GetSubjectPrincipal() calls?

That is a good question, and is the reason why I tried to convince bholley that we need to deal with OAs of expanded principals in some way, but it's really not clear what behavior we want in the general case, and Bobby is not OK with spot fixing.

Effectively the fundamental issue here is that code that gets OAs from expanded principals is most likely doing something wrong, but in practice it's not really possible to do anything about that besides runtime checks.  Even that isn't easy to do with the current structure of the code, but I'm planning to add some assertions to at least detect some OA usages off of expanded principals.

It's unclear to me what we should do for new code using SubjectPrincipal() besides hoping that all reviewers are thinking about the implicating of subject principals being expanded at all times.  :(

Comment 6

[(no longer active)]

I audited getWebIDLCallerPrincipal() callers.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> I think we need to audit getWebIDLCallerPrincipal callers, yes.  :(  This
> function really is the moral equivalent of GetSubjectPrincipal() and is
> meant to be used the same exact way.
> 
> What are we doing about new code introducing new GetSubjectPrincipal() calls?

I think the solution here is release-mode assertions against:
(1) nsEP ending up as the principal of a document, and
(2) Getting OAs directly off of Expanded or Base principals (the script-accessible callsites will need to throw)