1305012 - Allow downgrading a redirected/processed channel's principal to null principal

Status: RESOLVED FIXED

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

Description

[Kershaw Chang [:kershaw]]

Please see bug 1295309 comment#62.

Comment 1

[Kershaw Chang [:kershaw]]

Gijs, I am not sure if this bug should be confidential or not.

May I know what do you think? Thanks.

Comment 2

[:Gijs (he/him)]

It's defense-in-depth and we're not aware of any way the current situation can be exploited, so I think this can be public.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #2)
> It's defense-in-depth and we're not aware of any way the current situation
> can be exploited, so I think this can be public.

Hey Kershar, Gijs, when would we need this exactly again? And what principal should be reset to a nullPrincipal? The LoadingPrincipal, the TriggeringPrincipal, or the PrincipalToInherit? Or all three?

Maybe if you could describe a brief usecase then I could help identifying which principal should be allowed (needed) to be reset to a NullPrincipal.

Comment 4

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Maybe if you could describe a brief usecase then I could help identifying
> which principal should be allowed (needed) to be reset to a NullPrincipal.

AIUI, basically, a streamconverter that handles a file of type application/foo at URI http://foo.com/ needs to be able to set the principal of the resulting channel/document to the null principal, ie per bug 1295309 comment 37:

> Modify the request somehow so that the channel owner is a new null principal.

The reason you want this is because after some chrome code has manipulated the channel (contents), in some cases it should no longer be same-origin with the original content, to avoid other content using it as a 'leg up' in privilege escalation.

Does that help?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #4)
> Does that help?

Thanks for the summary. I can remember now. It's also good to have the use case documented here in the bug. Putting in the backlog for now unless we need that right away. If so, please let me know.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to :Gijs Kruitbosch from comment #4)
> The reason you want this is because after some chrome code has manipulated
> the channel (contents), in some cases it should no longer be same-origin
> with the original content
So, this could also help to solve bug 1303086, correct?

Honza

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> So, this could also help to solve bug 1303086, correct?

If that helps, than I can do it.

Comment 8

[:Gijs (he/him)]

