1214786 - Channelwrapper: Fix up maybeWrapChannel to wrap if not gecko internal channel

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Dragana Damjanovic [:dragana]]

with addon COMPUTERBILD-Abzockschutz-1.0.59.xpi installed

try to access elektromarkt-online.com (this web site is on the list of "abzock" sites)

The channel is not Http, I also checked it is not ftp, nsIFileChannel nor nsIJARChannel

I do not know what it is.

Comment 1

[Dragana Damjanovic [:dragana]]

Adding to the description above: if you try to access that site firefox is going to crash.

Do we have the source code of the addon? I tried on https://mxr.mozilla.org/addons but I have not found it.


It would be good to take a look into the source code of all addons that implement protocol handler. It would be good to know which channel we need to wrap, obvious only http is not enough.

Comment 2

[Dragana Damjanovic [:dragana]]

Attachment: bug_1214786.patch

So instead of having only httpChannel wrapper, I made an abstract one.

This fix the scenario described in this bug. 

What do you think?

Comment 3

[Dragana Damjanovic [:dragana]]

This is going to fail on:
http://mxr.mozilla.org/mozilla-central/source/caps/BasePrincipal.cpp#389

but this is a different issue, (i think)

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8674381 [details] [diff] [review]
bug_1214786.patch

Review of attachment 8674381 [details] [diff] [review]:
-----------------------------------------------------------------

I like that idea of having a generic wrapper and only wrap if it's not a gecko channel. Are we sure we can rely on nsIForcePendingChannel though? Just asking. If we can than that's great.

Jonas, I suppose you agree, right?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8674381 [details] [diff] [review]
bug_1214786.patch

Review of attachment 8674381 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsSecCheckWrapChannel.cpp
@@ -30,4 @@
>    NS_INTERFACE_MAP_ENTRY(nsIRequest)
>    NS_INTERFACE_MAP_ENTRY(nsIChannel)
> -  NS_INTERFACE_MAP_ENTRY(nsIUploadChannel)
> -  NS_INTERFACE_MAP_ENTRY(nsIUploadChannel2)

I suspect it's still a good idea to leave these interfaces on the secwrapper. In case an addon implements a channel that supports response headers. They are effectively no-ops anyway in case the addon doesn't support these interfaces.

I'm also not sure I understand why nsIInterfaceRequestor interface was added? I don't mind doing it though, just trying to understand.

@@ -102,5 @@
> -      do_QueryInterface(aChannel);
> -    // we can only wrap http channel.
> -    if (httpChannel) {
> -      // we have to wrap that channel
> -      channel = new nsSecCheckWrapChannel(aChannel, aLoadInfo);

The old code here is definitely bogus. New code looks correct.

Comment 6

[Dragana Damjanovic [:dragana]]

(In reply to Jonas Sicking (:sicking) from comment #5)
> Comment on attachment 8674381 [details] [diff] [review]
> bug_1214786.patch
> 
> Review of attachment 8674381 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsSecCheckWrapChannel.cpp
> @@ -30,4 @@
> >    NS_INTERFACE_MAP_ENTRY(nsIRequest)
> >    NS_INTERFACE_MAP_ENTRY(nsIChannel)
> > -  NS_INTERFACE_MAP_ENTRY(nsIUploadChannel)
> > -  NS_INTERFACE_MAP_ENTRY(nsIUploadChannel2)
> 
> I suspect it's still a good idea to leave these interfaces on the
> secwrapper. In case an addon implements a channel that supports response
> headers. They are effectively no-ops anyway in case the addon doesn't
> support these interfaces.
> 
> I'm also not sure I understand why nsIInterfaceRequestor interface was
> added? I don't mind doing it though, just trying to understand.
> 


I was trying to supply here only the necessary interfaces that a channel will have an forward all other getInteface query to the original channel. Therefore I added nsIInterfaceRequestor and change getInteface function.

Ideally we should create a wrapper for a channel that implements exactly interfaces that the channel implements and forward calls to the original channel.
If there is a better way to do this I will change it.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The idea of forwarding interfaces is to behave as similar as possible to Firefox 41 where the wrapper doesn't exist at all. This seems like the best way to maintain compatibility with existing addons.

If we forwarded enumerated all interfaces that addon-implemented channels implement, then we in theory should have near-perfect compatibility.

Using an nsIInterfaceRequestor doesn't seem to help with keeping existing addons and gecko-code working. I.e. the XHR code will QI the channel to nsIUploadChannel, it will not call channel->GetInterface to try to get at an nsIUploadChannel.

Comment 8

[Dragana Damjanovic [:dragana]]

Attachment: bug_1214786.patch

I mixed up getInterface and QueryInterface

Comment 9

[Dragana Damjanovic [:dragana]]

Attachment: bug_1214786.patch

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8674929 [details] [diff] [review]
bug_1214786.patch

Review of attachment 8674929 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsSecCheckWrapChannel.cpp
@@ +44,5 @@
> +    NS_ADDREF_THIS();
> +    return NS_OK;
> +  }
> +  return mChannel->QueryInterface(aIID, aResult);
> +}

