1143922 - Add AsyncOpen2 to nsIChannel and perform security checks when opening a channel

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

Since we have a loadInfo on (almost) all channels (see bug 1087720) and since we the shim (see bug 1129999) is almost ready to land - we can start thinking about moving security checks into asyncOpen2.

The way it would work:
* we add security checks to asyncOpen2() which internally then calls asyncOpen()
* we remove the nsIContentPolicy check from each callsite once at a time and call asyncOpen2() instead of asyncOpen().

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_securitymanager.patch

Hey Jonas, I think I already hit the problem you mentioned this morning :-) What is the best way to query the nsIStreamListener that is passed to AsyncOpen2 off the channel so I can pass it to the nsCORSListenerProxy??? Do you want me to extend nsIChannel so we can query the streamListener in the newly added securityManager?

I attached the first iteration of the securitymanager patch; there are still todos, but at the moment I am stuck, because I can't query the streamListener from the channel. What do you suggest?

Comment 2

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

I'm not sure I understand the question.

The streamListener is passed to AsyncOpen2, right? So just pass the streamlistener as an argument to doContentSecurityCheck.

The tricky part is that doContentSecurityCheck needs to be able to return a different streamlistener back to the caller, i.e. back to AsyncOpen2 so that we can install the CORS listener.

Comment 3

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

The easiest thing is likely to make doContentSecurityCheck take a nsCOMPtr<nsIStreamListener>& as argument. I think that's the recommended way to do in-out parameters in XPCOM.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #3)
> The easiest thing is likely to make doContentSecurityCheck take a
> nsCOMPtr<nsIStreamListener>& as argument. I think that's the recommended way
> to do in-out parameters in XPCOM.

So yes, that's exactly the point. AsyncOpen gets nsIStreamListener as first argument, which now gets potentially wrapped into nsCORSListenerProxy(listener,...). We either need an in/out parameter to to doContentSecuriteryCheck, or we extend nsiChannel with a getter/setter for the listener. I am rather for an in/out parameter to doContentSecurityCheck because otherwise the getter and setter on nsIChannel might get abused.

Comment 5

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

Exactly. Don't add a getter/setter on nsIChannel just for this.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_loadinfo_changes.patch

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_securitymanager.patch

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_nsichannel.patch

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_htmlmedaielement.patch

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Jonas, would you mind providing some feedback on the these patches? Some notes:

* loadinfo_changes: just adding the four new flags which then get checked in AsyncOpen before performing the security checks.

* securitymanager: All the functionality for performing security checks should be there. Agreed we can factor out some of the functionality into different functions.

* nsichannel: Adding asyncOpen2 and calling the newly added securityManager. I decided to do add an incoming and also provide an outgoing argument for the streamListener. Since AsyncOpen2() gets a raw (not an XPCOM) pointer for the streamListener, I think providing and incoming as well as an outgoing argument is the easier solution for the securityManager. If you disagree, I am happy to rewrite. Nevertheless, the current approach seems like to be the cleanest solution.

* htmlmedaielement: The first callsite to change from AsyncOpen to AsyncOpen2(). Removed all the security checks at the callsite.

* I left a few questions for you in the code. Just grep for "jonas:" - would be great if you can help me get those answered.

* CSP and MixedContentBlocker tests work with the patches applied (also covering TEST_MEDIA tests of course).

* We don't have any CORS tests that are specific to TYPE_MEDIA, do we want to write some?

* Overall comments are also very welcome - thanks!

Comment 11

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

Comment on attachment 8606490 [details] [diff] [review]
bug_1143922_add_asyncopen2_loadinfo_changes.patch

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

::: netwerk/base/nsILoadInfo.idl
@@ +59,5 @@
> +   * +-------------------------------------------------------------------+
> +   * |                                                                   |
> +   * |  EVERY LOADINFO MUST HAVE ONE OF THE FOLLOWING FOUR FLAGS SET.    |
> +   * |                                                                   |
> +   * +-------------------------------------------------------------------+

This is a bit of a lie, since none of the existing code sets one of these flags but still work fine.

So we might want to say that setting one of these is required in order to call AsyncOpen2. And that long term setting one of these flags will be required.

Comment 12

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

Comment on attachment 8606491 [details] [diff] [review]
bug_1143922_add_asyncopen2_securitymanager.patch

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

::: dom/security/nsContentSecurityManager.cpp
@@ +30,5 @@
> +
> +nsresult
> +nsContentSecurityManager::doContentSecurityCheck(nsIChannel* aChannel,
> +                                                 nsIStreamListener* aStreamListener,
> +                                                 nsIStreamListener** outCorsListener)

I prefer a single nsCOMPtr<...>& here, but I'll leave this up to the module owner/peer who's reviewing.

@@ +39,5 @@
> +  nsresult rv = aChannel->GetLoadInfo(getter_AddRefs(loadInfo));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!loadInfo) {
> +  	NS_ASSERTION(false, "channel needs to have loadInfo to perform security checks");

We should use MOZ_ASSERT here. No reason to let the code keep running in debug builds since this only affects code that intentionally changes to use AsyncOpen2.

@@ +41,5 @@
> +
> +  if (!loadInfo) {
> +  	NS_ASSERTION(false, "channel needs to have loadInfo to perform security checks");
> +    // some addon channels might not have a loadInfo attached, we have to allow such loads.
> +  	return NS_OK;

Return an error here.

@@ +63,5 @@
> +    fprintf(stderr, "}\n\n");
> +  } // -------------------------------------------------------------
> +
> +  nsCOMPtr<nsIURI> channelURI;
> +  aChannel->GetURI(getter_AddRefs(channelURI));

I'm not actually sure that this will always give the right URL. Redirects make this complicated. Please check what happens for schemes like about: and chrome:. Probably worth consulting with bz.

@@ +74,5 @@
> +  if (securityFlags & nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN) {
> +    // make sure that same-origin is not required and also allowed at the same time.
> +    MOZ_ASSERT((securityFlags & (nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN |
> +                                 nsILoadInfo::SEC_REQUIRE_CORS |
> +                                 nsILoadInfo::SEC_REQUIRE_CORS_WITH_CREDENTIALS)) == 0,

Add a generic assertion, and runtime test, at the top that tests that one and only one of these flags are set. It makes the code hard to read when that test is spread out all over the rest of the code. Probably up where you're testing that there's a loadinfo at all.

@@ +78,5 @@
> +                                 nsILoadInfo::SEC_REQUIRE_CORS_WITH_CREDENTIALS)) == 0,
> +              "can not allow and require same origin at the same time");
> +
> +    bool allowIfInheritsPrincipal =
> +      securityFlags & nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;

I don't think this is right. I think what we want here is the new inherit-is-same-origin security flag.

@@ +91,5 @@
> +    // make sure that one of the bits is set to allow cross origin load
> +    MOZ_ASSERT(securityFlags & nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN ||
> +               securityFlags & nsILoadInfo::SEC_REQUIRE_CORS ||
> +               securityFlags & nsILoadInfo::SEC_REQUIRE_CORS_WITH_CREDENTIALS,
> +               "explicitly specify to allow cross origin");

Only one of these may be set at a time.

@@ +97,5 @@
> +    rv = nsContentUtils::GetSecurityManager()->
> +      CheckLoadURIWithPrincipal(loadingPrincipal,
> +                                channelURI,
> +        // jonas: how do we get those flags?
> +        // incorporate them into the seucrityFlags?

We'll have to make a call on a case-by-case basis.

I have no idea what to do with LOAD_IS_AUTOMATIC_DOCUMENT_REPLACEMENT and DONT_REPORT_ERRORS since I don't know how it works or where they're used.

I think ALLOW_CHROME will be a security flag yes.

My hope is that we can do something sane with DISALLOW_INHERIT_PRINCIPAL. Maybe by reusing the inherit-is-same-origin flag.

DISALLOW_SCRIPT might need to be a new flag. Our javascript: handling has changed a bunch over the years, so this might not be needed any more. Definitely need to get bz's help.

@@ +117,5 @@
> +                                loadingPrincipal,
> +                                corsWithCredentials);
> +      // jonas: what do we do about the second argument here?
> +      // http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.h#33
> +      rv = corsListener->Init(aChannel, DataURIHandling::Allow);

We should probably use the inherits-is-same-origin flag here.

@@ +128,5 @@
> +
> +  nsContentPolicyType contentPolicyType = loadInfo->GetContentPolicyType();
> +
> +  nsCString mimeTypeGuess;
> +  nsCOMPtr<nsISupports> requestingContext = loadInfo->LoadingNode();

leave this as null.

@@ +152,5 @@
> +    case nsIContentPolicy::TYPE_MEDIA:
> +      // mimeTypeGuess = EmptyCString();
> +      // jonas: I still don't fully understand what we want to do with the loadingContext
> +      // maybe you can explain based on that example of HTMLMediaElement
> +      // http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1162

The short answer is "do whatever it takes to make sure that we use the same context as we currently do".

For TYPE_MEDIA we seem to always use the element, so simply do |requestingContext = loadInfo->LoadingNode();| and assert that the LoadingNode() is an element and not a document.

Comment 13

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

Comment on attachment 8606492 [details] [diff] [review]
bug_1143922_add_asyncopen2_nsichannel.patch

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

