1214122 - Do not wrap addon channels if they use a gecko internal channel implementation

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Dragana Damjanovic [:dragana]]

Separating this issue from bug 1206199.

A custom protocol handle may return a normal gecko channel. We do not need to wrap that channel at all.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_1214122.patch

I moved this patch to a separate bug because t is easier for qa.

Honza can you please review it.
there are some comments:
https://bugzilla.mozilla.org/show_bug.cgi?id=1206199#c61

Comment 2

[Dragana Damjanovic [:dragana]]

It is worth checking whether we need a wrapper at all. Maybe all custom protocolHandler actually return a gecko channel.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Dragana, I am assigning this bug to you because you already did all the work.

The other question is, now since we are having this bug, do you think we don't even need the patch from Bug 1206199 for mediating AsyncOnChannelRedirect?

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8672968 [details] [diff] [review]
bug_1214122.patch

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

I'll quickly check the final version.

::: netwerk/base/nsIOService.cpp
@@ +762,5 @@
> +
> +            // The protocol handler does not implement NewProxiedChannel2, so
> +            // we need to wrap the channel. It is also possible that the custom
> +            // protocol handler has actually returned a gecko httpChannel
> +            // so we need to check for that option as well.

...option as well.

and then do what with that?  (The comment is kinda incomplete.  You could also simply refer to a comment for the MaybeWrap function as an explanation.)

::: netwerk/base/nsSecCheckWrapChannel.cpp
@@ +98,5 @@
> +  if (isGeckoChannel) {
> +    // If it is a gecko channel (ftp or http) we do not need to wrap it.
> +    channel->SetLoadInfo(aLoadInfo);
> +  } else {
> +    nsCOMPtr<nsIHttpChannelInternal> httpChannelInt =

I think you should check nsIHttpChannel interface, no?  Add-ons have no obligation to impl the nsIHttpChannelInternal interface.

Comment 5

[Dragana Damjanovic [:dragana]]

Attachment: bug_1214122.patch

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8673754 [details] [diff] [review]
bug_1214122.patch

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

Disclaimer: not locally tested and not checked in any way this actually fixes the original issue(s).

::: netwerk/base/nsSecCheckWrapChannel.cpp
@@ +83,5 @@
>      CHANNELWRAPPERLOG(("nsSecCheckWrapChannel::nsSecCheckWrapChannel [%p] (%s)",this, spec.get()));
>    }
>  }
>  
> +//static

// static

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: bug_1214122.patch

Comment 8

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86b949f810a8

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Dragana Damjanovic [:dragana]]

Comment on attachment 8674853 [details] [diff] [review]
bug_1214122.patch

Approval Request Comment
[Feature/regressing bug #]:
https://bugzilla.mozilla.org/show_bug.cgi?id=1120487
[User impact if declined]:
Users installing addons that implement their own protocolHandlers will crash the browser.
[Describe test coverage new/current, TreeHerder]:
tested with 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]:
yes

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Dragana, how can we test that? Thanks

Comment 14

[Sylvestre Ledru [:Sylvestre]]

(just use the xpi mentioned in comment #12?)

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8674853 [details] [diff] [review]
bug_1214122.patch

Fix a crash, taking it even if I am not really happy with the size of the patch.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0340cc26e8e5

Comment 17

[Sylvestre Ledru [:Sylvestre]]

N-i to make sure Jorge sees the idl change (we will crash otherwise)

Comment 18

[Dragana Damjanovic [:dragana]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> Dragana, how can we test that? Thanks

This is only an intermediate bug, which avoid wrapping a channel if it is not really needed.
This fix part of the crashes.
Bug 1214786 will fix all crashes with this addon.
Sorry for fixing it one by one.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e1261e250e85

Comment 20

[Marco Zehe (:MarcoZ)]

a11y-review flag seems to have been set unintentionally. Don't see anything accessibility-related here that needs verification from our team.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

(In reply to Dragana Damjanovic [:dragana] from comment #18)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> > Dragana, how can we test that? Thanks
> 
> This is only an intermediate bug, which avoid wrapping a channel if it is
> not really needed.
> This fix part of the crashes.
> Bug 1214786 will fix all crashes with this addon.
> Sorry for fixing it one by one.
Well, except if you can get the patch landed today, this is going probably going to be too late. Today, we are going to build 42 beta 9. That means that only RC is the only remaining build.

So, should we backout this patch or does it make sense to keep it?

Comment 22

[Dragana Damjanovic [:dragana]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #21)
> (In reply to Dragana Damjanovic [:dragana] from comment #18)
> > (In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> > > Dragana, how can we test that? Thanks
> > 
> > This is only an intermediate bug, which avoid wrapping a channel if it is
> > not really needed.
> > This fix part of the crashes.
> > Bug 1214786 will fix all crashes with this addon.
> > Sorry for fixing it one by one.
> Well, except if you can get the patch landed today, this is going probably
> going to be too late. Today, we are going to build 42 beta 9. That means
> that only RC is the only remaining build.
> 
> So, should we backout this patch or does it make sense to keep it?

I am only helping ckerschb, so I haven't look at the deadlines.

We should keep this patch it prevents addon from crashing, just a small part of the addon doesn't work.
(I am starting to mix up this bugs)
I will see to get the other one landed today.

Comment 23

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #17)
> N-i to make sure Jorge sees the idl change (we will crash otherwise)

Thanks, doesn't appear to be used by add-ons.

Comment 24

[Vasilica Tamas, QA [:vtamas]]

(In reply to Dragana Damjanovic [:dragana] from comment #18)

> This is only an intermediate bug, which avoid wrapping a channel if it is
> not really needed.
> This fix part of the crashes.
> Bug 1214786 will fix all crashes with this addon.
> Sorry for fixing it one by one.

I do not clearly understand whether this bug can be manually tested or not.

Dragana, if this issue needs to be verified, could you please provide some reliable steps in order to test it. Otherwise, please remove the qe-verify flag.

Comment 25

[Dragana Damjanovic [:dragana]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #24)
> (In reply to Dragana Damjanovic [:dragana] from comment #18)
> 
> > This is only an intermediate bug, which avoid wrapping a channel if it is
> > not really needed.
> > This fix part of the crashes.
> > Bug 1214786 will fix all crashes with this addon.
> > Sorry for fixing it one by one.
> 
> I do not clearly understand whether this bug can be manually tested or not.
> 
> Dragana, if this issue needs to be verified, could you please provide some
> reliable steps in order to test it. Otherwise, please remove the qe-verify
> flag.

This will be verified together with bug 1214786.