I've spot a potential for crashing here already before.  When the inner channel actually doesn't QI to e.g. nsIUploadChannel (mUploadChannel left null) we easily crash, since: 1) the wrapper QI's to that interface 2) we use plain NS_FORWARD_* to mUploadChannel w/o any non-null checks.

I think that just doing all QI entries as conditional is pretty OK.

There are no good explanation nor backtrace of the crash that actually happens according comment 0 + 1 STR.  So it's hard to judge what is the correct approach here.

Your QueryInterface method, in this patch, violates the basic rule: QI to any interface always leads back to the same _object_.  For nsIChannel, nsIRequest and nsISecCheckWrapChannel you return the wrapper, and jumping across these 3 interfaces always leads to the secwrapper object, but for any other interface you return the inner channel where from you cannot ever go back to the secwrapper.  That is not permitted.  We may easily break some code with this when someone does nsIChannel(wrapper) --QI--> nsISomeIntreface(mChannel) --QI--> nsIChannel(X).  Correctly X should the the wrapper, but the caller will end up with mChannel in hands.  Who knows what and where may break when e.g. comparing any saved channel pointers with later result of a QueryInterface(nsIChannel) call.

If you really want to QI on the channel, I would rather do it as the last resort only for interfaces that e.g. the extension introduces which we cannot know.



OTOH, isn't there some XPCOM/XPConnect magic we could use to wrap and forward to ANY interface that the mChannel->QueryInterace may return?  Something tells me it should be possible.  Have a hashtable of IID->stub object, where the stub's QueryInterface would always delegate to the "central" nsSecCheckWrapChannelBase::QueryInterface.  Referencing would have to forward to the secwrapper as well, obviously.  

We don't have to return the same object for every interface, but we must return the same object for two different calls to QueryInterface with the same IID.  That's the only rule.

I think bz could point you at the right code.

@@ +92,5 @@
>  // static
>  already_AddRefed<nsIChannel>
>  nsSecCheckWrapChannel::MaybeWrap(nsIChannel* aChannel, nsILoadInfo* aLoadInfo)
>  {
> +  MOZ_ASSERT(aChannel, "can not create a channel wrapper without a channel");

OK, but how do we ensure this in the wild?  There should be a legal failure path when there is no inner channel passed in.  Or is the goal here different?

Actually, this should be handled near results of NewProxiedChannel/NewChannel.  When the implementation is faulty and return NS_OK while not returning a channel, we should fail with some regular NS_ERROR_X.

Comment 11

[Honza Bambas (:mayhemer)]

Dragana, I think NS_GetXPTCallStub could be your big friend here.  Look at https://mxr.mozilla.org/mozilla-central/source/xpcom/reflect/xptcall/xptcall.h#143 and below.

You may want to derive from nsAutoXPTCStub, implement its CallMethod to forward to NS_InvokeByIndex on mChannel.  

@that is mChannel, @methodIndex is aMethodIndex, @paramCount = aInfo->num_args, @params is an array you have to create manually: for each arg do nsXPTCVariant.Init(aParams[i], aInfo->params[i].type, aInfo->params[i].flags).  

You also have to figure out how to incorporate aInfo->result (where in the list of args you pass to NS_InvokeByIndex it belongs, also means to adjust paramCount accordingly).  You may inspect CallMethodHelper::InitializeDispatchParams() for that to answer.

Comment 12

[Honza Bambas (:mayhemer)]

Anyway, if this turns out to be to complicated for (only a temporary?) secwrapper implementation, go with breaking the QI rules for unknown interfaces.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Your QueryInterface method, in this patch, violates the basic rule: QI to
> any interface always leads back to the same _object_.

Yes! This was exactly my concern with this approach.

I don't know exactly what problems this would cause in practice, but it does seem like a scary rule to break given that we don't break it *anywhere* else to my knowledge.

> If you really want to QI on the channel, I would rather do it as the last
> resort only for interfaces that e.g. the extension introduces which we
> cannot know.

Yeah, at the very least we should do this. I.e. whitelist a pile of channel-related interfaces the same way that the existing code does.

> OTOH, isn't there some XPCOM/XPConnect magic we could use to wrap and
> forward to ANY interface that the mChannel->QueryInterace may return? 

XPConnect does this, and we at least used to have a cross-thread XPCOM proxy that was able to do this. However I think it adds a lot of complexity. Not really something we can uplift to beta. And probably not something that's wroth doing for what should be a temporary solution anyway.


My gut feeling would be to just whitelist a pile of channel-related interfaces and leave it at that. It seems safer to not break XPCOM QI rules. And at worst it'll break a few addons, but at least shouldn't be crash prone, which is something I'm worried about with breaking XPCOM QI rules.

Comment 14

[Dragana Damjanovic [:dragana]]

Attachment: bug_1214786_v2.patch

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #13)
> My gut feeling would be to just whitelist a pile of channel-related
> interfaces and leave it at that. It seems safer to not break XPCOM QI rules.
> And at worst it'll break a few addons, but at least shouldn't be crash
> prone, which is something I'm worried about with breaking XPCOM QI rules.

+1

I'm not worried about crashes that much, more about failing checks like |mOldChannel == aChannel| which might not pass where should.

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8675533 [details] [diff] [review]
bug_1214786_v2.patch

Review of attachment 8675533 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsSecCheckWrapChannel.cpp
@@ +29,5 @@
>    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIHttpChannel)
>    NS_INTERFACE_MAP_ENTRY(nsIRequest)
>    NS_INTERFACE_MAP_ENTRY(nsIChannel)
> +  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIUploadChannel, mUploadChannel)
> +  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIUploadChannel2, mUploadChannel2)