(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #4)
> > The reason you want this is because after some chrome code has manipulated
> > the channel (contents), in some cases it should no longer be same-origin
> > with the original content
> So, this could also help to solve bug 1303086, correct?

I don't really understand that bug. It seems to indicate we keep CSP HTTP headers around for this response and try to honour them. If those CSP headers are somehow part of the principal, then yes this would be fixed as a result of this bug. Otherwise, I don't think it would help (but it would still be safer to drop the CSP on the floor after we fix this bug than before).

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1305012_downgrade_principal_to_nullprincipal.patch

Gijs, I kept this function as generic as possible because we might run into a different use case eventually. Anyway, I suppose the new principal should still inherit the origin attributes of the current loadInfo.

Not sure if you would be the right reviewer for this in the end and I am also not sure if we should land this right away, but this patch could be used by someone working on one of those bugs where they would need support for that function on the loadInfo. Let me know if I missed anything. Or also feel free to forward that feedback request. Thanks!

Comment 10

[:Gijs (he/him)]

Can you explain how the principals you're setting here relate to the channel owner principal?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #10)
> Can you explain how the principals you're setting here relate to the channel
> owner principal?

Wait a second, is that the only thing you want to modify? The channel.owner? In that case my patch is useless and you just want to use channel->SetOwner(NullPrincipal); because GetChannelResultPrincipal() tries to query the owner first, before even looking at the loadInfo [1]. We might want to deprecate channel.owner at some point, but not in the upcoming future.

Also reading through [2] I think you just want to a get a different result from GetChannelResultPrincipal, right? If that is all what is needed here, then this bug is a wontfix.

If the owner is a nullptr, then we might need this patch however. Worth mentioning is that, if the loading is sandboxed, then we clear the owner at creation time of the channel [3], so GetChannelResultPrincipal() consumes the info from the loadInfo.

So the remaining question, is the loading sandboxed? Is the owner a nullptr?

[1] https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#338
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1295309#c51
[3] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#310

Comment 12

[Boris Zbarsky [:bzbarsky]]

The reason this bug exists is that afaik channel.owner is deprecated and we should not be adding new uses of it if we can avoid them...  If we're ok with adding such new uses, then sure, this is easy.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #12)
> The reason this bug exists is that afaik channel.owner is deprecated and we
> should not be adding new uses of it if we can avoid them...  If we're ok
> with adding such new uses, then sure, this is easy.

Indeed, we do have
> Bug 1286838 - Deprecate nsIChannel->owner and replace with nsILoadInfo->triggeringPrincipal
on file, but we still have a lot of use cases, but Boris is right, we should not introduce new usages in that case.

Comment 14

[:Gijs (he/him)]

The root problem for me here is that I don't know what I want. :-)

Or, more precisely, I know what I want: the code that ends up running in the HTML doc that we return should be running with the null principal.

What I'm less clear about is how to achieve that goal. The last few comments actually don't help me much: in today's world, is just setting the triggering and loading principal to null going to affect the resulting principal of the HTML document? If not, what about the channel owner? I'm happy to use the API in this patch if using the channel owner directly is deprecated, but if it doesn't work...

All that aside, if this is supposed to work I can try it tomorrow and tell you if it does or not and give f+ based on that, but I shouldn't be reviewing this patch.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> in today's world, is just setting the triggering and loading principal to null going to affect the resulting
> principal of the HTML document?

No, unless the SEC_FORCE_INHERIT_PRINCIPAL flag is set in the loadinfo.  If that's set, then the triggering principal is what will be used for the resulting document.

> If not, what about the channel owner?

Yes.  If the channel owner is not null, it will be used as the principal of the resulting document, period.  This overrides any settings from the loadinfo right now.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8795273 [details] [diff] [review]
bug_1305012_downgrade_principal_to_nullprincipal.patch

The intended consumer is devtools/client/converter-child.js, AIUI. So this needs to be scriptable to be accessible from the consumer.

Separately...

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #15)
> > in today's world, is just setting the triggering and loading principal to null going to affect the resulting
> > principal of the HTML document?
> 
> No, unless the SEC_FORCE_INHERIT_PRINCIPAL flag is set in the loadinfo.  If
> that's set, then the triggering principal is what will be used for the
> resulting document.

OK, but (how) can we change the flags from the nsIStreamConverter/Listener in converter-child? AFAICT they're readonly on the loadinfo, and I don't see a way to change them.

If this flag were set, AFAICT we could set principalToInherit ourselves and that would work without this extra API. Is that right?

It also looks like there's already a SEC_SANDBOX flag that basically does what we want, if we could set it. Or am I missing something?

> > If not, what about the channel owner?
> 
> Yes.  If the channel owner is not null, it will be used as the principal of
> the resulting document, period.  This overrides any settings from the
> loadinfo right now.

AFAICT the basic problem here is that we don't control the channel creation and we can't modify the loadinfo flags after that's happened. We can modify the channel owner object, which AFAICT is writable, but we'd like to not do that (I guess?).

I'm not very familiar with nsIStreamConverter and friends (in case that wasn't obvious...) but it seems like the other ones in the tree (the feedconverter and the pdfjs one) both create their own channel to a static page that they then send the right data. In the feed: case, we set originalURI to the original URI of the request, and perform some indirection to pass the original data to the page as it's displayed. Note that the feed: stuff writes to the channel owner property for their new channel, but I kind of assume that that could be fixed with the code as-is if we wanted to, as it controls the creation of the new channel.

Is there a reason a similar approach (to feed: and pdfjs) isn't used for the JSON viewer? Is there a simpler way to achieve the same result without rearchitecturing the whole thing? Boris, ISTR you were involved when this was implemented. Can you comment on what's going on here and what would be the simplest way of getting the json viewer to end up with a null principal?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> but (how) can we change the flags from the nsIStreamConverter/Listener in converter-child?

That needs to be added, right.  That's what this bug is (partially) about.

> It also looks like there's already a SEC_SANDBOX flag that basically does what we want, if we could set it

That's true.  That would work for your use case as well.

> we don't control the channel creation and we can't modify the loadinfo flags after that's happened

Indeed.

> but we'd like to not do that (I guess?).

Indeed.

