1120487 - Implement shim before moving security checks into AsyncOpen

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

After bug 1087720 lands, we should have a loadinfo on all gecko owned channels (created within C++ or JS). Before we can move security checks into AsyncOpen however, we have to implement a shim for addons. Addons can implement their own ProtocolHandlers and probably have not implemented newChannel2, or also AsyncOpen2(). In order to gracefully handle addons we need to implement a shim which forwards call to AsyncOpen in case AsyncOpen2() is not implemented.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: basic_shim.patch

Hey Jonas, when working on the shim I realized that we also might want to implement 'nsIWyciwygChannel'. Please note, that without inheriting 'nsIWyciwygChannel' everything compiles fine. When adding 'nsIWyciwygChannel' I get:

> nsIOService.cpp:711:15: error: ambiguous conversion from derived class 'nsSecCheckWrapChannel' to base class 'nsIChannel':
> class nsSecCheckWrapChannel -> class nsIWyciwygChannel -> class nsIChannel
> class nsSecCheckWrapChannel -> class nsIHttpChannel -> class nsIChannel
> channel = new nsSecCheckWrapChannel(channel, aLoadInfo);

Interesting is that resolving the ambiguity works just fine for NS_ISUPPORTS as well as NS_IREQUEST. I am really don't know why
> NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIWyciwygChannel)
doesn't do the trick here.

See the full mapping:
> NS_INTERFACE_MAP_BEGIN(nsSecCheckWrapChannel)
>  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIWyciwygChannel, mWyciwygChannel)
>  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIHttpChannel, mHttpChannel)
>  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIHttpChannelInternal, mHttpChannelInternal)
>  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIRequest, nsIWyciwygChannel)
>  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIChannel, nsIWyciwygChannel)
>  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIWyciwygChannel)
> NS_INTERFACE_MAP_END

* Any idea what might be wrong here?
* Also, let me know if something else feels like it should updated, simplified, or extended!

Comment 4

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

Comment on attachment 8574950 [details] [diff] [review]
basic_shim.patch

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

::: netwerk/base/nsSecCheckWrapChannel.cpp
@@ +26,5 @@
> +NS_IMPL_ADDREF(nsSecCheckWrapChannel)
> +NS_IMPL_RELEASE(nsSecCheckWrapChannel)
> +
> +NS_INTERFACE_MAP_BEGIN(nsSecCheckWrapChannel)
> +  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIWyciwygChannel, mWyciwygChannel)

I doubt any addons implement nsIWyciwygChannel, so I don't think we should have that here.

::: netwerk/base/nsSecCheckWrapChannel.h
@@ +40,5 @@
> +/*
> + * Using the MACRO from dist/include/nsIChannel.h to forward calls to the underlaying channel.
> + * The only methods we need to customize are the Get/SetLoadInfo bits.
> + */
> +#define NS_FORWARD_SAFE_SECCHECKWRAP_NSICHANNEL(_to) \

Another solution is to insert an extra class.

I.e. rename your main class to nsSecCheckWrapChannel_base and have that class forward all interfaces the same way you do now. Including forward all of nsIChannel.

Then create a subclass called nsSecCheckWrapChannel which overrides Get/SetLoadInfo and has a ctor which just forwards the ctor arguments to nsSecCheckWrapChannel_base.

Up to you.