maybe add most interfaces nsHttpChannel implements? (a reasonable list, probably don't include callback-style interfaces)  it's just about listing them here and having members for them, so no big work.

Comment 17

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #16)
> Comment on attachment 8675533 [details] [diff] [review]
> bug_1214786_v2.patch
> 
> Review of attachment 8675533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsSecCheckWrapChannel.cpp
> @@ +29,5 @@
> >    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIHttpChannel)
> >    NS_INTERFACE_MAP_ENTRY(nsIRequest)
> >    NS_INTERFACE_MAP_ENTRY(nsIChannel)
> > +  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIUploadChannel, mUploadChannel)
> > +  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIUploadChannel2, mUploadChannel2)
> 
> maybe add most interfaces nsHttpChannel implements? (a reasonable list,
> probably don't include callback-style interfaces)  it's just about listing
> them here and having members for them, so no big work.

Actually the only addon that I have checks does not return an http channel at all, so for that one we do not need Http part at all (it implements only nsIUploadChannel2 :)) (I checked couple of others but they just return a gecko channel)

there is a bug 1216518 that should check which interfaces we actually need. So I will wait until that is investigated before adding and removing interfaces.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Seems that we want that in 42.

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 8675533 [details] [diff] [review]
bug_1214786_v2.patch

Review of attachment 8675533 [details] [diff] [review]:
-----------------------------------------------------------------

let's do it.

jonas is slow in reviews, I think my r+ should be enough if you want to land it yet today.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5aa550a2e1f

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8675533 [details] [diff] [review]
bug_1214786_v2.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1120487

[User impact if declined]:
Users installing addons which implement their own nsIProtocolHandler and using their own protocols (not http protocol) will crash.

[Describe test coverage new/current, TreeHerder]:
tested several addons including:
http://addons.computerbild.de/antiabzocke/plugin/COMPUTERBILD-Abzockschutz-1.0.59.xpi

[Risks and why]:
Low, because the patch only applies to users that install addons which implement their own protocol handlers (on AMO we where able to find ~150 such addons).

[String/UUID change made/needed]:
No

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8675533 [details] [diff] [review]
bug_1214786_v2.patch

Fix a crash, taking it.
Should be in 42 beta 9 (hopefully)

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d5aa550a2e1f

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6c1457027394
https://hg.mozilla.org/releases/mozilla-beta/rev/5927d34ab471

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Missed beta 9, will be in rc... :(

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Dragana, Christopher, could you check that the issue is fixed in the next aurora?

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #26)
> Dragana, Christopher, could you check that the issue is fixed in the next
> aurora?

Sylvestre, I am in contact with QA and we will perform mayor testing of the channelwrapper on mozilla-central and aurora this week to make sure our code is in good shape. If something comes up, I'll let you know right away. Thanks!