::: dom/jsurl/nsJSProtocolHandler.cpp
@@ +678,5 @@
> +{
> +  nsCOMPtr<nsIStreamListener> corsListener;
> +  nsresult rv = nsContentSecurityManager::doContentSecurityCheck(this, aListener,
> +                                                                 getter_AddRefs(corsListener));
> +  NS_ENSURE_SUCCESS(rv, rv);

Actually, we should not make AsyncOpen2 return an error if the security check failed. Instead we should act like the open succeeds, but that the channel immediately fails.

It's better to have channels fail a single way, rather than sometimes fail by returning an error from AsyncOpen2 and other times return an error through OnStopRequest.

Comment 14

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

Comment on attachment 8606493 [details] [diff] [review]
bug_1143922_add_asyncopen2_htmlmedaielement.patch

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

::: dom/html/HTMLMediaElement.cpp
@@ +1248,5 @@
> +                     nsILoadInfo::SEC_REQUIRE_CORS_WITH_CREDENTIALS :
> +                     nsILoadInfo::SEC_REQUIRE_CORS;
> +  }
> +  else {
> +    securityFlags |= nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN;

securityFlags |=
  !ShouldCheckAllowOrigin() ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN :
  GetCORSMode() == CORS_USE_CREDENTIALS ? nsILoadInfo::SEC_REQUIRE_CORS_WITH_CREDENTIALS :
  nsILoadInfo::SEC_REQUIRE_CORS

@@ +1279,5 @@
>  
> +  // jonas? it seems on this callsite we either do CORS or CheckLoadURIWithPrincipal.
> +  // within nsContentSecurityMnager we decided to do
> +  // checkLoadURIWithPrincipal and potentially perform CORS
> +  // is that still correct?

Ah, corsListener->Init calls CheckLoadURI, so no need to do that separately when corsListener->Init is called.

Comment 15

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

Adding bz, especially in light of bug 1157451. For background see bug title and attachment 8606490 [details] [diff] [review].

We *could* require all places which sets one of the CORS modes to indicate if they should forbid or allow data:.

But I don't actually think that there's anything special about places where we use CORS. For non-cors loads we have the DISALLOW_INHERIT_PRINCIPAL for CheckLoadURI, which is very similar.

My proposal was to add a flag which indicates how to handle loads of URIs where the scheme is a principal-inheriting one. I.e. URIs which have URI_INHERITS_SECURITY_CONTEXT set.

My initial thought was to simply add a flag like nsILoadInfo::SEC_INHERITING_IS_SAME_ORIGIN. When set, this flag would cause all principal-inheriting loads to be treated as same-origin. I.e. DISALLOW_INHERIT_PRINCIPAL would not get set in calls to CheckLoadURI and we'd pass DataURIHandling::Allow if CORS is used. (As an aside, we should switch the CORS code to use URI_INHERITS_SECURITY_CONTEXT rather than hard-code data:.

Additionally my plan was to let nsILoadInfo::SEC_INHERITING_IS_SAME_ORIGIN, (together with a new nsILoadInfo::SEC_INHERET_FOR_ABOUT_BLANK if we need to) take over the responsibility of nsContentUtils::ChannelShouldInheritPrincipal. So when nsILoadInfo::SEC_INHERITING_IS_SAME_ORIGIN is set, we'd make GetChannelResultPrincipal call nsContentUtils::ChannelShouldInheritPrincipal and if that's true, return nsILoadInfo.triggeringPrincipal.


If nsILoadInfo::SEC_INHERITING_IS_SAME_ORIGIN is not set, the resource would be treated as being a null origin. This would cause any CORS or same-origin loads to fail, but would still allow SEC_ALLOW_CROSS_ORIGIN loads to succeed, but with GetChannelResultPrincipal returning a nullprincipal.


However this wouldn't *force* people to think about if data: should be allowed or not. One, arguably extreme, approach would be to have both nsILoadInfo::SEC_INHERITING_IS_SAME_ORIGIN and nsILoadInfo::SEC_INHERITING_IS_NULL_ORIGIN, and *require* that one of those are set.

But I'm not super excited about making the hundred or so callsites that we have in the tree set one of these flags in addition to same-origin policy flag.


Either way we could also have a nsILoadInfo::SEC_BLOCK_INHERITING_SCHEMES flag, though I'm not sure if the use cases for that are strong as long as we have the option to give inheriting URLs a nullprincipal.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> One, arguably extreme, approach would be to have both
> nsILoadInfo::SEC_INHERITING_IS_SAME_ORIGIN and
> nsILoadInfo::SEC_INHERITING_IS_NULL_ORIGIN, and *require* that one of those are set.

We could require that only if one of the CORS flags is set.  Otherwise default to the SEC_INHERITING_IS_NULL_ORIGIN behavior unless SEC_INHERITING_IS_SAME_ORIGIN is set.  That should give us something pretty similar to what we have now, ergonomics wise, right?

> Either way we could also have a nsILoadInfo::SEC_BLOCK_INHERITING_SCHEMES flag

We should look into which checkLoadURI callers pass the corresponding security manager flag now and see why they're doing it and whether they could make use of such a loadinfo flag instead.

Comment 17

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

Chatted with bz about which "modes" actually makes sense to have. It seems like it's just the following, but this needs to be verified.

SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS
Used by plain <img>, <video>, <link rel=stylesheet> etc.

SEC_REQUIRE_CORS_DATA_INHERITS
Used by <img crossorigin>, <video crossorigin>, XHR, fetch() etc.

SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL
Used in blink/webkit for <iframe>s. Likely also the mode that should be used by most Chrome code.

SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS/IS_BLOCKED
We didn't get into same-origin loads that much in our conversation. But we might need both DATA_IS_BLOCKED and DATA_INHERITS here. There are definitely existing callsites that do both, though I suspect many of them have the wrong policy when it comes to data: handling.


This doesn't include CORS requests that send credentials, but only XHR and fetch() does that. So we can probably support this by using SEC_REQUIRE_CORS_DATA_INHERITS and setting a separate securityflag. 


With that we end up with 4 or 5 modes. That's not too bad, and feels like few enough that we can require that gecko developers choose one of them any time a channel is created.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_loadinfo_changes.patch

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_securitymanager.patch

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_htmlmediaelement.patch

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Jonas, I think we are ready for another round of feedback. Getting all those flags right seems quite complicated at the moment. We have to make sure we are not missing anything. Anyway, I updated/added all the necessary flags to nsILoadInfo. I updated the securityManager to cancel the channel if any of the security checks fail. I think it's a better place to do that in the security manager than in each channel::asyncOpen2() implementation. I verified with necko folks that we should just call ->Cancel() on the channel. Pat Mcmanus is out this week. I cc'ed him on the bug and will flag him for feedback once he is back, just to make sure.

Comment 22

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

Comment on attachment 8613004 [details] [diff] [review]
bug_1143922_add_asyncopen2_loadinfo_changes.patch

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

::: netwerk/base/nsILoadInfo.idl
@@ +55,5 @@
> +  const unsigned long SEC_SANDBOXED = (1<<1);
> +
> +  /**
> +   * The following five flags determine what security checks are going to
> +   * be performed before opening the channel (SOP, CORS, ...):

change "before opening the channel" to "throughout the lifetime of the channel".

@@ +61,5 @@
> +   *    * SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED
> +   *    * SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS
> +   *    * SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL
> +   *    * SEC_REQUIRE_CORS_DATA_INHERITS
> +   * One of those five flags needs be set so channel->AsyncOpen2() can

"Exactly one of these flags are required to be set in order to allow the channel to perform the correct security checks and return the correct result principal.

If none or more than one of these flags are set AsyncOpen2 will fail."

@@ +81,5 @@
> +  /**
> +   * Allow loads from a different origin, but still consult the
> +   * scriptSecurityManager whether the page is allowed to perform
> +   * the request. E.g., check if the principal allows to access
> +   * schemes like chrome:, javascript:, data:.

These checks are *always* done, directly or indirectly. So no need to mention them specifically here.

Just say something like "Allow loads from other origins. Loads from data: will inherit the principal of the origin that triggered the load.".

@@ +90,5 @@
> +  /**
> +   * Allow loads from a different origin, but still consult the
> +   * scriptSecurityManager whether the page is allowed to perform
> +   * the request. E.g., check if the principal allows to access
> +   * schemes like chrome:, javascript:, data:.

"Allow loads from other origins. Loads from data: will be allowed, but the resulting resource will get a null principal."

@@ +97,5 @@
> +   */
> +  const unsigned long SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL = (1<<5);
> +
> +  /**
> +   * Allow loads from a different origin, but require CORS.

"Allow loads from any origin, but require CORS for cross-origin loads. Loads from data: are allowed and the result will inherit the principal of the origin that triggered the load."

@@ +105,5 @@
> +  const unsigned long SEC_REQUIRE_CORS_DATA_INHERITS = (1<<6);
> +
> +  /**
> +   * Use this flag in addition to SEC_REQUIRE_CORS_DATA_INHERITS
> +   * to indicate whether CORS should be performed with credentials.

Use this flag in addition to SEC_REQUIRE_CORS_DATA_INHERITS to make cross-origin CORS loads happen with credentials (such as cookies and client side certs).

Comment 23

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

Comment on attachment 8613005 [details] [diff] [review]
bug_1143922_add_asyncopen2_securitymanager.patch

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

::: dom/security/nsContentSecurityManager.cpp
@@ +61,5 @@
> +  MOZ_ASSERT((enforceSecurityFlag == nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS) ||
> +             (enforceSecurityFlag == nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED) ||
> +             (enforceSecurityFlag == nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS) ||
> +             (enforceSecurityFlag == nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL) ||
> +             (enforceSecurityFlag == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS),

This should be a runtime check. I.e. not a debug-only check.

Also add a test that checks that SEC_REQUIRE_CORS_WITH_CREDENTIALS is only set if |enforceSecurityFlag == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS|

@@ +87,5 @@
> +  nsCOMPtr<nsIPrincipal> loadingPrincipal = loadInfo->LoadingPrincipal();
> +  nsCOMPtr<nsIURI> channelURI;
> +  nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(channelURI));
> +  if (NS_FAILED(rv)) {
> +    aChannel->Cancel(rv);

Are you *sure* that calling Cancel on a channel before AsyncOpen has been called will work for all channels? Looking at the code it didn't look that way.

You should probably add another level of function nesting here so that you don't need to duplicate this code everywhere you have an early return. It's very easy to miss a place, especially as more code will be added over time.

@@ +115,5 @@
> +    // @arg nsIScriptSecurityManager::STANDARD
> +    // lets use STANDARD for now and evaluate on a callsite basis, see also:
> +    // http://mxr.mozilla.org/mozilla-central/source/caps/nsIScriptSecurityManager.idl#62
> +    rv = nsContentUtils::GetSecurityManager()->
> +      CheckLoadURIWithPrincipal(loadingPrincipal,

You don't need to do this if |enforceSecurityFlag == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS| since the CORS code will do this check.

@@ +174,5 @@
> +    // alias nsIContentPolicy::TYPE_DATAREQUEST:
> +    case nsIContentPolicy::TYPE_OBJECT_SUBREQUEST:
> +    case nsIContentPolicy::TYPE_DTD:
> +    case nsIContentPolicy::TYPE_FONT:
> +      break;

Might want to add an assertion here which indicates that this code is not ready for other contentPolicy types yet.

::: dom/security/nsContentSecurityManager.h
@@ +18,5 @@
> +
> +public:
> +  static nsresult doContentSecurityCheck(nsIChannel* aChannel,
> +  	                                     nsIStreamListener* aStreamListener,
> +  	                                     nsIStreamListener** outCorsListener);

Fix whitespace

Comment 24

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

Comment on attachment 8606492 [details] [diff] [review]
bug_1143922_add_asyncopen2_nsichannel.patch

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

::: netwerk/base/nsIChannel.idl
@@ +182,5 @@
>  
> +    /**
> +     * Performs content security check and calls asyncOpen().
> +     */
> +    void asyncOpen2(in nsIStreamListener aListener, in nsISupports aContext);

Remove the context argument. We almost never use it in gecko and it can always be provided through other means by the caller. So might as well get rid of this uselessness as part of this change.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Pat, we are about to move security checks into AsyncOpen2(), which then will perform the security checks (SOP, CORS, CSP, MCB, etc) and then calls the current AsyncOpen() on the channel.

Jonas suggested it might be a good idea to:
a) let channels fail the same way throughout Necko. Hence the question, can we do that within the securityManager (see attached wip-patches) by calling channel->Cancel() even though the channel hasn't been openend yet? If so, what should be the argument to cancel? NS_ABORT_BINDING? And should AsyncOpen2() return the error, or should asyncOpen2() always return NS_OK? What is the best way of handling the scenario when the newly added securityManager has to abort a load (cancel a channel)?

