1316275 - Try to explicitly pass aTriggeringPrincipal to nsDocShell::LoadURI

Status: RESOLVED FIXED

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Ulimately we want to replace that whole block of code:




 // If principalIsExplicit is not set there are 4 possibilities:
  // (1) If the system principal or an expanded principal was passed
  //     in and we're a typeContent docshell, inherit the principal
  //     from the current document instead.
  // (2) In all other cases when the principal passed in is not null,
  //     use that principal.
  // (3) If the caller has allowed inheriting from the current document,
  //     or if we're being called from system code (eg chrome JS or pure
  //     C++) then inheritPrincipal should be true and InternalLoad will get
  //     a principal from the current document. If none of these things are
  //     true, then
  // (4) we don't pass a principal into the channel, and a principal will be
  //     created later from the channel's internal data.
  //
  // If principalIsExplicit *is* set, there are 4 possibilities
  // (1) If the system principal or an expanded principal was passed in
  //     and we're a typeContent docshell, return an error.
  // (2) In all other cases when the principal passed in is not null,
  //     use that principal.
  // (3) If the caller has allowed inheriting from the current document,
  //     then inheritPrincipal should be true and InternalLoad will get
  //     a principal from the current document. If none of these things are
  //     true, then
  // (4) we dont' pass a principal into the channel, and a principal will be
  //     created later from the channel's internal data.
  nsCOMPtr<nsIPrincipal> principalToInherit = triggeringPrincipal;
  if (principalToInherit && mItemType != typeChrome) {
    if (nsContentUtils::IsSystemPrincipal(principalToInherit)) {
      if (principalIsExplicit) {
        return NS_ERROR_DOM_SECURITY_ERR;
      }
      principalToInherit = nullptr;
      inheritPrincipal = true;
    } else if (nsContentUtils::IsExpandedPrincipal(principalToInherit)) {
      if (principalIsExplicit) {
        return NS_ERROR_DOM_SECURITY_ERR;
      }
      // Don't inherit from the current page.  Just do the safe thing
      // and pretend that we were loaded by a nullprincipal.
      //
      // We didn't inherit OriginAttributes here as ExpandedPrincipal doesn't
      // have origin attributes.
      principalToInherit = nsNullPrincipal::CreateWithInheritedAttributes(this);
      inheritPrincipal = false;
    }
  }
  if (!principalToInherit && !inheritPrincipal && !principalIsExplicit) {
    // See if there's system or chrome JS code running
    inheritPrincipal = nsContentUtils::LegacyIsCallerChromeOrNativeCode();
  }

  if (aLoadFlags & LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL) {
    inheritPrincipal = false;
    principalToInherit = nsNullPrincipal::CreateWithInheritedAttributes(this);
  }

  // If the triggeringPrincipal is not passed explicitly, we first try to create
  // a principal from the referrer, since the referrer URI reflects the web origin
  // that triggered the load. If there is no referrer URI, we fall back to using
  // the SystemPrincipal. It's safe to assume that no provided triggeringPrincipal
  // and no referrer simulate a load that was triggered by the system.
  // It's important to note that this block of code needs to appear *after* the block
  // where we munge the principalToInherit, because otherwise we would never enter
  // code blocks checking if the principalToInherit is null and we will end up with
  // a wrong inheritPrincipal flag.
  if (!triggeringPrincipal) {
    if (referrer) {
      nsresult rv = CreatePrincipalFromReferrer(referrer,
                                                getter_AddRefs(triggeringPrincipal));
      NS_ENSURE_SUCCESS(rv, rv);
    }
    else {
      triggeringPrincipal = nsContentUtils::GetSystemPrincipal();
    }
  }

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1316275_try_to_explicitly_pass_triggeringprincipal_to_loaduri.patch