@@ +70,5 @@
> +                                      , public nsIHttpChannelInternal
> +{
> +public:
> +  NS_FORWARD_SAFE_NSIWYCIWYGCHANNEL(mWyciwygChannel)
> +  NS_FORWARD_SAFE_NSIHTTPCHANNEL(mHttpChannel)

You shouldn't need to use the 'SAFE' versions of the macros. If mHttpChannel is null then these functions should never be called.

@@ +72,5 @@
> +public:
> +  NS_FORWARD_SAFE_NSIWYCIWYGCHANNEL(mWyciwygChannel)
> +  NS_FORWARD_SAFE_NSIHTTPCHANNEL(mHttpChannel)
> +  NS_FORWARD_SAFE_NSIHTTPCHANNELINTERNAL(mHttpChannelInternal)
> +  NS_FORWARD_SAFE_SECCHECKWRAP_NSICHANNEL(mChannel)

Likewise here. mChannel should never be null.

Comment 5

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

The problem isn't the QI macros. Those compile file. The problem is this line:


channel = new nsSecCheckWrapChannel(channel, aLoadInfo);

The compiler doesn't know how to cast an nsSecCheckWrapChannel to an nsIChannel since the compiler won't call QI. So you'll have to do something like:

channel = static_cast<nsIHttpChannel>(new nsSecCheckWrapChannel(channel, aLoadInfo));

But really, remove the wyciwyg channel stuff. It shouldn't be there in the final code anyway.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Thanks Jonas for all your answers. Having a subclass sounds like the cleaner solution, especially if someone extends nsIChannel.idl in the future, then we don't have to worry about any manual updates to our self-defined MACRO.

I will remove the wyciwyg channel again - but we should do a whole lot of testing before landing that patch - I am slightly worried at this time. Let's talk in our meeting on Wednesday.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1120487_shim.patch

Ok, only wrapping if newChannel2 is not available on the ProtocolHandler in the ioService. It seems to work fine after some initial testing, which included:
* browsing the web
* installing adBlock plus and browsing the web
* running some mochitests

Definitely needs more testing though!!!

The only time I see some channels getting wrapped look like this one:
nsSecCheckWrapChannelBase(file:///home/ckerschb/.cache/mozilla/firefox/...png)

So, potentially we still forgot to update some code somewhere, I think we shouldn't wrap file channels - we have to investigate, because nsFileProtocolHandler seems correct to me.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: shim.patch

I think this should be pretty much it - created and *.idl to give addons the ability to query the innerChannel from within our wrapper.

This is not going to land for 39 but really early in the cycle for 40 so we have a full cycle available to shake out eventual bugs.

Pushing to TRY as well:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=166fe8adf015

Comment 10

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

Comment on attachment 8576436 [details] [diff] [review]
shim.patch

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

::: netwerk/base/nsSecCheckWrapChannel.cpp
@@ +45,5 @@
> +
> +  {
> +    nsCOMPtr<nsIURI> uri;
> +    aChannel->GetURI(getter_AddRefs(uri));
> +    nsAutoCString spec;

This should be |#ifdef PR_LOGGING|

@@ +80,5 @@
> +{
> +  {
> +    nsCOMPtr<nsIURI> uri;
> +    mChannel->GetURI(getter_AddRefs(uri));
> +    nsAutoCString spec;

Same here. Or just remove this since the base class does it.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #10)
> Comment on attachment 8576436 [details] [diff] [review]
> shim.patch
> 
> Review of attachment 8576436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsSecCheckWrapChannel.cpp
> @@ +45,5 @@
> > +
> > +  {
> > +    nsCOMPtr<nsIURI> uri;
> > +    aChannel->GetURI(getter_AddRefs(uri));
> > +    nsAutoCString spec;
> 
> This should be |#ifdef PR_LOGGING|
> 
> @@ +80,5 @@
> > +{
> > +  {
> > +    nsCOMPtr<nsIURI> uri;
> > +    mChannel->GetURI(getter_AddRefs(uri));
> > +    nsAutoCString spec;
> 
> Same here. Or just remove this since the base class does it.

Thanks Jonas, but I guess we are going to delete the PR_LOGGING anyway - it's just for testing the shim before landing.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Putting a MOZ_ASSERT(false) in the constructor of the channel wrapper. We need to make sure it's not called on TBPL (our own code) - otherwise we have to fix it before we land the shim:
   https://treeherder.mozilla.org/#/jobs?repo=try&revision=38c638be43c8

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

We have to shake out the remaining problems here where we are still calling newChannel or do not propagate the same laodInfo instance on channels. Pushing this bug together with the patches of bug 1147562 to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17485a876539

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1120487_shim_ioservice.patch

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1120487_shim_wrapper.patch

Comment 16

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

Comment on attachment 8589790 [details] [diff] [review]
bug_1120487_shim_wrapper.patch

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

::: netwerk/base/nsSecCheckWrapChannel.cpp
@@ +26,5 @@
> +
> +NS_INTERFACE_MAP_BEGIN(nsSecCheckWrapChannelBase)
> +  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIHttpChannel, mHttpChannel)
> +  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIHttpChannelInternal, mHttpChannelInternal)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIRequest, nsIHttpChannel)

This one probably doesn't need to use the _AMBIGUOUS macro

@@ +28,5 @@
> +  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIHttpChannel, mHttpChannel)
> +  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIHttpChannelInternal, mHttpChannelInternal)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIRequest, nsIHttpChannel)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIHttpChannel)
> +  NS_INTERFACE_MAP_ENTRY(nsISecCheckWrapChannel)

You need an entry for nsIChannel as well

Comment 17

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8589790 [details] [diff] [review]
bug_1120487_shim_wrapper.patch

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

Looks good. Some nits just. r=me.

::: netwerk/base/nsISecCheckWrapChannel.idl
@@ +18,5 @@
> +{
> +  /**
> +   * Returns the wrapped channel inside this class.
> +   */
> +  readonly attribute nsIChannel innerChannel;

I know you and Jonas discussed this; I know there was some discussion about including this API. Since it will be a little unsafe for the addon devs to use this, do you want to call it unsafeInnerChannel?

::: netwerk/base/nsSecCheckWrapChannel.cpp
@@ +41,5 @@
> + , mHttpChannel(do_QueryInterface(aChannel))
> + , mHttpChannelInternal(do_QueryInterface(aChannel))
> + , mRequest(do_QueryInterface(aChannel))
> +{
> +  NS_ASSERTION(mChannel, "can not create a channel wrapper without a channel");

MOZ_ASSERT?

@@ +55,5 @@
> +
> +NS_IMETHODIMP
> +nsSecCheckWrapChannelBase::GetInnerChannel(nsIChannel **aInnerChannel)
> +{
> +  NS_ADDREF(*aInnerChannel = mChannel);

nit: I think the preferred way is to use 'forget' now. See https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp?from=HttpBaseChannel.cpp#1269 for an example.

Or NS_IF_ADDREF :)

@@ +92,5 @@
> +
> +NS_IMETHODIMP
> +nsSecCheckWrapChannel::GetLoadInfo(nsILoadInfo** aLoadInfo)
> +{
> +  CHANNELWRAPPERLOG(("nsSecCheckWrapChannel::GetLoadInfo()"));

You talked offline about maybe deleting these. If you keep them, print the pointer or the uri. Make sure it links back to the constructors log too.

::: netwerk/base/nsSecCheckWrapChannel.h
@@ +24,5 @@
> + * provides information about the content-type of the channel,
> + * who initiated the load, etc.
> + *
> + * Addon created channels might *not* provide that loadInfo object for
> + * some transition time before we mark the API as deprecated.

Which API? The old newChannel?

@@ +32,5 @@
> + * Please note that the wrapper only forwards calls for
> + *  * nsIRequest
> + *  * nsIChannel
> + *  * nsIHttpChannel
> + *  * nsIHttpChannelInternal

Please add a small comment about the Get/SetInnerChannel, and say something about it being readonly.

@@ +46,5 @@
> +  NS_FORWARD_NSIHTTPCHANNELINTERNAL(mHttpChannelInternal->)
> +  NS_FORWARD_NSICHANNEL(mChannel->)
> +  NS_FORWARD_NSIREQUEST(mRequest->)
> +  NS_DECL_NSISECCHECKWRAPCHANNEL
> +  NS_DECL_ISUPPORTS

I don't think these need to be threadsafe, so this is good right?

@@ +54,5 @@
> +protected:
> +  virtual ~nsSecCheckWrapChannelBase();
> +
> +  nsCOMPtr<nsIChannel>             mChannel;
> +  // we do a QI in the constructor

"We do a QI in the constructor to set the following pointers."

@@ +61,5 @@
> +  nsCOMPtr<nsIRequest>             mRequest;
> +};
> +
> +
> +class nsSecCheckWrapChannel : public nsSecCheckWrapChannelBase

Can you add a comment something like the following?

"We define a separate class here to make it clear that we're overriding Get/SetLoadInfo, rather that using the forwarded implementations provided by NS_FORWARD_NSICHANNEL"

My summary of what you told me offline :) Please amend as necessary.

Comment 18

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

> do you want to call it unsafeInnerChannel?

What unsafetyness in particular are you thinking about?

Comment 19

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Jonas Sicking (:sicking) from comment #18)
> > do you want to call it unsafeInnerChannel?
> 
> What unsafetyness in particular are you thinking about?

The addon calling wrapper.innerChannel.asyncOpen directly. No?

Comment 20

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

In the scenarios that we're worried about the addon is implementing the channel, but gecko is the one calling asyncOpen. So I don't think that's a concern.

If the addon is the one calling asyncOpen, then hopefully it's also doing security checks. So then things should be quite fine.

Comment 21

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8589789 [details] [diff] [review]
bug_1120487_shim_ioservice.patch

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

Looks good. r=me.

::: netwerk/base/nsIOService.cpp
@@ +685,3 @@
>  
> +    // Make sure that all the individual protocolhandlers attach a loadInfo.
> +    nsCOMPtr<nsILoadInfo> loadInfo;

Can this be moved inside the if branch?

Comment 22

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Jonas Sicking (:sicking) from comment #20)
> In the scenarios that we're worried about the addon is implementing the
> channel, but gecko is the one calling asyncOpen. So I don't think that's a
> concern.
> 
> If the addon is the one calling asyncOpen, then hopefully it's also doing
> security checks. So then things should be quite fine.

OK - just wanted to flag it.

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1120487_shim_wrapper.patch

Thanks Steve, addressed all of your concerns, but still calling the innerChannel just 'innerChannel'. It think addon developers will understand that innerChannel represents the wrapped channel and if they mess with that channel than they might get unexpected behavior.

Carrying over r+ from Jonas and Steve.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1120487_shim_ioservice.patch

Carrying over r+ from Jonas and Steve.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

If we decide to accept the TBPL assertions in Bug 1162657, then we should probably add a similar assertion to the constructor of a wrapped channel - because we shouldn't need to wrap any channels from within our own codebase.

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8604266 [details] [diff] [review]
bug_1120487_shim_ioservice.patch

When adding the new patch I forgot to carry over the r+ from Steve and Jonas. Let's make sure those patches work on all machines before landing (has been some time since the last try run):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf8997470811

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1120487_shim_wrapper.patch

Ups, seems like we have to mark the constructor as 'explicit' to make sure it compiles everywhere. Here we go. Otherwise try looks good so far.

Carrying over r+ from Steve and Jonas.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33c588a14445
https://hg.mozilla.org/integration/mozilla-inbound/rev/357fcbe1de45

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Backed out for card_manager_test.js Gaia unit test failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=10011964&repo=mozilla-inbound

Comment 30

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c2fbba7e47c

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Here is another TRY run where I put a MOZ_ASSERT and also a printf statement so we can somehow figure out what's going on here:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b52e3e99cbd

James, Gaia unit tests are failing on
> B2G Desktop Linux x64 opt
it seems I can't query any log for 'Gu' tests. Do you know how I can access those logs?

For the sake of completeness: on TBPL we shouldn't wrap any channels. Only for addon created channels where the protocolhandler of the addon does not provide a newChannel2 implementation. Potentially the problem is indeed covered by Bug 1166948. But again, I am not sure at the moment and would like to verify by inspecting the logs.

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

James, sorry, I forgot to set the needinfo flag, please see comment 31.

Comment 33

[James Lal [:lightsofapollo]]

So I see your output in the logs (example: https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/Iaqp2yD9TU6Dum9epkU_OA/2/public/logs/live_backing.log)

I am looking at "Gu" and clicking on the log link button in treeherder (the bars |||)

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

(In reply to James Lal [:lightsofapollo] Offline until May 26th from comment #33)
> So I see your output in the logs (example:
> https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/
> Iaqp2yD9TU6Dum9epkU_OA/2/public/logs/live_backing.log)
> 
> I am looking at "Gu" and clicking on the log link button in treeherder (the
> bars |||)

Thanks James.

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

So, the reason this patch got backed out was fixed in Bug 1166948. Within Gaia we were still using newChannel instead of newChannel2. Here is a TRY push that verifies it's fixed now.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8b8158e6f24

Going to reland whenever inbound opens up again!

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b06b05456f75
https://hg.mozilla.org/integration/mozilla-inbound/rev/4819768c871f

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b06b05456f75
https://hg.mozilla.org/mozilla-central/rev/4819768c871f

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

We're in the final build process for 41 at this point. I'm letting Ritu know!

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

carsten it looks like you landed this on m-c but marked it fixed for status-firefox41. That may mess up your queries...  I'm setting it back to "affected" for 41.

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Don't see a pressing reason to track this for 41, it is a wontfix.

Comment 41

[Carsten Book [:Tomcat]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #39)
> carsten it looks like you landed this on m-c but marked it fixed for
> status-firefox41. That may mess up your queries...  I'm setting it back to
> "affected" for 41.

Hey Liz,

so this landed back in June (June 1st) and the merge tool at that time was setting automatically the fixed status since this was the target milestone at that date. Even hg says for https://hg.mozilla.org/mozilla-central/rev/b06b05456f75 milestone: 41.0a1

so what the tool did with fixed for 41 should be technically ok or ?

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

Oh it was June. Whoops! Sorry tomcat. Somehow I saw a "9" not a 6 and thought the work had just landed. My mistake!

Comment 43

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

A patch for bug 1205410 landed on mozilla-release with this bug's number in the commit message, so you might see some bugspam for that bug in here.

Comment 44

[Sylvestre Ledru [:Sylvestre]]

I think that 42 is also fixed.