b) remove aContext from AsyncOpen2() (See Comment 24). While I agree, I just wanted to make sure we are not missing anything.

Thanks!

Comment 26

[Patrick McManus [:mcmanus]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25)
> Pat, we are about to move security checks into AsyncOpen2(), which then will
> perform the security checks (SOP, CORS, CSP, MCB, etc) and then calls the
> current AsyncOpen() on the channel.
> 
> Jonas suggested it might be a good idea to:
> a) let channels fail the same way throughout Necko. Hence the question, can
> we do that within the securityManager (see attached wip-patches) by calling
> channel->Cancel() even though the channel hasn't been openend yet? If so,
> what should be the argument to cancel? NS_ABORT_BINDING? And should
> AsyncOpen2() return the error, or should asyncOpen2() always return NS_OK?
> What is the best way of handling the scenario when the newly added
> securityManager has to abort a load (cancel a channel)?

you can cancel the channel independently of whether open*() returns OK or not, and it should be fine to cancel it with open on the stack. I think all of these things currently happen under different circumstances.

If open does throw an error you should release the listeners and make sure onstart/onstop are not called, and vice versa if you return OK.

> 
> b) remove aContext from AsyncOpen2() (See Comment 24). While I agree, I just
> wanted to make sure we are not missing anything.
> 
> Thanks!

that's fine. but you're still using the normal onstart/ondata/onstop methods, so document that it will be null in those callbacks if using asyncopen2

Comment 27

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