> Is there a reason a similar approach (to feed: and pdfjs) isn't used for the JSON viewer?

Probably simplicity.  Note that other stream converters we have don't create different channels (e.g. see binhexdecoder, ftpdirlistingconv, indexedtohtml).  Some others do (multimixedconv, the two you point out).  Arguably binhexdecoder is similar to deflateconverter in that it does not change the data semantics, but indexedtohtml and ftpdirlistingconv certainly do...

> and what would be the simplest way of getting the json viewer to end up with a null principal?

The simplest way as the code stands today is to set a nullprincipal as .owner on the channel.  The only reason we're having this conversation at all is that we're trying to deprecate .owner, and this is a clear case of something that _can_ be done with .owner right now, and _should_ be supported in the new world without .owner, and we might as well add support for it, therefore, since we're going to need it at some point anyway.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #17)
> The simplest way as the code stands today is to set a nullprincipal as
> .owner on the channel.  The only reason we're having this conversation at
> all is that we're trying to deprecate .owner, and this is a clear case of
> something that _can_ be done with .owner right now, and _should_ be
> supported in the new world without .owner, and we might as well add support
> for it, therefore, since we're going to need it at some point anyway.

Since we are already having that discussion and we need to support that case in the future anyway, we might as well fix it right now within this bug.

Proposal: What if we add a new flag to nsILoadInfo: SEC_OVERRULE_CHANNEL_OWNER (or something similar) which will be set when we call ResetPrincipalsToNullPrincipal(). We reset all the three principals to a NullPrincipal and within GetChannelResultPrincipal, we first check the loadInfo if the flag SEC_OVERRULE_CHANNEL_OWNER is present and if so, return the overruled TriggeringPrincipal (which is a NullPrincipal). Just for the sake of completeness, that would happen before checking if the owner of the channel is not null :-). Somewhere around here:
https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#335

What do you think?

Comment 19

[Boris Zbarsky [:bzbarsky]]

It seems like it would be simpler to have a way to set SEC_SANDBOX to true, right?

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #19)
> It seems like it would be simpler to have a way to set SEC_SANDBOX to true,
> right?

We could, but using a separate flag would allow us to be more explicit of what we try to accomplish. Also, if a new consumer just calls loadinfo->ResetPrincipals() but does not also do channel->SetOwner(nullptr) then the loadInfo might have the right information but GetChannelResultPrincipal would still return the old owner of the channel. Within loadInfo we would have no control over that. Explicitly checking for that flag at the beginning of GetChannelResultPrincipal() would eliminate that risk however, right?

Comment 21

[Boris Zbarsky [:bzbarsky]]

That's fair, though I'm a bit surprised that SEC_SANDBOXED doesn't override owner...  Means it's impossible to sandbox chrome:// things.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #21)
> That's fair, though I'm a bit surprised that SEC_SANDBOXED doesn't override
> owner...  Means it's impossible to sandbox chrome:// things.

The problem is that loadinfo holds no pointer to the channel. Hence we overwrite the owner of a channel within ::NewChannel(), see:
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#811

Comment 23

[Boris Zbarsky [:bzbarsky]]

> The problem is that loadinfo holds no pointer to the channel

I wasn't surprised we don't _overwrite_ owner.  I was surprised it doesn't _override_ it for purposes of GetChannelResultPrincipal.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1305012_downgrade_principal_to_nullprincipal.patch

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1305012_downgrade_principal_to_nullprincipal_tests.patch

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Boris, I suppose the code within the first patch (second one only contains tests) is what we want in the end. Do you feel you want to review those bits or should I rather try to find someone else? Thanks!

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8795840 [details] [diff] [review]
bug_1305012_downgrade_principal_to_nullprincipal.patch

