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

RESOLVED FIXED in Firefox 42

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed, firefox43 fixed, firefox44 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8672968 [details] [diff] [review]
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
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 2

2 years ago
It is worth checking whether we need a wrapper at all. Maybe all custom protocolHandler actually return a gecko channel.
Blocks: 1206199
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?
Assignee: nobody → dd.mozilla
Flags: needinfo?(dd.mozilla)
Flags: needinfo?(honzab.moz)
Attachment #8672968 - Flags: review?(honzab.moz)
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.
Attachment #8672968 - Flags: review?(honzab.moz) → feedback+
(Assignee)

Comment 5

2 years ago
Created attachment 8673754 [details] [diff] [review]
bug_1214122.patch
Attachment #8672968 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8673754 - Flags: review?(honzab.moz)
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
Attachment #8673754 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 7

2 years ago
Created attachment 8674853 [details] [diff] [review]
bug_1214122.patch
Attachment #8673754 - Attachment is obsolete: true
Attachment #8674853 - Flags: review+
(Assignee)

Comment 8

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86b949f810a8
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa80f05e188
Keywords: checkin-needed
No longer blocks: 1206199
Blocks: 1216214
Summary: Check whether a custom protocolHandler actually returns a gecko channel → Do not wrap addon channels if they use a gecko internal channel implementation
https://hg.mozilla.org/mozilla-central/rev/eaa80f05e188
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
https://hg.mozilla.org/mozilla-central/rev/eaa80f05e188
(Assignee)

Comment 12

2 years ago
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
Attachment #8674853 - Flags: approval-mozilla-beta?
Attachment #8674853 - Flags: approval-mozilla-aurora?
status-firefox42: --- → affected
status-firefox43: --- → affected
Dragana, how can we test that? Thanks
Flags: qe-verify+
Flags: needinfo?(dd.mozilla)
Flags: a11y-review?
(just use the xpi mentioned in comment #12?)
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.
Attachment #8674853 - Flags: approval-mozilla-beta?
Attachment #8674853 - Flags: approval-mozilla-beta+
Attachment #8674853 - Flags: approval-mozilla-aurora?
Attachment #8674853 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/0340cc26e8e5
status-firefox43: affected → fixed
N-i to make sure Jorge sees the idl change (we will crash otherwise)
Flags: needinfo?(jorge)
(Assignee)

Comment 18

2 years ago
(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.
Flags: needinfo?(dd.mozilla)
https://hg.mozilla.org/releases/mozilla-beta/rev/e1261e250e85
status-firefox42: affected → fixed
a11y-review flag seems to have been set unintentionally. Don't see anything accessibility-related here that needs verification from our team.
Flags: a11y-review?
(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?
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 22

2 years ago
(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.
Flags: needinfo?(dd.mozilla)
(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.
Flags: needinfo?(jorge)
(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.
Flags: needinfo?(dd.mozilla)

Updated

2 years ago
Depends on: 1218143

Updated

2 years ago
Depends on: 1195520
(Assignee)

Comment 25

2 years ago
(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.
Flags: needinfo?(dd.mozilla)
(Assignee)

Updated

2 years ago
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.