(In reply to Patrick McManus [:mcmanus] from comment #26)
> If open does throw an error you should release the listeners and make sure
> onstart/onstop are not called, and vice versa if you return OK.

Right, this is what I think the current patch fails to do.

I.e. when the security checks fail we should return OK and then later call onstart/onstop. Which is not what's currently happening.

Potentially one way to ensure that this works is to make AsyncOpen2 call Cancel() *after* calling AsyncOpen() for the case when security checks failed. But I'm not sure if that might end up making us hit the network even when we shouldn't.

Comment 28

[Patrick McManus [:mcmanus]]

(In reply to Jonas Sicking (:sicking) from comment #27)
> (In reply to Patrick McManus [:mcmanus] from comment #26)
>
> 
> Potentially one way to ensure that this works is to make AsyncOpen2 call
> Cancel() *after* calling AsyncOpen() for the case when security checks
> failed. But I'm not sure if that might end up making us hit the network even
> when we shouldn't.

right now the point of no return for calling cancel before any on-the-wire networking happens is http-on-modify-request.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_nsichannel.patch

Pat, summarizing Jonas and your comments this is what should happen, right?

Within AsyncOpen2()
* Call Security Checks
* Call AsyncOpen()
* If AsyncOpen failed - return failure
* If Security checks failed, call cancel() on the channel but still return NS_OK.

That's what I am doing at the moment. Can you take a quick look at the patch and tell me/us if we are on the right track or if we are still missing something?

Thanks!

Comment 30

[Patrick McManus [:mcmanus]]

Comment on attachment 8614983 [details] [diff] [review]
bug_1143922_add_asyncopen2_nsichannel.patch

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

what you are doing seems ok from a contract/idl pov, but as jonas points out it could cause some on the wire networking to happen that you probably don't want in the face of a security error.

If AsyncOpen returns NS_OK that means the channel has already been passed to the socket thread and it is off asynchronously trying to do the work of the channel. You can cancel it, but that won't undo the ethernet action.

It seems that if you know you have a security error in AsyncOpen2 you should avoid calling AsyncOpen() at all (is there a reason to call it?)..

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Patrick McManus [:mcmanus] from comment #30)
> what you are doing seems ok from a contract/idl pov, but as jonas points out
> it could cause some on the wire networking to happen that you probably don't
> want in the face of a security error.
> 
> If AsyncOpen returns NS_OK that means the channel has already been passed to
> the socket thread and it is off asynchronously trying to do the work of the
> channel. You can cancel it, but that won't undo the ethernet action.
> 
> It seems that if you know you have a security error in AsyncOpen2 you should
> avoid calling AsyncOpen() at all (is there a reason to call it?)..

Hi Pat, thanks for the info. We definitely do not want any on the wire networking to happen if security checks fail. I have been discussing things with Steve today, and he suggest to do something like the following, but then we are not sure about the contract/idl and whether we have to call other clean up functionality, like e.g. ReleaseListeners().

> NS_IMETHODIMP
> nsHttpChannel::AsyncOpen2(nsIStreamListener *aListener)
> {
>  nsCOMPtr<nsIStreamListener> corsListener;
>  nsresult rv =
>    nsContentSecurityManager::doContentSecurityCheck(this, aListener,
>                                                     getter_AddRefs(corsListener));
>  if (NS_FAILED(rv)) {
>    this->Cancel(rv);
>    return rv;
>  }
>  if (corsListener) {
>    return AsyncOpen(corsListener, nullptr)
>  }
>  return AsyncOpen(aListener, nullptr);
> }

Would that work? Would we also have to release the listeners in such a case? What we would like to avoid is to actually add some code to ::asyncOpen(). Is there a nice and clean way to just add code to ::AsyncOpen2() which would still do all the relevant things we want to accomplish here?

I am not sure, but looking at the nsiChannel.idl:
>     * after asyncOpen returns.  If asyncOpen returns successfully, the
>     * channel promises to call at least onStartRequest and onStopRequest.
it looks like we could use the approach (code) from above, because if asyncOpen2() returns an error, then onStartRequest() and onStopRequest() are *not* promised to be called, right? Or am I missing something here?

Comment 32

[Patrick McManus [:mcmanus]]

comment 31 lgtm

> Would that work? Would we also have to release the listeners in such a case?

do you mean in the case of the contentsecuritycheck() failing? no, you wouldn't have to release them because the only reference to them is in corsListener which presumably drops them when it goes out of scope. In the other path, the corsListener becomes the listener itself.

> 
> I am not sure, but looking at the nsiChannel.idl:
> >     * after asyncOpen returns.  If asyncOpen returns successfully, the
> >     * channel promises to call at least onStartRequest and onStopRequest.
> it looks like we could use the approach (code) from above, because if
> asyncOpen2() returns an error, then onStartRequest() and onStopRequest() are
> *not* promised to be called, right? Or am I missing something here?

That's right. If *Open() fails, then the onStart/onData/onAvail callbacks do not happen (and likewise if *Open succeeds then they MUST happen). I'll warn you that various channels have gotten this wrong over time, but I think we're all ok right now.

Comment 33

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

The code in comment 31 does not look good to me. First off, the call to Cancel() doesn't seem to accomplish anything? But more importantly it's causing us to fail my second goal below.

What I want to accomplish is two things:

1. Maintain current nsIChannel contract. I.e. that onstart/onstop is called when and only when
   asyncopen/asyncopen2 returns a success code.

2. The security checks that we do up front here (CheckMayLoad, nsIContentPolicy, CheckLoadURI etc) should
   *not* cause AsyncOpen2() to return an error code.


The reason for 1 is that most likely lots of code depends on 1. It's also a good contract that I see no reason to change.

The reason for 2 is that I think it's bad if some security checks cause the channel to fail one way, but other checks cause the channel to fail another way. It's more likely to result in bugs as we add and change security checks and other forms of policies.

Specifically, I'd like to see a lot of these security checks move to happen in the parent process in a not too distant future. The fact that we're calling nsIContentPolicy from the child process results in a lot of performance-costing synchronous IPC.

It'll also give us much more freedom to do more security checks in the future, if we've already established an asynchronous contract here.


So, all we need to accomplish is that when doContentSecurityCheck returns an error that we still make AsyncOpen2 return NS_OK. And that we later call onstart/onstop with the error code that doContentSecurityCheck returned.

This doesn't seem like rocket science, but maybe I'm wrong?

Would simply removing the "return rv" from comment 31 give that result? I.e. what will happen if you call Cancel(error) on a channel and *then* call AsyncOpen? Will that prevent the channel from hitting the network? Will it cause onstart/onstop to be called as we want?

Comment 34

[Patrick McManus [:mcmanus]]

fwiw cancelling the channel when returning a sync error from asyncopen2() is probably good practice - it sets the error code on the channel in case anyone calls getstatus() for one thing..

(In reply to Jonas Sicking (:sicking) from comment #33)
>
> The reason for 2 is that I think it's bad if some security checks cause the
> channel to fail one way, but other checks cause the channel to fail another
> way. It's more likely to result in bugs as we add and change security checks
> and other forms of policies.

Its already true that a channel can fail either synchronously at open() time or asynchronously in onstart/onstop.. I don't see that implementing this feature one way or the other saves any user of the api any work (or makes the results more reliable) as both need to be supported now for proper operation.

That being said I don't have a particular preference for the sync version posted here and your note about the parent process is good.

 
> It'll also give us much more freedom to do more security checks in the
> future, if we've already established an asynchronous contract here.
> 

you already have 2 established paths to choose from. using sync here doesn't mean new checks can't coexist and use async, does it?


> So, all we need to accomplish is that when doContentSecurityCheck returns an
> error that we still make AsyncOpen2 return NS_OK. And that we later call
> onstart/onstop with the error code that doContentSecurityCheck returned.
> 
> This doesn't seem like rocket science, but maybe I'm wrong?

that would work too.

> 
> Would simply removing the "return rv" from comment 31 give that result? I.e.
> what will happen if you call Cancel(error) on a channel and *then* call
> AsyncOpen? Will that prevent the channel from hitting the network? Will it
> cause onstart/onstop to be called as we want?

I think it would work.. it would do a chunk of work before getting to the onstop/onstart, including probably calling on-modify-request observers. An early check could be put in easily enough.

I would worry more about the less heavily used channel implementations.. I doubt they were constructed to consider a channel being canceled before being opened.

Comment 35

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

I guess it's a valid point that in theory all code paths should be able to handle both synchronous and asynchronous errors. Though I know that this is something that has bitten us in the past. In particular callers tend to not do the right thing when they receive a synchronous error (many throw exceptions when they per specs should fire an error event).

But I can live with failing synchronously.

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_loadinfo_changes.patch

Added some attributes so the functionality is within the loadinfo so consumers of any securityflags don't have to do any bit-arithmetic, which is always error prone.

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_securitymanager.patch

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_scriptSecurityManager.patch

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_nsichannel.patch

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_htmlmediaelement.patch

Comment 41

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

Comment on attachment 8616225 [details] [diff] [review]
bug_1143922_add_asyncopen2_loadinfo_changes.patch

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

::: netwerk/base/nsILoadInfo.idl
@@ +43,5 @@
> +  /*
> +   * Enforce the same origin policy where data: loads inherit
> +   * the principal.
> +   */
> +  const unsigned long SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS = (1<<0);

These don't all need to be separate bits, since only one of them should be set.

So you could do

SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS = 1
SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED = 2
SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS = 3
SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL = 4
SEC_REQUIRE_CORS_DATA_INHERITS = 5

SEC_REQUIRE_CORS_WITH_CREDENTIALS = (1 << 3)
SEC_FORCE_INHERIT_PRINCIPAL = (1 << 4)
etc

@@ +202,5 @@
>    /**
> +   * Helpers to query only the contentSecurityFlags defined above and
> +   * also to determine whether to enforce the SOP or CORS.
> +   */
> +  [infallible] readonly attribute unsigned long contentSecurityFlags;

I wonder if we should call this "security mode" or some such. "content" isn't particularly descriptive. So something like "securityMode" would be my preference.

@@ +204,5 @@
> +   * also to determine whether to enforce the SOP or CORS.
> +   */
> +  [infallible] readonly attribute unsigned long contentSecurityFlags;
> +  [infallible] readonly attribute boolean requireSameOrigin;
> +  [infallible] readonly attribute boolean allowCrossOrigin;

I don't feel strongly, but are there really usecases for these?

@@ +213,5 @@
> +  [infallible] readonly attribute boolean requireSameOriginDataInherits;
> +  [infallible] readonly attribute boolean requireSameOriginDataIsBlocked;
> +  [infallible] readonly attribute boolean allowCrossOriginDataInherits;
> +  [infallible] readonly attribute boolean allowCrossOriginDataIsNull;
> +  [infallible] readonly attribute boolean requireCorsDataInherits;

Again, I don't feel strongly, but is it really better/simpler that people use these, than that they write
|.securityMode == SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS|?

@@ +216,5 @@
> +  [infallible] readonly attribute boolean allowCrossOriginDataIsNull;
> +  [infallible] readonly attribute boolean requireCorsDataInherits;
> +
> +  /**
> +   * Determines whether CORS should be enforced with credentials.

"Determines whether credentials are sent with CORS requests."

Comment 42

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

Comment on attachment 8616228 [details] [diff] [review]
bug_1143922_add_asyncopen2_securitymanager.patch

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

r=me

::: dom/security/nsContentSecurityManager.cpp
@@ +113,5 @@
> +    // let's set the outgoing argument.
> +    corsListener.forget(outCorsListener);
> +  }
> +
> +  // ======================================================

Another check we should do is

CheckLoadURIWithPrincipal(triggeringPrincipal,
                          channelURI,
                          nsIScriptSecurityManager::STANDARD);

Obviously this only needs to be done when |loadingPrincipal != triggeringPrincipal|

::: dom/security/nsContentSecurityManager.h
@@ +18,5 @@
> +
> +public:
> +  static nsresult doContentSecurityCheck(nsIChannel* aChannel,
> +                                         nsIStreamListener* aStreamListener,
> +                                         nsIStreamListener** outCorsListener);

I think it would be cleaner to do 

doContentSecurityCheck(nsIChannel* aChannel,
                       nsCOMPtr<nsIStreamListener*>& aStreamListener);

That way you can simply have doContentSecurityCheck modify the nsCOMPtr to point to the CORS listener if needed.

Comment 43

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

Comment on attachment 8616229 [details] [diff] [review]
bug_1143922_add_asyncopen2_scriptSecurityManager.patch

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

::: caps/nsScriptSecurityManager.cpp
@@ +368,5 @@
> +    aChannel->GetLoadInfo(getter_AddRefs(loadInfo));
> +    if (loadInfo) {
> +        nsCOMPtr<nsIPrincipal> triggeringPrincipal = loadInfo->TriggeringPrincipal();
> +        bool aInheritForAboutBlank = loadInfo->GetAboutBlankInherits();
> +        bool aForceInheritPrincipal = loadInfo->GetForceInheritPrincipal();

This function should not honor the "force inherit" flag. That is handled by GetChannelResultPrincipal.

In fact, the reason we added GetChannelURIPrincipal was to have a function which was not affected by the force-inherit flag.

However you should the "mode" flags. I.e. all of this stuff should only be done if SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS, SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS or SEC_REQUIRE_CORS_DATA_INHERITS is set.

And if SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED or SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL is set, then you should set the principal to a null principal rather than to the triggeringPrincipal.

Comment 44

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

Comment on attachment 8616231 [details] [diff] [review]
bug_1143922_add_asyncopen2_htmlmediaelement.patch

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

::: dom/html/HTMLMediaElement.cpp
@@ +1154,5 @@
>    if (docShell && !docShell->GetAllowMedia()) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  nsresult rv;

Set to NS_OK. Or better yet, only declare it where it's first used if possible.

@@ +1226,5 @@
> +  //                                                   false, // aInheritForAboutBlank
> +  //                                                   false // aForceInherit
> +  //                                                   )) {
> +  //   securityFlags = nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;
> +  // }

Remove this

@@ +1231,5 @@
> +
> +  // determine what security checks need to be performed in AsyncOpen2().
> +  securityFlags |=
> +    ShouldCheckAllowOrigin() ? nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS :
> +                               nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS;

Just do

nsSecurityFlags securityFlags =
    ShouldCheckAllowOrigin() ? ...

Comment 45

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

Comment on attachment 8616228 [details] [diff] [review]
bug_1143922_add_asyncopen2_securitymanager.patch

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

::: dom/security/nsContentSecurityManager.cpp
@@ +96,5 @@
> +  // ======================================================
> +  // (3) Peform CORS check
> +  else {
> +    // do we want something more aggressive than NS_ENSURE_ARG_POINTER?
> +    NS_ENSURE_ARG_POINTER(outCorsListener);

You should also check that aStreamListener is non-null. For calls to nsIChannel.Open2() we'll probably fail to do proper security checks otherwise.

It should be quite fine to not support CORS with synchronous loads.

Comment 46

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

Comment on attachment 8616230 [details] [diff] [review]
bug_1143922_add_asyncopen2_nsichannel.patch

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

::: dom/jsurl/nsJSProtocolHandler.cpp
@@ +682,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (corsListener) {
> +    return AsyncOpen(corsListener, nullptr);
> +  }
> +  return AsyncOpen(aListener, nullptr);

With the suggested signature change to nsContentSecurityManager::doContentSecurityCheck, this could would simply become:

nsCOMPtr<nsIStreamListener> listener = aListener;
nsresult rv = nsContentSecurityManager::doContentSecurityCheck(this,
                listener);
NS_ENSURE_SUCCESS(rv, rv);

return AsyncOpen(listener, nullptr);

Comment 47

[Patrick McManus [:mcmanus]]

Comment on attachment 8616228 [details] [diff] [review]
bug_1143922_add_asyncopen2_securitymanager.patch

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

::: dom/security/nsContentSecurityManager.h
@@ +18,5 @@
> +
> +public:
> +  static nsresult doContentSecurityCheck(nsIChannel* aChannel,
> +                                         nsIStreamListener* aStreamListener,
> +                                         nsIStreamListener** outCorsListener);

+1

Comment 48

[Patrick McManus [:mcmanus]]

Comment on attachment 8616230 [details] [diff] [review]
bug_1143922_add_asyncopen2_nsichannel.patch

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

r+ with change to nsContentSecurityManager::doContentSecurityCheck handling.

thanks christoph!

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_loadinfo_changes.patch

Comment 50

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_securitymanager.patch

Jonas, thanks for all of your feedback. I factored out a lot of functionality and also incprorated the CheckLoadURIWithPrincipal in case loadingPrincipal and triggeringPrincipal are different. Would be great if you could take another look. In particular I am not sure whether this line is correct:
> aInAndOutListener = corsListener;
I think it should be correct to just assign the corsListener to the reference of the nsComptr, right?

Comment 51

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_scriptSecurityManager.patch

(In reply to Jonas Sicking (:sicking) from comment #43)
> Comment on attachment 8616229 [details] [diff] [review]
> However you should the "mode" flags. I.e. all of this stuff should only be
> done if SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS,
> SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS or SEC_REQUIRE_CORS_DATA_INHERITS is
> set.
>
> And if SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED or
> SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL is set, then you should set the
> principal to a null principal rather than to the triggeringPrincipal.

So, I am slightly confused on what to do here. Should the check to ChannelShouldInheritPrincipal always happen or just if any of the *_DATA_INHERITS flags are set? Or do you mean it depends on those 3 flags whether we should call it using the triggeringPrincipal versus the nullPrincipal?

Comment 52

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_nsichannel.patch

Changed all callsites to only use one argument as an InAndOut argument instead of passing 2 separate arguemnts. Carrying over r+ from mcmanus.

Comment 53

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_htmlmediaelement.patch

Comment 54

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

Comment on attachment 8616895 [details] [diff] [review]
bug_1143922_add_asyncopen2_securitymanager.patch

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

::: dom/security/nsContentSecurityManager.cpp
@@ +23,5 @@
> +  }
> +
> +  // if CORS is not used, then all good
> +  if (!aLoadInfo->GetRequireCorsWithCredentials()) {
> +    return NS_OK;

Nit, might be more future-proof to reverse this check and do:

if (aLoadInfo->GetRequireCorsWithCredentials()) {
  ... check that securityMode == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS ...
}


That way more checks can be added at the end of the function if/when more flag checks are added.

Up to you.

@@ +36,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +permitsSOP(nsIURI* aURI, nsILoadInfo* aLoadInfo)

These function names make it very hard to do understand what the functions do.

Maybe something like doSOPChecks, doCheckLoadURIChecks, doCORSChecks etc?

@@ +53,5 @@
> +  nsresult rv = loadingPrincipal->CheckMayLoad(aURI,
> +                                               true, // report to console
> +                                               sameOriginDataInherits);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  return NS_OK;

Just do 'return loadingPrincipal->...'

::: dom/security/nsContentSecurityManager.h
@@ +17,5 @@
> +  virtual ~nsContentSecurityManager() {}
> +
> +public:
> +  static nsresult doContentSecurityCheck(nsIChannel* aChannel,
> +  	                                     nsCOMPtr<nsIStreamListener>& aInAndOutListener);

Fix whitespace.

Comment 55

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

Comment on attachment 8616896 [details] [diff] [review]
bug_1143922_add_asyncopen2_scriptSecurityManager.patch

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

Looks good otherwise

::: caps/nsScriptSecurityManager.cpp
@@ +375,5 @@
> +            bool aInheritForAboutBlank = loadInfo->GetAboutBlankInherits();
> +            if ((nsContentUtils::ChannelShouldInheritPrincipal(triggeringPrincipal,
> +                                                               uri,
> +                                                               aInheritForAboutBlank,
> +                                                               true))) {

The last argument should be 'false', shouldn't it?

Comment 56

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8616895 [details] [diff] [review]
bug_1143922_add_asyncopen2_securitymanager.patch

Tanvi, maybe you wanna take a look at this patch as well. Thanks!

Comment 57

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

Comment on attachment 8616896 [details] [diff] [review]
bug_1143922_add_asyncopen2_scriptSecurityManager.patch

Should also get bz's review on this.

Also, I wonder if we need to put these changes in GetChannelResultPrincipal rather than GetChannelURIPrincipal. In particular I'm worried that we'll re-break the classifier in the tracking protection code.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8616896 [details] [diff] [review]
bug_1143922_add_asyncopen2_scriptSecurityManager.patch

This doesn't make sense.  Why would you ever call ChannelShouldInheritPrincipal with a literal true for the aForceInherit argument?

Comment 59

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8616896 [details] [diff] [review]
bug_1143922_add_asyncopen2_scriptSecurityManager.patch

Also, why is this change in GetChannelURIPrincipal and not in GetChannelResultPrincipal?  

And aInheritForAboutBlank should have a name that doesn't look like a function argument.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8616895 [details] [diff] [review]
bug_1143922_add_asyncopen2_securitymanager.patch

Using a mutable nsCOMPtr ref argument is really not a pattern we want to perpetuate.  It makes callers very confusing to read.  Better to have an explicit nsIStreamListener** outparam.

And typical DOM style is to have function names start with a capital letter.

Comment 61

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8616898 [details] [diff] [review]
bug_1143922_add_asyncopen2_htmlmediaelement.patch

r=me.  Nice to see this coming together!

Comment 62

[Tanvi Vyas[:tanvi]]

Comment on attachment 8616891 [details] [diff] [review]
bug_1143922_add_asyncopen2_loadinfo_changes.patch

+  /**
+   * "Allow loads from other origins. Loads from data: will be allowed,
Remove leading "

+   * but the resulting resource will get a null principal.
+   * Used in blink/webkit for <iframe>s. Likely also the mode
+   * that should be used by most Chrome code.
+   */
+  const unsigned long SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL = (1<<3);

Comment 63

[Tanvi Vyas[:tanvi]]

Comment on attachment 8616895 [details] [diff] [review]
bug_1143922_add_asyncopen2_securitymanager.patch

+  if (aLoadInfo->GetSecurityMode() != nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS) {
+    // XXX: @arg nsIScriptSecurityManager::STANDARD
+    // lets use STANDARD for now and evaluate on a callsite basis, see also:
+    // http://mxr.mozilla.org/mozilla-central/source/caps/nsIScriptSecurityManager.idl#62
+    rv = nsContentUtils::GetSecurityManager()->
+      CheckLoadURIWithPrincipal(loadingPrincipal,
+                                aURI,
+                                nsIScriptSecurityManager::STANDARD);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }

Didn't Jonas say that if CheckLoadURIWithPrincipal only needs to be called on non-same-origin loads?  Line 171 here https://etherpad.mozilla.org/RevampGeckoSecHooks

Comment 64

[Tanvi Vyas[:tanvi]]

Comment on attachment 8616896 [details] [diff] [review]
bug_1143922_add_asyncopen2_scriptSecurityManager.patch

(In reply to Jonas Sicking (:sicking) from comment #57)
> Comment on attachment 8616896 [details] [diff] [review]
> bug_1143922_add_asyncopen2_scriptSecurityManager.patch
> 
> Should also get bz's review on this.
> 
> Also, I wonder if we need to put these changes in GetChannelResultPrincipal
> rather than GetChannelURIPrincipal. In particular I'm worried that we'll
> re-break the classifier in the tracking protection code.

This patch doesn't make sense to me.  Why are we changing the way GetChannelURIPrincipal works?  Per https://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#344, it shouldn't be affected by inheritance.  I think changes belong in GetChannelResultPrincipal instead.

Comment 65

[Tanvi Vyas[:tanvi]]

Comment on attachment 8616898 [details] [diff] [review]
bug_1143922_add_asyncopen2_htmlmediaelement.patch

-  nsSecurityFlags securityFlags = nsILoadInfo::SEC_NORMAL;
-  if (nsContentUtils::ChannelShouldInheritPrincipal(NodePrincipal(),
-                                                    mLoadingSrc,
-                                                    false, // aInheritForAboutBlank
-                                                    false // aForceInherit
-                                                    )) {
-    securityFlags = nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;
+  // determine what security checks need to be performed in AsyncOpen2().
+  nsSecurityFlags securityFlags =
+    ShouldCheckAllowOrigin() ? nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS :
+                               nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS;
+
+  if (GetCORSMode() == CORS_USE_CREDENTIALS) {
+    securityFlags |= nsILoadInfo::SEC_REQUIRE_CORS_WITH_CREDENTIALS;
   }

In an earlier version of this patch you wrote:
  // Since we move that check to the GetChannelURIPrincipal, how do we
  // determine the SEC_FORCE_INHERIT_PRINCIPAL.
I have the same question.  And I don't think we want to move this check to GetChannelURIPrincipal.

Also, since this is the first time we're moving security checks, can you do a few prints locally to confirm that the nodes/principals used when security checks are called in AsyncOpen2 are the same ones that were originally used in the direct call to content policies?

Comment 66

[Tanvi Vyas[:tanvi]]

Comment on attachment 8616897 [details] [diff] [review]
bug_1143922_add_asyncopen2_nsichannel.patch

Hi Christoph,
Are you sure about these:

 NS_IMETHODIMP
+nsViewSourceChannel::Open2(nsIInputStream** aStream)
+{
+    // no need to perform a ContentSecurityCheck here, performed by the
+    // underlying channel (see mChannel).
+    return Open(aStream);
+}
+

+NS_IMETHODIMP
+nsViewSourceChannel::AsyncOpen2(nsIStreamListener *aListener)
+{
+    // no need to perform a ContentSecurityCheck here, performed by the
+    // underlying channel (see mChannel).
+    return AsyncOpen(aListener, nullptr);
+}

Comment 67

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

I agree about Tanvi's comment above. Aren't we calling AsyncOpen, not AsyncOpen2 on the underlying channel, causing no security checks to happen.

Actually, that makes me realize that we might have the same problem on redirects. If a callsite in Gecko creates a channel and sets the REQUIRE_SAME_ORIGIN flag, and then call AsyncOpen2, then we're only going to do the same-origin check on the initial URI, right?

If the channel is redirected, then won't the http-channel implementation create a new channel but then call AsyncOpen on that new channel? And so nothing is stopping the redirect from going to another origin.

I think we need to set a boolean in the channel which indicates "please do security checks on subsequent channels". If that boolean is set, we'd call AsyncOpen2 on any inner or redirected channels. Only tricky part is that we wouldn't want to add additional CORS listeners, or call nsIContentPolicy on redirects (since that likely will break addons).

Comment 68

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_loadinfo_changes.patch

> I think we need to set a boolean in the channel which indicates "please do
> security checks on subsequent channels". If that boolean is set, we'd call
> AsyncOpen2 on any inner or redirected channels. Only tricky part is that we
> wouldn't want to add additional CORS listeners, or call nsIContentPolicy on
> redirects (since that likely will break addons).

Jonas, before I am going to incorporate the other comments/reviews/recommendations I was wondering if we could summarize the changes:
a) In this patch I added (in addition to what already was r+) the 'useAsyncOpen2' flag, which has a toggle function in the *.idl and a readonly getter to make sure that once the flag is toggled to true, no one can ever change that back. Agreed?

b) Where should we set that flag? Either within ::AsyncOpen2() or within the securityManager. I am more for the second approach, although it has the downside that we would toggle the flag to true again after a redirect. In other words, we would call 'indicateUseOfAsyncOpen2' potentially several times for every redirect.

c) Whenever the channel undergoes a redirect we have to check that flag and based on that call AsyncOpen or asyncOpen2(). I have to investigate those callsites. The same is true for inner channels.

d) Now that we have the redirectChain available within the loadInfo, we could check the size of that array and do *not* create a new CORS listener in the securityManager if != 0.

e) Separate redirectHandlers (::AsyncOnChannelRedirect) within CSP and Mixed Content Blocker should return NS_OK right away without consulting CSP and MCB, because now security checks after redirects are performed within asyncOpen2, right?

f) Everything else should be fine, right? At least once I have addressed the other comments/reviews in this bug, or am I missing something?