I you can find someone else to review this, that would be great.  That said, at a brief glance the IDL comments are confusing (e.g. the comment for resetPrincipalsToNullPrincipal() doesn't match the impl for the document case, the comment for forceInheritPrincipalOverruleOwner is identical to the one for forceInheritPrincipal except for the sandbox thing, SEC_FORCE_INHERIT_PRINCIPAL_OVERRULE_OWNER talks about "the Principal", whatever that is; maybe it means "the PrincipalToInherit"?).  Might be good to make those better.

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1305012_downgrade_principal_to_nullprincipal.patch

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #27)

> I you can find someone else to review this, that would be great.  That said,
> at a brief glance the IDL comments are confusing

Thanks Boris. I updated the documentation within the *.idl.

Olli, any chance you would want to review the changes here?

Comment 29

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

Not sure I understand yet what this bug is about. Who would use forceInheritPrincipalOverruleOwner and resetPrincipalsToNullPrincipal() ?




"non TYPE_DOUCMENT"

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #29)
> Not sure I understand yet what this bug is about. Who would use
> forceInheritPrincipalOverruleOwner and resetPrincipalsToNullPrincipal() ?

A possible consumer would be a streamconverter, see comment 4 for more details.

Comment 31

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

Comment on attachment 8795841 [details] [diff] [review]
bug_1305012_downgrade_principal_to_nullprincipal_tests.patch

So this tests only resetPrincipalsToNullPrincipal.
We need also some test that GetChannelResultPrincipal ends up working the expected way.

Comment 32

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

Hopefully the test would actually test the use case this stuff is being written for.

Comment 33

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

Comment on attachment 8797972 [details] [diff] [review]
bug_1305012_downgrade_principal_to_nullprincipal.patch

>+LoadInfo::ResetPrincipalsToNullPrincipal()
>+{
>+  // take the originAttributes from the LoadInfo and create
>+  // a new NullPrincipal using those origin attributes.
>+  PrincipalOriginAttributes pAttrs;
>+  pAttrs.InheritFromNecko(mOriginAttributes);
>+  nsCOMPtr<nsIPrincipal> newNullPrincipal = nsNullPrincipal::Create(pAttrs);
>+
>+  // the loadingPrincipal for toplevel loads is always a nullptr;
>+  if (mInternalContentPolicyType != nsIContentPolicy::TYPE_DOCUMENT) {
>+    mLoadingPrincipal = newNullPrincipal;
>+  }
So this should then assert that if we have TYPE_DOCUMENT, mLoadingPrincipal actually is null

>+   * For all loads of non TYPE_DOUCMENT this function resets the 
none TYPE_DOCUMENT

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1305012_downgrade_principal_to_nullprincipal.patch

(In reply to Olli Pettay [:smaug] from comment #33)
> So this should then assert that if we have TYPE_DOCUMENT, mLoadingPrincipal
> actually is null

Done! Thanks for the speedy review, carrying over r+ from smaug!

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1305012_downgrade_principal_to_nullprincipal_tests.patch

(In reply to Olli Pettay [:smaug] from comment #31)
> So this tests only resetPrincipalsToNullPrincipal.
> We need also some test that GetChannelResultPrincipal ends up working the
> expected way.

Indeed - extended the testcase - thanks!

Comment 36

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

Comment on attachment 8798174 [details] [diff] [review]
bug_1305012_downgrade_principal_to_nullprincipal_tests.patch

Ok, fine. Would be great to have test using streamconverter or something and actually see that things work as expected. But I guess such tests will be written once someone starts to use this.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b7254b9fe5f
Downgrade a new channel's principal to NullPrincipal. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc3a6811a69
Test downgrading a new channel's principal to NullPrincipal. r=smaug

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b7254b9fe5f
https://hg.mozilla.org/mozilla-central/rev/4bc3a6811a69

Comment 39

[Boris Zbarsky [:bzbarsky]]

Did this actually fix the thing from bug 1295309 that we wanted to fix?  I don't see us _using_ the new API anywhere!

Comment 40

[Boris Zbarsky [:bzbarsky]]

And the summary of this bug is completely useless: new channel where?  There's no new channel involved in any of the patches.  Could we please make the summary reflect what the bug was/is actually about?

Comment 42

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #40)
> And the summary of this bug is completely useless: new channel where? 
> There's no new channel involved in any of the patches.  Could we please make
> the summary reflect what the bug was/is actually about?

Sorry about that. We added that new API to allow downgrading a channel's principal to a null principal. We kept it very generic and just added a testcase within this Bug. The first actual consumer landed within Bug 1333210.