Smaug, ideally we would not need the codeblock:
> if (!triggeringPrincipal) {
within nsDocShell::LoadURI() but rather have the actual triggeringPrincipal be passed to LoadURI (within the loadinfo argument).

Within this very rudimentary patch I am trying to figure out how feasible it would actually be to extend loadURI() as well as loadURIWithOptions() (within implementations of nsIWebNavigation.idl) to actually pass a valid triggeringPrincipal.

Even though I am using nullptr/null as a placeholder for the actual triggeringPrincipal I was wondering if you think my approach is feasible or even worth the time trying to pass the actual triggeringPrincipal. Obviously each callsite would need to be carefully evaluated what triggeringPrincipal to pass.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Even though I am using nullptr/null as a placeholder for the actual
> triggeringPrincipal I was wondering if you think my approach is feasible or
> even worth the time trying to pass the actual triggeringPrincipal.
> Obviously
> each callsite would need to be carefully evaluated what triggeringPrincipal
> to pass.
And that is very worrisome. All the triggering/loadingPrincipal handling is very error prone, how could we expect that the right principal would get passed as param?


In the patch you assert MOZ_ASSERT(triggeringPrincipal, "need a valid triggeringPrincipal");
but then have +  // if (!triggeringPrincipal) {
+  //   if (referrer) {
+  //     nsresult rv = CreatePrincipalFromReferrer(referrer,
+  //                                               getter_AddRefs(triggeringPrincipal));
So what is expected to happen?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8823235 [details] [diff] [review]
bug_1316275_try_to_explicitly_pass_triggeringprincipal_to_loaduri.patch

So I'm worried about this kind of approach. Wouldn't we end up spreading the knowledge about which principal to pass to even more places?
More callers to possibly pass the wrong principal.

What is the issue the patch tries to fix? Do we have cases when we would like to pass some principal explicitly as the triggeringPrincipal and not use the principal LoadURI currently ends up choosing?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8823235 [details] [diff] [review]
> bug_1316275_try_to_explicitly_pass_triggeringprincipal_to_loaduri.patch
> 
> So I'm worried about this kind of approach. Wouldn't we end up spreading the
> knowledge about which principal to pass to even more places?
> More callers to possibly pass the wrong principal.

Potentially yes, but ideally we would push the decision of what principal to use to the actual point in the code that triggers the load. The problem with the current 'create-principal-from-referrer' or also 'useSystemPrincipal' fallback is that we might accidentally use the wrong triggeringPrincipal just because of the fact that it's so hard to evaluate all the different load scenarios within docshell.

> What is the issue the patch tries to fix? Do we have cases when we would
> like to pass some principal explicitly as the triggeringPrincipal and not
> use the principal LoadURI currently ends up choosing?

Yeah, as previously mentioned I would like to cut down that forest of fallbacks where no one clearly knows what principal we end up using. Don't you agree that it would be nice to actually set triggeringPrincipal at the point where the load actually happens?

But don't get me wrong, I am not entirely sure myself, it's just that such a massive code block of if-else with boolean flags on top [1] makes we wanna cry. I would like to clean up that beast which has grown over time one step at a time. By pushing the triggeringPrincipal decision out of LoadURI I would imagine makes it easier to reason about triggered loads.

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1453

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Right. I'm just worried to push the decision making even to more places. There are so many callers that it feels like it will be guaranteed that wrong principal will be passed in some cases.

Could we somehow simplify whatever LoadURI is doing now, make the rules which principal to use simpler?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #6)
> Right. I'm just worried to push the decision making even to more places.
> There are so many callers that it feels like it will be guaranteed that
> wrong principal will be passed in some cases.

That is a very plausible concern and I actually share that concern.

> Could we somehow simplify whatever LoadURI is doing now, make the rules
> which principal to use simpler?

Potentially yes, but honestly I don't know how. Every load requires a triggeringPrincipal. As a first step it would be nice if we could figure out what the systemPrincipal fall back cases are and somehow classify them. In other words, the ultimate fallback should *not* be Systemprincipal. But it's also very hard to filter out those cases that actually do need a systemprincipal as the triggeringPrincipal given the provided arguments we have available.

Open to suggestions and further discussion - thanks for your time.

Comment 8

[Tanvi Vyas[:tanvi]]

I really want to stop falling back to systemPrincipal for triggeringPrincipal.  The referrer bit doesn't bother me as much.

What happens if we just remove that fallback?  How much failure do we see on try?

We now call AsyncOpen2 for DocShell loads.  Content Policy checks for top level documents and frames will get called with a triggeringPrincipal that may have been set to systemPrincipal because we didn't know what else to set it to.  Those loads will bypass content policy checks.

Boris may have said (though my memory is vague) that all the places that don't pass a triggeringPrincipal are from chrome and hence system is fine for them.  But how do we know this for sure?  How do we know there aren't other entry points that don't pass a trigger.  Can't those places just explicitly pass system?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> What happens if we just remove that fallback?

I expect that we break a bunch of extensions and the Firefox URL bar.

> But how do we know this for sure?

Which?  We know all the relevant callsites are chrome JS because:

1)  We can look for C++ callsites and we have audited them all.  Or if we haven't
    yet, we should!  There aren't very many of them.
2)  The JS callsites are using APIs that are only exposed to privileged JS,
    so are chrome by definition.

Whether system principal is fine for them is a separate issue.  That requires actually auditing the callsite and deciding what it's doing.

> How do we know there aren't other entry points that don't pass a trigger. 

Because we audited them, I believe.  Again, there aren't that many if you want to repeat the audit.

> Can't those places just explicitly pass system?

There is no script-exposed API for loading things in a docshell that allows passing in a triggering principal.  The patches in this bug so far aim to add one precisely so we can do that...

Comment 10

[Tanvi Vyas[:tanvi]]

This is part of a large project.  Removing my needinfo.  Chris, do we have a metabug for this, or is this it?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas[:tanvi] from comment #10)
> This is part of a large project.  Removing my needinfo.  Chris, do we have a
> metabug for this, or is this it?

This is the meta bug actually.

Comment 12

[Jonathan Kingston [:jkt] he/him]

Going to close this in favour of Bug 1333030. Most of the work has been completed now and follow up work can be tracked through that meta bug.