Comment 69

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

Comment on attachment 8627523 [details] [diff] [review]
bug_1143922_add_asyncopen2_loadinfo_changes.patch

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

Looks good otherwise.

::: netwerk/base/LoadInfo.cpp
@@ +87,5 @@
>                     nsContentPolicyType aContentPolicyType,
>                     uint64_t aInnerWindowID,
>                     uint64_t aOuterWindowID,
>                     uint64_t aParentOuterWindowID,
> +                   uint64_t aUseAsyncOpen2,

Make this a 'bool' instead.

::: netwerk/base/LoadInfo.h
@@ +61,5 @@
>             nsContentPolicyType aContentPolicyType,
>             uint64_t aInnerWindowID,
>             uint64_t aOuterWindowID,
>             uint64_t aParentOuterWindowID,
> +           uint64_t aUseAsyncOpen2,

'bool'

@@ +80,5 @@
> +  nsCOMPtr<nsIURI>             mBaseURI;
> +  uint64_t                     mInnerWindowID;
> +  uint64_t                     mOuterWindowID;
> +  uint64_t                     mParentOuterWindowID;
> +  uint64_t                     mUseAsyncOpen2;

'bool'

::: netwerk/base/nsILoadInfo.idl
@@ +330,5 @@
> +   * remain true throughout the lifetime of the channel.
> +   * Hence, lets provide a set function and a readonly
> +   * attribute to query whether the flag was ever set.
> +   */
> +  void indicateUseOfAsyncOpen2();

Alternatively, you can make the attribute settable, but make the setter crash if someone tries to set to false.

@@ +332,5 @@
> +   * attribute to query whether the flag was ever set.
> +   */
> +  void indicateUseOfAsyncOpen2();
> +
> +  readonly attribute unsigned long long useAsyncOpen2;

boolean.

It might also be nice to name this 'enfoceSecurity' or some such.

@@ +335,5 @@
> +
> +  readonly attribute unsigned long long useAsyncOpen2;
> +
> +%{ C++
> +  inline uint64_t GetUseAsyncOpen2()

bool

Comment 70

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

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #68)
> Created attachment 8627523 [details] [diff] [review]
> bug_1143922_add_asyncopen2_loadinfo_changes.patch
> 
> > I think we need to set a boolean in the channel which indicates "please do
> > security checks on subsequent channels". If that boolean is set, we'd call
> > AsyncOpen2 on any inner or redirected channels. Only tricky part is that we
> > wouldn't want to add additional CORS listeners, or call nsIContentPolicy on
> > redirects (since that likely will break addons).
> 
> Jonas, before I am going to incorporate the other
> comments/reviews/recommendations I was wondering if we could summarize the
> changes:
> a) In this patch I added (in addition to what already was r+) the
> 'useAsyncOpen2' flag, which has a toggle function in the *.idl and a
> readonly getter to make sure that once the flag is toggled to true, no one
> can ever change that back. Agreed?

Yes.

> b) Where should we set that flag? Either within ::AsyncOpen2() or within the
> securityManager. I am more for the second approach, although it has the
> downside that we would toggle the flag to true again after a redirect. In
> other words, we would call 'indicateUseOfAsyncOpen2' potentially several
> times for every redirect.

In the securityManager seems fine. Doesn't matter if the flag is set to true multiple times.

> c) Whenever the channel undergoes a redirect we have to check that flag and
> based on that call AsyncOpen or asyncOpen2(). I have to investigate those
> callsites. The same is true for inner channels.

Indeed.

> d) Now that we have the redirectChain available within the loadInfo, we
> could check the size of that array and do *not* create a new CORS listener
> in the securityManager if != 0.

Exactly.

> e) Separate redirectHandlers (::AsyncOnChannelRedirect) within CSP and Mixed
> Content Blocker should return NS_OK right away without consulting CSP and
> MCB, because now security checks after redirects are performed within
> asyncOpen2, right?

The redirectHandlers can only return early if useAsyncOpen2 is set. Or we put the checks inside the same if-statement as where the CORS-listener is added and which is only run if the redirectChain.length != 0.

Eventually all this can be cleaned up once all channels have useAsyncOpen2 set to true.

> f) Everything else should be fine, right? At least once I have addressed the
> other comments/reviews in this bug, or am I missing something?

Yup, that's all I can think of that we need to do.

Comment 71

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_loadinfo_changes.patch

(In reply to Jonas Sicking (:sicking) from comment #69)
> > +           uint64_t aUseAsyncOpen2,
> 
> 'bool'

Obviously it should be a boolean - thanks.

Comment 72

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_securitymanager.patch

Initially r+ed, the securityManager changed in two ways:
* We are setting the 'enforceSecurity' flag on the loadInfo to indicate that redirected channels should be openen using asyncOpen2.
* We do not enforce CORS (set up a new CorsListener) if the channel is redirected - we do this by consulting the length of the redirectChain in the loadInfo.

We still have to answer Tanvi's question in tomorrows meeting
> Didn't Jonas say that if CheckLoadURIWithPrincipal only needs to be called on non-same-origin loads?

Comment 73

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_scriptSecurityManager.patch

I just rebased this patch, we should find answers to the following questions from Tanvi and bz:
> This doesn't make sense.  Why would you ever call ChannelShouldInheritPrincipal with a literal true for the aForceInherit argument?
> Comment 55 and 58 are controversal.
> Should it live in GetChannelURIPrincipal, or GetChannelResultPrincipal

Comment 74

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_nsichannel.patch

This patch was already r+ by Pat, but I added security checks within nsViewSourceChannel using mChannel. We should confirm this is what we wanna do before landing.

Comment 75

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncOpen2_channel_redirects.patch

This is a new patch which calls asyncOpen2 instead of asyncOpen after a redirect if the initial channel was openend using asyncOpen2.

What is the best way to set mListenerContext, since asyncOpen2 does not take two arguments anymore?

Comment 76

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncOpen2_csp_mcb_changes.patch

This is also a new patch which ignores the special code added to handle redirects within CSP and mixed content. If the channel as openend using asyncOpen2, then the redirected channel will also be openen using asyncOpen2 which performs CSP and mixed content internally.

Comment 77

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncOpen2_htmlmediaelement.patch

This patch has already been r+ by Jonas and bz, but Tanvi had concerns about:
> Tanvi: Should we move ChannelShouldInheritPrincipal into GetChannelURIPrincipal at all? We do not know the SEC_FORCE_INHERIT_PRINCIPAL.
We should take another look and make sure we do the right thing here.

Comment 78

[Christoph Kerschbaumer [:ckerschb]]

The good news is that there is a testcase for media elements which also covers redirects, see:
dom/security/test/csp/test_redirects.html

Comment 79

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

Comment on attachment 8628071 [details] [diff] [review]
bug_1143922_add_asyncopen2_loadinfo_changes.patch

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

::: netwerk/base/LoadInfo.cpp
@@ +258,5 @@
> +{
> +  // Indicates whether the channel was openend using AsyncOpen2. Once set
> +  // to true, it must remain true throughout the lifetime of the channel.
> +  // Setting it to anything else than true will be discarded.
> +  NS_ASSERTION(aEnforceSecurity, "enforceSecurity must be true");

Use MOZ_ASSERT

@@ +259,5 @@
> +  // Indicates whether the channel was openend using AsyncOpen2. Once set
> +  // to true, it must remain true throughout the lifetime of the channel.
> +  // Setting it to anything else than true will be discarded.
> +  NS_ASSERTION(aEnforceSecurity, "enforceSecurity must be true");
> +  mEnforceSecurity = true;

It's a little strange to set enforceSecurity = true if someone sets it to false. In debug builds we will assert, but in release builds we'll just silently treat the 'false' as a 'true' which is a bit odd.

Maybe make release builds simply not modify mEnforceSecurity at all if aEnforceSecurity is false? I.e.

mEnforceSecurity = mEnforceSecurity || aEnforceSecurity;

Comment 80

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8628079 [details] [diff] [review]
bug_1143922_add_asyncOpen2_csp_mcb_changes.patch

As agreed in our last meeting, we are going to keep the AsyncOnChannelRedirect code (and evaluation) for CSP and MCB, which can veto the channel before asyncOpen2 and hence the security manager is called.

Comment 81

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8628080 [details] [diff] [review]
bug_1143922_add_asyncOpen2_htmlmediaelement.patch

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #77)
> Created attachment 8628080 [details] [diff] [review]
> bug_1143922_add_asyncOpen2_htmlmediaelement.patch
> 
> This patch has already been r+ by Jonas and bz, but Tanvi had concerns about:
> > Tanvi: Should we move ChannelShouldInheritPrincipal into GetChannelURIPrincipal at all? We do not know the SEC_FORCE_INHERIT_PRINCIPAL.
> We should take another look and make sure we do the right thing here.

As discussed in our meeting with Tanvi and Jonas, we do the right thing in htmlMediaElement. This patch was already r+ by Jonas and bz, Tanvi veto'ed, but we convinced her that everything we do there is correct. Carrying over r+.

Comment 82

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8628077 [details] [diff] [review]
bug_1143922_add_asyncopen2_nsichannel.patch

This patch we need to update so that we only set the EnforceSecurityFlag whenever we are dealing with nested channels. Jonas, before I am going to implement all that, are you fine with the following approach. Here examplified on the viewSourceChannel:

+NS_IMETHODIMP
+nsViewSourceChannel::AsyncOpen2(nsIStreamListener *aListener)
+{
+  nsCOMPtr<nsILoadInfo> loadInfo = mChannel->GetLoadInfo();
+  MOZ_ASSERT(loadInfo, "can not enforce security without loadInfo");
+  if (loadInfo) {
+    loadInfo->SetEnforceSecurity(true);
+  }
+  return AsyncOpen(aListener, nullptr);
+}




+NS_IMETHODIMP
 nsViewSourceChannel::AsyncOpen(nsIStreamListener *aListener, nsISupports *ctxt)
 {
     NS_ENSURE_TRUE(mChannel, NS_ERROR_FAILURE);
 
     mListener = aListener;
 
     /*
      * We want to add ourselves to the loadgroup before opening
@@ -246,30 +256,47 @@ nsViewSourceChannel::AsyncOpen(nsIStream
      */
     
     nsCOMPtr<nsILoadGroup> loadGroup;
     mChannel->GetLoadGroup(getter_AddRefs(loadGroup));
     if (loadGroup)
         loadGroup->AddRequest(static_cast<nsIViewSourceChannel*>
                                          (this), nullptr);
     
-    nsresult rv = mChannel->AsyncOpen(this, ctxt);
+    nsCOMPtr<nsILoadInfo> loadInfo = mChannel->GetLoadInfo();
+    nsresult rv = NS_OK;
+    if (loadInfo && loadInfo->GetEnforceSecurity()) {
+        rv = mChannel->AsyncOpen2(this);
+    }
+    else {
+       rv = mChannel->AsyncOpen(this, ctxt); 
+    }

If you are fine with that approach I am going to go ahead an incorporate that change on all nested channels.

Comment 83

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_loadinfo_changes.patch

Sorry, I should have added the latest changes in a different patch. Anyway, the only thing that changed in addition to the already r+ patch is the flag
> baseSecurityChecks
Now that I think about it, probably 'initialSecurityCheck' is a better name, but I don't feel strongly about it.

Comment 84

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_securitymanager.patch

So for the securityManager a few things changed:
* use of originalChannelURI and finalChannelURI for different checks
* perform checkLoadURI only if security flag is SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS or SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL
* we set SetBaseSecurityChecks
* as well as SetEnforceSecurity
* and opt out after SOP checks in case SetBaseSecurityChecks was already set. In other words we are undergoing a redirect. Please note, that SetEnforceSecurity might be set on the loadInfo within ::AsyncOpen2() for innerchannels and that the redirectChain might contain internal redirects so we can not rely on the size of that array to actually determine whether the channel was 'actually' redirected or just internally. Hence we use the flag SetBaseSecurityChecks to indicate that.

Am I missing something?

Comment 85

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8628075 [details] [diff] [review]
bug_1143922_add_asyncopen2_scriptSecurityManager.patch

I know we agreed to move the loadInfo check to GetChannelResultPrincipal. Looking at the code again, I think it makes more sense to keep it in GetChannelURIPrincipal. Mostly because we already query GetFinalChannelURI within GetChannelUriPrincipal, otherwise we would also have to query the uri  within GetChannelResultPrincipal.

I don't feel strongly, but I would rather keep the check within GetChannelUriPrincipal. If you would like me to move it, I will happily to do that as well, just let me know.

Comment 86

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

Comment on attachment 8628075 [details] [diff] [review]
bug_1143922_add_asyncopen2_scriptSecurityManager.patch

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

I know that the difference is philosophical at this point, but I think we decided to move this to GetChannelResultPrincipal, right?

::: caps/nsScriptSecurityManager.cpp
@@ +371,5 @@
> +        if ((securityFlags == nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS) ||
> +            (securityFlags == nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS) ||
> +            (securityFlags == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS)) {
> +            nsCOMPtr<nsIPrincipal> triggeringPrincipal = loadInfo->TriggeringPrincipal();
> +            bool aInheritForAboutBlank = loadInfo->GetAboutBlankInherits();

Remove the 'a' prefix.

Comment 87

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

Comment on attachment 8630847 [details] [diff] [review]
bug_1143922_add_asyncopen2_loadinfo_changes.patch

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

r=me with that.

::: netwerk/base/nsILoadInfo.idl
@@ +362,5 @@
> +   * throughout the lifetime of the channel. Trying to set it
> +   * to anything else than true will be discareded.
> +   *
> +   */
> +  attribute bool baseSecurityChecks;

Name this 'baseSecurityChecksDone'

Comment 88

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

Comment on attachment 8630849 [details] [diff] [review]
bug_1143922_add_asyncopen2_securitymanager.patch

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

r=me if you check with bz about the below.

::: dom/security/nsContentSecurityManager.cpp
@@ +253,5 @@
> +  nsCOMPtr<nsIURI> originalChannelURI;
> +  rv = aChannel->GetOriginalURI(getter_AddRefs(originalChannelURI));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = DoCheckLoadURIChecks(originalChannelURI, loadInfo);
> +  NS_ENSURE_SUCCESS(rv, rv);

I'm really not sure which URI to get here. Please double-check with bz that GetOriginalURI is the correct one to use if you haven't already.

I suspect it's the right one to use before any redirects, but after redirects we should use finalChannelURI. But since this code is so far only run before redirects it's probably fine. But worth adding a comment since eventually I think we should move this code to above the 'baseSecurityChecks' test and remove the other check that we currently have.

Comment 89

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

> I know we agreed to move the loadInfo check to GetChannelResultPrincipal.
> Looking at the code again, I think it makes more sense to keep it in
> GetChannelURIPrincipal. Mostly because we already query GetFinalChannelURI
> within GetChannelUriPrincipal, otherwise we would also have to query the uri
> within GetChannelResultPrincipal.
> 
> I don't feel strongly, but I would rather keep the check within
> GetChannelUriPrincipal. If you would like me to move it, I will happily to
> do that as well, just let me know.

Ah, didn't see this comment before. I'm fine with having it in either place. But bz asked to have it moved, so you'd have to check with him if you want to keep it in current place.

Comment 90

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

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #82)
> If you are fine with that approach I am going to go ahead an incorporate
> that change on all nested channels.

Sounds good. But please do this as a separate patch.

Comment 91

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_scriptSecurityManager.patch

(In reply to Jonas Sicking (:sicking) from comment #86)
> I know that the difference is philosophical at this point, but I think we
> decided to move this to GetChannelResultPrincipal, right?

As agreed in our meeting, let's move this into GetChannelResultPrincipal - here we go.

Comment 92

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8628077 [details] [diff] [review]
bug_1143922_add_asyncopen2_nsichannel.patch

Then let's leave this patch r+ed and provide the additonal changes in a separate patch (Intial r+ by Pat, see comment 48).

Comment 93

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

Comment on attachment 8631308 [details] [diff] [review]
bug_1143922_add_asyncopen2_scriptSecurityManager.patch

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

Looks great! I'd still like to get bz's review, if for no other reason so that he's aware of this way of doing inheritance. My hope is that we can transition all inheritance to happen this way, but that's clearly a longer project.

Comment 94

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_innerchannels.patch

When dealing with inner channels we do not want to enforce security checks twice, hence we just set the enforceSecurityFlag within ::AsyncOpen2() on the outer channel and we branch on that flag on the inner channel whether to call asyncOpen2 or asyncOpen. We do this for:
* nsViewSourceChannel, as well as
* nsJarChannel

We also set the enforceSecurityFlag within the securityManger so that redirected channels know they also have to be openend using asyncOpen2(), we check that flag within:
* nsBaseChannel, as well as
* nsHttpChannel

Jonas, Pat, are we missing something? Or any other objections?

Please note that the securityManager sets an additonal flag on the loadInfo (intialSecurityFlag) the first time asyncOpen2 consults the securityManager. We do this so that redirected channels, openend through AsyncOpen2() and evaluated in the securityManager do not perform certain security checks. E.g. no need to set up CORS again, or also no need to consult all contentPolcies. Please additionaly note that CSP and MCB use their own ::redirect handlers, so they get evaluated correctly before asyncOpen2 is evaluated.

Comment 95

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8631308 [details] [diff] [review]
bug_1143922_add_asyncopen2_scriptSecurityManager.patch

Both of the if conditions have things over-parenthesized.  Please nix the extraneous parens.

I assume longer-term the plan is to remove the GetForceInheritPrincipal thing?

Otherwise this looks fine, as long as you've double-checked that things don't go wrong (or at least differently from what we have right now) with file:// when symbolic links are involved.  Both directions: a file linking to a symlink in a subdir that points outside the file's dir, and a file linking to a symlink outside its dir that points into a subdir of the original file's directory.

Comment 96

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

Comment on attachment 8631345 [details] [diff] [review]
bug_1143922_add_asyncopen2_innerchannels.patch

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

::: modules/libjar/nsJARChannel.cpp
@@ +992,5 @@
>  {
> +  MOZ_ASSERT(mLoadInfo, "can not enforce security without loadInfo");
> +  if (mLoadInfo) {
> +    mLoadInfo->SetEnforceSecurity(true);
> +  }

Might be worth adding a comment saying that the inner channel will take care of doing the security checks.

Also, I think it'd be safer, and more consistent, to make AsyncOpen2 fail if there's no LoadInfo.

@@ +1032,5 @@
>              return rv;
>          }
> +
> +        if (mLoadInfo && mLoadInfo->GetEnforceSecurity()) {
> +            rv = channel->AsyncOpen2(this);

Should be 'downloader' rather than 'this', right?

::: netwerk/base/nsBaseChannel.cpp
@@ +154,5 @@
>    // with the redirect.
>  
>    if (mOpenRedirectChannel) {
> +    nsresult rv = NS_OK;
> +    nsCOMPtr<nsILoadInfo> loadInfo = mRedirectChannel->GetLoadInfo();

Is there a reason we're getting the loadinfo from mRedirectChannel rather than from this channel? I.e. why not just use mLoadInfo here?

@@ +156,5 @@
>    if (mOpenRedirectChannel) {
> +    nsresult rv = NS_OK;
> +    nsCOMPtr<nsILoadInfo> loadInfo = mRedirectChannel->GetLoadInfo();
> +    if (loadInfo && loadInfo->GetEnforceSecurity()) {
> +      rv = mRedirectChannel->AsyncOpen2(mListener);

Maybe MOZ_ASSERT that mListenerContext is null?

Both these comments also apply to nsHttpChannel

Comment 97

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #95)
> Comment on attachment 8631308 [details] [diff] [review]
> bug_1143922_add_asyncopen2_scriptSecurityManager.patch
> 
> Both of the if conditions have things over-parenthesized.  Please nix the
> extraneous parens.

Agreed, updated!
 
> I assume longer-term the plan is to remove the GetForceInheritPrincipal
> thing?

Yes, but that's futher down the road.

> Otherwise this looks fine, as long as you've double-checked that things
> don't go wrong (or at least differently from what we have right now) with
> file:// when symbolic links are involved.  Both directions: a file linking
> to a symlink in a subdir that points outside the file's dir, and a file
> linking to a symlink outside its dir that points into a subdir of the
> original file's directory.

I haven't, but I will make sure to test before landing.

Comment 98

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_innerchannels.patch

(In reply to Jonas Sicking (:sicking) from comment #96)
> Comment on attachment 8631345 [details] [diff] [review]
> bug_1143922_add_asyncopen2_innerchannels.patch
>
> Also, I think it'd be safer, and more consistent, to make AsyncOpen2 fail if
> there's no LoadInfo.

I think we are not quite there yet. Also nsIOService allows creation of a channel without a loadInfo [1]. Agreed, it would be great to return in such a case, but let's go with minimal changes for the first iteration when calling the securityManager inside asyncOpen2().

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#817
 
> ::: netwerk/base/nsBaseChannel.cpp
> @@ +154,5 @@
> >    // with the redirect.
> >  
> >    if (mOpenRedirectChannel) {
> > +    nsresult rv = NS_OK;
> > +    nsCOMPtr<nsILoadInfo> loadInfo = mRedirectChannel->GetLoadInfo();
> 
> Is there a reason we're getting the loadinfo from mRedirectChannel rather
> than from this channel? I.e. why not just use mLoadInfo here?

Technically it doesn't make a difference at all. My thinking was, that for people reading the code it might be easier to understand. In fact it's the same loadInfo, so we might as well use mLoadInfo. I went ahead and incorporated the change mostly because we safe a few cycles by not quering the loadInfo from mRedirectChannel.

 
> @@ +156,5 @@
> >    if (mOpenRedirectChannel) {
> > +    nsresult rv = NS_OK;
> > +    nsCOMPtr<nsILoadInfo> loadInfo = mRedirectChannel->GetLoadInfo();
> > +    if (loadInfo && loadInfo->GetEnforceSecurity()) {
> > +      rv = mRedirectChannel->AsyncOpen2(mListener);
> 
> Maybe MOZ_ASSERT that mListenerContext is null?

Yes, makes sense. Hopefully treeherder feels the same way :-)

> Both these comments also apply to nsHttpChannel
Yes, also there I am using mLoadInfo instead.

Comment 99

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_securitymanager.patch

(In reply to Jonas Sicking (:sicking) from comment #88)
> Comment on attachment 8630849 [details] [diff] [review]
> bug_1143922_add_asyncopen2_securitymanager.patch
> 
> Review of attachment 8630849 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me if you check with bz about the below.
> 
> ::: dom/security/nsContentSecurityManager.cpp
> @@ +253,5 @@
> > +  nsCOMPtr<nsIURI> originalChannelURI;
> > +  rv = aChannel->GetOriginalURI(getter_AddRefs(originalChannelURI));
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  rv = DoCheckLoadURIChecks(originalChannelURI, loadInfo);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> 
> I'm really not sure which URI to get here. Please double-check with bz that
> GetOriginalURI is the correct one to use if you haven't already.
> 
> I suspect it's the right one to use before any redirects, but after
> redirects we should use finalChannelURI. But since this code is so far only
> run before redirects it's probably fine. But worth adding a comment since
> eventually I think we should move this code to above the
> 'baseSecurityChecks' test and remove the other check that we currently have.

[Note: baseSecurityChecks got renamed to initialSecurityCheckDone]

Boris, do you agree with me and Jonas or would you like to have things changed in the patch?

Comment 100

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8631789 [details] [diff] [review]
bug_1143922_add_asyncopen2_securitymanager.patch

So the nsCorsListenerProxy actually does CheckLoadURI on both the original URI and the channel URI.

But in general, I agree that the URI the check should be done on is the original URI in all cases that are not explicit redirects.  That is, it should be the URI passed to newChannel.  In some ways, finalChannelURI captures this most closely, no?  Is there a reason we're not just using that?

On a side note, the securityMode check in DoCheckLoadURIChecks has overparenthesized conditions.

Comment 101

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

> But in general, I agree that the URI the check should be done on is the
> original URI in all cases that are not explicit redirects.  That is, it
> should be the URI passed to newChannel.  In some ways, finalChannelURI
> captures this most closely, no?  Is there a reason we're not just using that?

I think that's the question for you. It does indeed look like finalChannelURI is good, but I wouldn't be comfortable using that without having checked with you first :)

Comment 102

[Boris Zbarsky [:bzbarsky]]

I think using finalChannelURI, along with a comment explaining why it's the right thing, would make the most sense to me.

Comment 103

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #102)
> I think using finalChannelURI, along with a comment explaining why it's the
> right thing, would make the most sense to me.

Ok, using finalChannelURI makes the most sense. So I am going to remove originalURI from the patch completely and use finalChannelURI for all the security checks within contentSecuritymanager. But what comment would you like to have and where? Just one above
> nsCOMPtr<nsIURI> finalChannelURI;
or
> rv = DoCheckLoadURIChecks(finalChannelURI, loadInfo);
or both?

Comment 104

[Boris Zbarsky [:bzbarsky]]

I think the comment should be above DoCheckLoadURIChecks and should explain why what we're passing in is equivalent to what we want to conceptually be passing in, that being the "URI passed to newChannel".

Comment 105

[Tanvi Vyas[:tanvi]]

Comment on attachment 8631789 [details] [diff] [review]
bug_1143922_add_asyncopen2_securitymanager.patch

+nsresult
+nsContentSecurityManager::doContentSecurityCheck(nsIChannel* aChannel,
+                                                 nsCOMPtr<nsIStreamListener>& aInAndOutListener)
+{
...
+  // lets store the initialSecurityCheckDone flag which indicates whether the channel
+  // was initialy evaluated by the contentSecurityManager. Once the inital
+  // asyncOpen() of the channel went through the contentSecurityManager then
+  // redirects do not have perform all the security checks, e.g. no reason
+  // to setup CORS again.
+  bool initialSecurityCheckDone = loadInfo->GetInitialSecurityCheckDone();
+
+  // now lets set the baseSecurityFlag for subsequent calls
+  rv = loadInfo->SetInitialSecurityCheckDone(true);
+  NS_ENSURE_SUCCESS(rv, rv);

Perhaps you should set this to true later in the function after all the security checks.



+  // http://mxr.mozilla.org/mozilla-central/source/caps/nsIScriptSecurityManager.idl#62
You may want to make this a link with a rev number since line 62 may change overtime.

Comment 106

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1143922_add_asyncopen2_open_innerchannels.patch

Jonas, Pat, one thing I just realized when looking at the patches again is that not only do we have to set the enforceSecurityFlag for innerChannels within ::asyncOpen2() but also for ::Open2().

There are two cases:

(1) nsViewSourceChannel::Open2()
We just set the flag within ::Open2() and branch on the flag within ::Open(). Exactly what we do for asyncOpen2() within bug_1143922_add_asyncopen2_innerchannels.patch

(2) nsJARChannel::Open2
internally calls CreateJarInput and calls Open() on an nsIZipReader [1] (not the channel). So I suppose we keep the security checks within ::Open2().

[1] http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsJARChannel.cpp#294

Comment 107

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

Comment on attachment 8635441 [details] [diff] [review]
bug_1143922_add_asyncopen2_open_innerchannels.patch

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

Leave a comment in nsJarChannel::Open saying something like:

"nsJarChannel::Open doesn't call Open() on the inner channel. Instead it only supports inner channels that represent an nsIFile and open that nsIFile directly. So we do security checks here directly"

::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp
@@ +225,5 @@
>  {
>      NS_ENSURE_TRUE(mChannel, NS_ERROR_FAILURE);
>  
> +    nsresult rv = NS_OK;
> +    nsCOMPtr<nsILoadInfo> loadInfo = mChannel->GetLoadInfo();

Can't you just use mLoadInfo here? Same in Open2.

Comment 108

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

Comment on attachment 8635441 [details] [diff] [review]
bug_1143922_add_asyncopen2_open_innerchannels.patch

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

::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp
@@ +247,5 @@
> +    nsCOMPtr<nsILoadInfo> loadInfo = mChannel->GetLoadInfo();
> +    if(!loadInfo) {
> +        MOZ_ASSERT(loadInfo, "can not enforce security without loadInfo");
> +        return NS_ERROR_UNEXPECTED;
> +    }

If you use mLoadInfo you can just write this as

MOZ_ASSERT(mLoadInfo);
NS_ENSURE_TRUE(mLoadInfo, NS_ERROR_FAILURE);

Comment 109

[Patrick McManus [:mcmanus]]

Comment on attachment 8635441 [details] [diff] [review]
bug_1143922_add_asyncopen2_open_innerchannels.patch

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

I like jonas's comments here..

Comment 110

[Christoph Kerschbaumer [:ckerschb]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9241b60b15a4bbd77ef82879b22614e4171913
changeset:  2f9241b60b15a4bbd77ef82879b22614e4171913
user:       Christoph Kerschbaumer <[e-mail]>
date:       Sun Jul 19 19:11:57 2015 -0700
description:
Bug 1143922 - Add AsyncOpen2 to nsIChannel and perform security checks when opening a channel - loadinfo changes (r=sicking,tanvi,sworkman)

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/1d919946baac36b6137f567632b97fd42518870e
changeset:  1d919946baac36b6137f567632b97fd42518870e
user:       Christoph Kerschbaumer <[e-mail]>
date:       Sun Jul 19 19:12:11 2015 -0700
description:
Bug 1143922 - Add AsyncOpen2 to nsIChannel and perform security checks when opening a channel - securitymanager (r=sicking,tanvi)

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/6deb27dd785b2d79152a0b96730803f9a5a9c290
changeset:  6deb27dd785b2d79152a0b96730803f9a5a9c290
user:       Christoph Kerschbaumer <[e-mail]>
date:       Sun Jul 19 19:12:26 2015 -0700
description:
Bug 1143922 - Add AsyncOpen2 to nsIChannel and perform security checks when opening a channel - scriptSecurityManager changes (r=sicking,bholley)

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/9b5a890968bbc3588901c8763954001ca04aaec1
changeset:  9b5a890968bbc3588901c8763954001ca04aaec1
user:       Christoph Kerschbaumer <[e-mail]>
date:       Fri May 15 13:21:20 2015 -0700
description:
Bug 1143922 - Add AsyncOpen2 to nsIChannel and perform security checks when opening a channel - channel changes (r=mcmanus,sicking)

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/accaecdd989d0a05d3d948608b7e1a7f1a97602d
changeset:  accaecdd989d0a05d3d948608b7e1a7f1a97602d
user:       Christoph Kerschbaumer <[e-mail]>
date:       Sun Jul 19 19:13:28 2015 -0700
description:
Bug 1143922 - Add AsyncOpen2 to nsIChannel and perform security checks when opening a channel - media element changes (r=sicking,tanvi)

Comment 111

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f9241b60b15
https://hg.mozilla.org/mozilla-central/rev/1d919946baac
https://hg.mozilla.org/mozilla-central/rev/6deb27dd785b
https://hg.mozilla.org/mozilla-central/rev/9b5a890968bb
https://hg.mozilla.org/mozilla-central/rev/accaecdd989d

Comment 112

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/977d5b7ecba3
https://hg.mozilla.org/integration/mozilla-inbound/rev/deda472458fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/309b4d1ab81c

Comment 113

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Pulsebot from comment #112)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/977d5b7ecba3
> https://hg.mozilla.org/integration/mozilla-inbound/rev/deda472458fd
> https://hg.mozilla.org/integration/mozilla-inbound/rev/309b4d1ab81c

Sorry, these have the wrong bugnumber :-(
Those patches should be over in Bug 1204703

Comment 114

[Pulsebot]

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