Closed Bug 1139297 Opened 9 years ago Closed 9 years ago

Implement CSP upgrade-insecure-requests directive

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: annevk, Assigned: ckerschb, Mentored)

References

(Depends on 1 open bug, Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete, sec-want, Whiteboard: [adv-main42-])

Attachments

(15 files, 27 obsolete files)

1.74 KB, patch
geekboy
: review+
bholley
: review+
Details | Diff | Splinter Review
2.05 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
33.68 KB, patch
geekboy
: review+
Details | Diff | Splinter Review
4.48 KB, patch
smaug
: review+
Details | Diff | Splinter Review
21.85 KB, patch
ckerschb
: review+
sicking
: review+
Details | Diff | Splinter Review
5.02 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
3.49 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
3.13 KB, patch
tanvi
: review+
Details | Diff | Splinter Review
5.75 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.13 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
8.60 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
6.78 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
15.34 KB, patch
tanvi
: review+
geekboy
: review+
Details | Diff | Splinter Review
5.83 KB, patch
baku
: review+
Details | Diff | Splinter Review
4.41 KB, patch
sworkman
: review+
Details | Diff | Splinter Review
Instead of blocking mixed content, this will automatically upgrade it, helping sites with lots of legacy content to more easily move to TLS without having to worry about mixed content warnings in their UI.
Mentor: tanvi
Keywords: dev-doc-needed
Assignee: nobody → mozilla
Blocks: csp-w3c-2
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Component: Security → DOM: Security
Keywords: sec-want
We have been discussing the best way to implement this feature, because overall it's part of CSP and hence it should somehow fall into the responsibilities of CSP implemented in dom/security/. I am still not completely sure what the best way to go about this feature is. I am attaching my initial approach. Please note (besides that this patch is by far not complete in respect to the spec) that this approach where we would update the nsIURI handed to contentPolicies before MixedContent is evaluated is not going to work out. Reason is, some nsIURIs passed into contentPolicies are marked immutable, and hence we should not update them from http to https (or also ws to wss for websockets). I am attaching this first patch/approach anyway even though we need evaluate different approaches. However, we can reuse the parser changes, the tests, the updates to WebSockets, etc from this patch.

Once we have moved security checks into AsyncOpen2 (See bug 1143922) this feature would be way easier to implement, because we could perform a channel redirect before actually openening the channel. Moving all the security checks into AsyncOpen2() is unfortunately not going to be done anytime soon. So we have to come up with a interim solution to get this feature landed.
Attachment #8608920 - Attachment is obsolete: true
Comment on attachment 8611025 [details] [diff] [review]
bug_1139297_upgrade_insecure_docshell.patch

Hi Boris, I chatted with Jonas and Tanvi about our (actually your idea) of returning an nsIURI from ContentPolicies in case CSP performs an upgrade of the URI. We think the easier solution is to whitelist the http load within CSP and also MixedContentBlocking with the promise that the httpchannel implementation later upgrades the request from http to https before it hits the wire.

I have implemented that approach in the attached patches. Please note that we have to walk the docshells up to the rootDoc and in case we detect any CSP that makes use of upgrade-insecure-reuqests we have to upgrade the subresource request from http to https. The spec requires that even subresources within iframes are upgraded.

Please further note that the attached patches as of now do not cover:
* Upgrades for websockets from ws to wss
* Form submissions (POST requests)
* CORS issues, which also need to believe in the promise the httpchannel implementation upgrades the request.

What do you think? Sounds reasonable/acceptable?
Attachment #8611025 - Flags: feedback?(bzbarsky)
Comment on attachment 8611028 [details] [diff] [review]
bug_1139297_upgrade_insecure_netwerk.patch

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

So this basically follows the same path as HSTS, i.e. setting up an internal redirect to the HTTPS version of the channel.

Can you make sure that HTTP Referer doesn't get overwritten in a test please? I assume that the referrer should not change for either HSTS nor upgrade-insecure-requests when the scheme is changed.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +335,5 @@
> +          bool upgradeInsecureRequests = false;
> +          rv = docShell->GetUpgradeInsecureRequests(&upgradeInsecureRequests);
> +          NS_ENSURE_SUCCESS(rv, rv);
> +          if (upgradeInsecureRequests) {
> +            return AsyncCall(&nsHttpChannel::HandleAsyncRedirectChannelToHttps);

Can you add a new histogram value here for HTTP_SCHEME_UPGRADE.

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json#1210
Attachment #8611028 - Flags: feedback+
Comment on attachment 8611025 [details] [diff] [review]
bug_1139297_upgrade_insecure_docshell.patch

This seems plausible, depending on what exactly the upgrade-insecure-requests CSP does.  In particular, what does it do for protocols other than "http" (say "ftp")?  What does the mixed content blocker do with those?

The "loadInfo" temporary in that patch is unused.
Attachment #8611025 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Not doing reviews right now from comment #12)
> In particular, what does it do for protocols other than "http" (say "ftp")?
If I am interpreting the spec correctly upgrade-insecure-requests only applies to http and ws (websocket) loads. I will make sure we are not missing anything here.

> What does the mixed content blocker do with those?
Websockets are not subject to MixedContentBlocking, since insecure websockets are not allowed on secure pages and blocked even before they might hit MixedContentBlocking. As for 'FTP' and potentially other schemes, I am not completely sure. Very likely we have to update the code currently in the MixedContentBlocker patch to only return early if we indeed load an http page that is about to be upgraded to https, but still evaluate mixedContent for potential other schemes.

> The "loadInfo" temporary in that patch is unused.
Thanks - updated :-)
Right.  My point is that we need to make sure the mixed content blocker only returns early in cases that will actually be upgraded by the CSP.
Jonas, as discussed in person the other day we also have to modify the CORS code to account for the potential upgrade if the CSP directive upgrade-insecure-requests is used. Can you take a look at the attached patch and provide some feedback? Am I missing something or does that approach look reasonable?

Also, I am using NS_GetFinalChannelURI to query the uri which I am performing the comparison on - do we also have to perform some checks on the originalURI as in updateChannel?
Attachment #8612705 - Flags: feedback?(jonas)
To all reviewers, let me summarize the approach and all the actors that are included when the CSP directive 'upgrade-insecure-requests' is used:

(1) CSP:
    The CSP implementation whitelists the http-request
    in case the policy is executed in enforcement mode.
    The CSP implementation however does not allow http
    requests to succeed if executed in report-only mode.
    In such a case the CSP implementation reports the
    error back to the page.
(2) MixedContent:
    The evalution of MixedContent whitelists all http
    requests with the promise that the http requests
    gets upgraded to https before any data is fetched
    from the network.
(3) CORS:
    Does not consider the http request to be of a
    different origin in case the scheme is the only
    difference in otherwise matching URIs.
(4) nsHttpChannel:    Before connecting, the channel
    gets redirected to use https.
(5) WebSocketChannel:
    Similar to the httpChannel, the websocketchannel
    gets upgraded from ws to wss.
Hey Sid, a few notes:
* I factored out the scheme-matching into it's own function permitsScheme() so we don't have two implementations. One in nsCSPSchemeSrc and one in nsCSPHostSrc.
This approach also allowed to simplify scheme-less host source matching (no need to store and make use of  'mAllowHttps').

* I eliminated nsCSPDirective::permits(nsIURI* aURI). It's a legacy method that should have been removed together with Bug 999656. I was unused since then. I removed it now.

* After all CSP upgrade-insecure-requests is a CSP directive, hence the subclass approach. Even though the subclass does not do much itself, it allows that 'upgrade-insecure-requests' appears at the same location in case we call toString() on the policy. Of course, we could store an additional int to store the location, but I think it's the cleaner approach to subclass nsDirective. Also semantic wise.

* I left a note in the code regarding the caching mechanism. I think the patch should not affect caching, but i would like you to confirm.
Attachment #8611025 - Attachment is obsolete: true
Attachment #8611026 - Attachment is obsolete: true
Attachment #8611027 - Attachment is obsolete: true
Attachment #8611028 - Attachment is obsolete: true
Attachment #8611029 - Attachment is obsolete: true
Attachment #8611030 - Attachment is obsolete: true
Attachment #8611031 - Attachment is obsolete: true
Attachment #8612705 - Attachment is obsolete: true
Attachment #8613109 - Attachment is obsolete: true
Attachment #8613110 - Attachment is obsolete: true
Attachment #8612705 - Flags: feedback?(jonas)
Attachment #8614866 - Flags: review?(sstamm)
Attachment #8614868 - Flags: review?(sworkman)
Attachment #8614868 - Flags: review?(jonas)
Attachment #8614869 - Flags: review?(tanvi)
Jonas, do we also have to account for OriginalURI somehow?
Attachment #8614870 - Flags: review?(jonas)
Attachment #8614871 - Flags: review?(sworkman)
Baku, jst suggested you as a reviewer for websockets, please let me know if I need to find someone else. Thank you!
Attachment #8614872 - Flags: review?(amarchesini)
Attachment #8614873 - Flags: review?(amarchesini)
Bobby, I would need you to sign off on the webidl bits.
Attachment #8614874 - Flags: review?(sstamm)
Attachment #8614874 - Flags: review?(bobbyholley)
Attachment #8614877 - Flags: review?(tanvi)
Attachment #8614877 - Flags: review?(sstamm)
Comment on attachment 8614874 [details] [diff] [review]
bug_1139297_upgrade_insecure_csp_devtool.patch

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

rs=me assuming this matches the spec.
Attachment #8614874 - Flags: review?(bobbyholley) → review+
Comment on attachment 8614870 [details] [diff] [review]
bug_1139297_upgrade_insecure_cors.patch

Olli, Jonas would prefer if you could review the CORS bits of that patch. Again, the question, do we have to account for the OriginalURI in any way?
Attachment #8614870 - Flags: review?(jonas) → review?(bugs)
Reviewing this will take time. I know nothing about https://w3c.github.io/webappsec/specs/upgrade/
nor do I know whether that draft spec has been reviewed, or whether it is stable at all.
What does the spec say about original uri vs requestor uri usage? Especially if there has been some
redirects.
Comment on attachment 8614870 [details] [diff] [review]
bug_1139297_upgrade_insecure_cors.patch

and to help reviewing, the odd looking originalURI check was added in bug 460425.
Comment on attachment 8614870 [details] [diff] [review]
bug_1139297_upgrade_insecure_cors.patch

>+// Please note that the CSP directive 'upgrade-insecure-requests' relies
>+// on the prommise that channels get updated from http: to https: before
>+// the channel fetches any data from the netwerk. Such channels should
>+// not be blocked by CORS and marked as cross origin requests. E.g.:
>+// toplevel page: https://www.example.com loads
>+//         image: http://www.example.com/foo.jpg which gets updated to
>+//                https://www.example.com/foo.jpg
>+// In such a case we can return early and rely in the promise
>+// that nsHttpChannel::Connect() upgrades the request from http to https.
>+bool
>+checkUpgradeInsecureRequestsPreventsCORS(nsIPrincipal* aRequestingPrincipal,
s/check/Check/

>+                                         nsIChannel* aChannel)
>+{
>+  nsCOMPtr<nsIURI> channelURI;
>+  nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(channelURI));
>+  NS_ENSURE_SUCCESS(rv, false);
>+  bool isHttpScheme = false;
>+  rv = channelURI->SchemeIs("http", &isHttpScheme);
>+  NS_ENSURE_SUCCESS(rv, false);
>+
>+  // upgrade insecure requests is only applicable to http requests
>+  if (!isHttpScheme) {
>+    return false;
>+  }
>+
>+  nsCOMPtr<nsIURI> principalURI;
>+  rv = aRequestingPrincipal->GetURI(getter_AddRefs(principalURI));
>+  NS_ENSURE_SUCCESS(rv, false);
>+
>+  // if the requestingPrincipal does not have a uri, there is nothing to do
>+  if (!principalURI) {
>+    return false;
>+  }
>+
>+  nsAutoCString principalHost;
>+  nsAutoCString channelHost;
>+
>+  // if we can not query a host from the uri, there is nothing to do
>+  if (NS_FAILED(principalURI->GetAsciiHost(principalHost)) ||
>+      NS_FAILED(channelURI->GetAsciiHost(channelHost))) {
>+    return false;
>+  }
>+
>+  // if the hosts do not match, there is nothing to do
>+  if (!principalHost.EqualsIgnoreCase(channelHost.get())) {
>+    return false;
>+  }
>+
>+  nsCOMPtr<nsILoadInfo> loadInfo;
>+  rv = aChannel->GetLoadInfo(getter_AddRefs(loadInfo));
>+  NS_ENSURE_SUCCESS(rv, false);
>+
>+  // lets see if the loadInfo indicates that the request will
>+  // be upgraded before fetching any data from the netwerk.
>+  return loadInfo->GetUpgradeInsecureRequests();
>+}
so I don't know what GetUpgradeInsecureRequests() actually means. Is it bound to the requesting principal, or does it mean all the requests in the redirect chain?
What if we have domain A which does a CORS request to domain B, and that redirects to 
(http) domain A. What should happen? (Note, UpdateChannel is called from Init and OnRedirectVerifyCallback)
I guess it would be safer to check originalURI here too, that its host is the same as url's host.
But what does the spec say?

How does the upgrade actually happen? At which point? Do we copy the relevant flags which are normally set at the end of UpdateChannel?
if (mIsPreflight || !mWithCredentials) { ...

I think this needs some clarifications and comments in the code.
Attachment #8614870 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #36)
> I think this needs some clarifications and comments in the code.

Let me try clarify. The spec says nothing about CORS, it's the way we implement the CSP directive 'upgrade-insecure-requests' within Gecko. Assume the toplevel page uses a CSP with the directive upgrade-insecure-requests. Using that directive indicates that all http requests within that page should be upgraded to http*s* requests. Please note, that the directive also applies to nested contexts (iframes).

So here is the way our implementation works:
* CSP: In case the subresource load is subject to CSP of the current context, then our CSP implementation does *not* block the http request even if the enforced policy uses
> 'upgrade-insecure-requests'; default-src: https;
which would indicate that the request should be blocked because it's not https. CSP allows the load with the promise that nsHttpChannel::Connect() upgrades the reqeust to https. If for example, the subresource load happens within an iframe on a page that does not ship with it's own CSP, then CSP is not called even if the toplevel page ships with CSP. Becauce the CSP of the toplevel page itself is not inherited to the iframe.

* MixedContent: The http subresource request of the page is now subject to MixedContentBlocking which would usually block the load, because it's an http load within an https page. We implemented a function called ::UpgradeInsecureRequestsFromNestedContext(nsIDocument) which uses the current document (that loads the subresource) and returns true in case of any of the documents up to the toplevel document makes use of the CSP directive upgrade insecure requests. If true, then MCB whitelists the load with the promise that the channel gets upgraded to https before any data is fetched from the network.

* Please not that CSP and MCB are contentPolicies which are called before a channel is created and operate solely on the nsIURI of the subresource to be fetched. Once contentPolicies greenlight the load we are about to create a channel. When we create the channel we set a flag in the LoadInfo of the channel (based on whether ::UpgradeInsecureRequestsFromNestedContext(nsIDocument) returns true or not. (please note that we set that flag so we have it availabel in e10s as well where the nsINode of the loadInfo is not availabe in the parent process anymore).

* CORS: Now we have to apply the same mechansim that the request does not become subject to CORS, because CORS would consider a toplevel page of https and a subresource load of http to be cross origin, right? We use the flag from the loadInfo so the request does *not* become subject of CORS because nsHttpChannel::Connect() will upgrade the request from http to https and hence no need for CORS, right?

* nsHttpChannel: Once we call ::Connect() on the channel, we again query the flag from the loadInfo and perform an internal redirect to https if the flag indicates to do so.


To sum it up, we just need to make sure that the subresource load that will be upgraded to https does *not* become subject to CORS. It shouldn't affect anything else, or am I missing something here? If it makes it easier, I am happy to hop on vidyeo and we can discuss things in person.
Flags: needinfo?(bugs)
So even in case of redirection we don't need CORS handling for upgrades?
I mean, 
domain A (which has 'upgrade insecure requests' flag) does cors to domain B, and domain redirects to (http) domain A. So we should do http->https for that latter case. We get UpdateChannel call for
domain B -> domain A(http) redirection, (and the patch then just bypasses part of the CORS handling since it is http and we have the upgrade flag), and I assume we'll get another UpdateChannel call for 
the http->https upgrade, right?
In other words, what is the behavior of OnRedirectVerifyCallback (which calls UpdateChannel) in this new world?
Flags: needinfo?(bugs) → needinfo?(mozilla)
(In reply to Olli Pettay [:smaug] from comment #38)
> So even in case of redirection we don't need CORS handling for upgrades?
> I mean, 
> domain A (which has 'upgrade insecure requests' flag) does cors to domain B,
> and domain redirects to (http) domain A. So we should do http->https for
> that latter case. We get UpdateChannel call for
> domain B -> domain A(http) redirection, (and the patch then just bypasses
> part of the CORS handling since it is http and we have the upgrade flag),
> and I assume we'll get another UpdateChannel call for 
> the http->https upgrade, right?

Yes, I would hope so. Whenever we create a new channel, we either
* copy the loadInfo of the old channel.
In case of a redirect the loadInfo of the initial channel would determine whether any of the documents up to the toplevel document uses the CSP directive and hence sets the upgrade flag on the loadInfo, or

* we create a new loadInfo for any new channel.
In this case we also call ::UpgradeInsecureRequestsFromNestedContext(nsIDocument) within the constructor of the loadInfo and hence set the upgrade flag in the loadInfo.

So all redirect cases should be handled correctly.

> In other words, what is the behavior of OnRedirectVerifyCallback (which calls UpdateChannel) in this new world?

As long as the only difference is the scheme (http/https) we should *not* do CORS, in all other cross origin scenarios we should do CORS. I hope the patch implements all that. Besides it's not taking the OriginalURI into account as of now, but I think we should perform that check as well, right?
Flags: needinfo?(mozilla) → needinfo?(bugs)
right now the patch seems to bypass cors whenever doing someurl->http_url_of_requesting_domain, and
I guess the spec hints that http->https should happen always, if requestor has the flag.
So also someDomain->someOtherDomain_http should be bypassed, since someOtherDomain_http should get
upgraded to someOtherDomain_https. So if that is the wanted behavior, you need to compare originalURI to url

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #39)
> > In other words, what is the behavior of OnRedirectVerifyCallback (which calls UpdateChannel) in this new world?
> 
> As long as the only difference is the scheme (http/https) we should *not* do
> CORS, in all other cross origin scenarios we should do CORS. I hope the
> patch implements all that.
The patch makes us bypass someDomain->requestorDomain_http redirect check, so do we then get a new
OnRedirectVerifyCallback call when requestorDomain_https (upgraded from http) request is created?
Flags: needinfo?(bugs)
Comment on attachment 8614868 [details] [diff] [review]
bug_1139297_upgrade_insecure_loadinfo.patch

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

LGTM :)
Attachment #8614868 - Flags: review?(sworkman) → review+
Comment on attachment 8614871 [details] [diff] [review]
bug_1139297_upgrade_insecure_netwerk.patch

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

Also LGTM.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +7,5 @@
>  // HttpLog.h should generally be included first
>  #include "HttpLog.h"
>  
> +#include "mozilla/dom/nsCSPUtils.h"
> +#include "mozilla/dom/nsCSPContext.h"

Please check that you need these includes.

@@ +329,5 @@
>  
>      if (!isHttps) {
> +        // It's time to fulfill the promise to CSP and mixed content blocking to
> +        // upgrade the channel from http to https if any of the documents up the
> +        // chain to the rootDocument makes use of 'upgrade-insecure-requests'.

nit: Can you rephrase this comment to be ...
"If any of the documents .... then it's time to fulfill...."
Just to make it REALLY clear from the start that this is for 'upgrade..."
:)

@@ +349,5 @@
> +                                EmptyString(), // aScriptSample
> +                                0, // aLineNumber
> +                                0, // aColumnNumber
> +                                nsIScriptError::warningFlag, "CSP",
> +                                innerWindowId); // innerWindowID

nit: Please remove eol comment.
Attachment #8614871 - Flags: review?(sworkman) → review+
Comment on attachment 8614866 [details] [diff] [review]
bug_1139297_upgrade_insecure_csp.patch

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

I like what you did with permitsScheme.

Please address the question below about the perf issue and then re-flag me.

::: dom/security/nsCSPContext.cpp
@@ +192,5 @@
>                             : nsIContentPolicy::REJECT_SERVER;
>  
> +  // Sid: I don't think the way we implemented upgrade-insecure-requests
> +  // interfers in any way with the caching here, can you confirm?
> +

Can't tell by this patch alone, but it looks like the http->https redirect happens before the call to the content policies, so we should never reach the ShouldLoad query *before* upgrading.

@@ +335,5 @@
> + * @return
> + *        Whether the subresource load needs to be upgraded to https (wss).
> + */
> +bool
> +nsCSPContext::UpgradeInsecureRequestsFromNestedContext(nsIDocument* aDocument)

Why is this function in nsCSPContext and not in nsDocument?  Won't it be needed by all loads, such as those in documents that don't have a CSP?

This could be a perf issue if we have to search through the document tree for every subresource load.  Why not put this nsDocument and cache the result for "some ancestor wants upgrades"?

::: dom/security/nsCSPUtils.cpp
@@ +295,5 @@
> +  // http://www.w3.org/TR/CSP2/#match-source-expression
> +  if (aEnforcementScheme.EqualsASCII("http") &&
> +      scheme.EqualsASCII("https")) {
> +    return true;
> +  }

If you're going to refactor/change this code that decides whether or not to allow https loads for non-https protected documents, please ensure we have adequate test coverage for this case.

@@ +938,5 @@
> +void
> +nsUpgradeInsecureDirective::toString(nsAString& outStr) const
> +{
> +  outStr.AppendASCII(CSP_CSPDirectiveToString(
> +    nsIContentSecurityPolicy::UPGRADE_IF_INSECURE_DIRECTIVE));

Nit: just put this all on one line.

::: dom/security/nsCSPUtils.h
@@ +365,5 @@
> +      { return false; }
> +
> +    void toString(nsAString& outStr) const;
> +
> +    void addSrcs(const nsTArray<nsCSPBaseSrc*>& aSrcs);

Why not just define the addSrcs function here too?
Attachment #8614866 - Flags: review?(sstamm)
Attachment #8614876 - Flags: review?(sstamm) → review+
Attachment #8614874 - Flags: review?(sstamm) → review+
Comment on attachment 8614879 [details] [diff] [review]
bug_1139297_upgrade_insecure_test_4_referrer.patch

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

::: dom/base/test/csp/test_upgrade_insecure_referrer.html
@@ +62,5 @@
> +    queryResults();
> +  }
> +  myXHR.onerror = function(e) {
> +    ok(false, "could not query results from server (" + e.message + ")");
> +    finishTest();

Where is finishTest() defined?

@@ +67,5 @@
> +  }
> +  myXHR.send();
> +
> +  // give it some time and run the testpage
> +  SimpleTest.executeSoon(loadNextTest);

I think you want to do this "do next test" inside the xhr onload/onerror.  Right now there could be a race if the xhrs take a long time to complete (where counter = tests.length and we're still waiting on the xhrs to come back).
Attachment #8614879 - Flags: review?(sstamm) → review-
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #44)
> Where is finishTest() defined?

That is a copy paste error - I'll fix that.


> > +  // give it some time and run the testpage
> > +  SimpleTest.executeSoon(loadNextTest);
>
> I think you want to do this "do next test" inside the xhr onload/onerror. 
> Right now there could be a race if the xhrs take a long time to complete
> (where counter = tests.length and we're still waiting on the xhrs to come
> back).

Well, not quite. The XHR response is handled asynchronously() on the server and wets till the image request was received so it can return back the referrer of the image request. I agree, it's suboptimal, but SimpleTest.executionSoon() should give it enough time. Anyway, I can slighlty rewrite the code, so that it doesn't matter which request (the XHR for quering the result, or the img request) wins the race.
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #43)
> > +bool
> > +nsCSPContext::UpgradeInsecureRequestsFromNestedContext(nsIDocument* aDocument)
> 
> Why is this function in nsCSPContext and not in nsDocument?  Won't it be
> needed by all loads, such as those in documents that don't have a CSP?
> 
> This could be a perf issue if we have to search through the document tree
> for every subresource load.  Why not put this nsDocument and cache the
> result for "some ancestor wants upgrades"?

I totally agree, I will refactor that part.

> ::: dom/security/nsCSPUtils.cpp
> @@ +295,5 @@
> > +  // http://www.w3.org/TR/CSP2/#match-source-expression
> > +  if (aEnforcementScheme.EqualsASCII("http") &&
> > +      scheme.EqualsASCII("https")) {
> > +    return true;
> > +  }
> 
> If you're going to refactor/change this code that decides whether or not to
> allow https loads for non-https protected documents, please ensure we have
> adequate test coverage for this case.

We do, I wrote tests for that a while ago, see:
Bug 826805 - Allow http and https for scheme-less sources

> ::: dom/security/nsCSPUtils.h
> > +    void addSrcs(const nsTArray<nsCSPBaseSrc*>& aSrcs);
> 
> Why not just define the addSrcs function here too?

You mean inline? Yes, we should do that.
Comment on attachment 8614878 [details] [diff] [review]
bug_1139297_upgrade_insecure_test_3_reports.patch

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

Just some minors.

::: dom/base/test/csp/file_upgrade_insecure_reporting_server.sjs
@@ +6,5 @@
> +// small red image
> +const BASE64_IMG =
> +  "iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12" +
> +  "P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==";
> +const IMG_BYTES = atob(BASE64_IMG);

Why not just use the string literal as the argument to atob?

@@ +62,5 @@
> +    return;
> +  }
> +
> +  // (4) Finally we receive the report, let's return the request from (3)
> +  // signaling that we received the report correctly

I don't understand how this returns the request from (3).  Do you mean (1)?

::: dom/base/test/csp/test_upgrade_insecure_reporting.html
@@ +13,5 @@
> +<script class="testbody" type="text/javascript">
> +
> +/* Description of the test:
> + * We load a page with the following CSP and CSP report only:
> + *   default-src https:

This is not quite accurate... the policies in the .sjs are not this policy.  Maybe reference that file?

@@ +16,5 @@
> + * We load a page with the following CSP and CSP report only:
> + *   default-src https:
> + *
> + * Within that page we load an image over http, make sure that the violation report was
> + * sent but the image was also correctly loaded.

Please make it clearer why, although the image loaded, the violation report should still be sent.  Something like "because the enforced policy upgrades insecure requests and the report-only one does not, we should receive a violation report but the image should still load via https".

@@ +23,5 @@
> +var expectedResults = 2;
> +
> +function finishTest() {
> +  // let's wait till the image was loaded and the report was received
> +  if (--expectedResults != 0) {

Nit: please use > 0.

@@ +36,5 @@
> +  // the violation report from the report only policy.
> +  var myXHR = new XMLHttpRequest();
> +  myXHR.open("GET", "file_upgrade_insecure_reporting_server.sjs?queryresult");
> +  myXHR.onload = function(e) {
> +    is(myXHR.responseText, "report-ok", "csp-report was correclty sent");

Nit: typo in correclty.

@@ +48,5 @@
> +
> +  // (2) We load a page that is served using a CSP and a CSP report only which loads
> +  // an image over http.
> +  var src = "https://example.com/tests/dom/base/test/csp/file_upgrade_insecure_reporting_server.sjs?toplevel";
> +  document.getElementById("testframe").src = src;

Why not just assign the literal string to the element.src?
Attachment #8614878 - Flags: review?(sstamm) → review+
Comment on attachment 8614873 [details] [diff] [review]
bug_1139297_upgrade_insecure_forms.patch

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

Do we have a test for this?

::: dom/html/HTMLFormElement.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "mozilla/dom/HTMLFormElement.h"
> +#include "mozilla/dom/nsCSPUtils.h"
> +#include "mozilla/dom/nsCSPContext.h"

move these 2 in the next block.
Attachment #8614873 - Flags: review?(amarchesini) → review+
Comment on attachment 8614872 [details] [diff] [review]
bug_1139297_upgrade_insecure_websocket.patch

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

Can you submit a new patch on top of m-c/m-i?

::: dom/base/WebSocket.cpp
@@ +8,5 @@
>  #include "mozilla/dom/WebSocketBinding.h"
>  #include "mozilla/net/WebSocketChannel.h"
>  
> +#include "mozilla/dom/nsCSPUtils.h"
> +#include "mozilla/dom/nsCSPContext.h"

move this in the alphabetic order into the rest of the headers.

@@ +22,5 @@
>  #include "mozilla/dom/WorkerScope.h"
>  #include "nsIScriptGlobalObject.h"
>  #include "nsIDOMWindow.h"
>  #include "nsIDocument.h"
> +#include "nsIDocument.h"

this is not needed.

@@ +1467,5 @@
> +  nsCOMPtr<nsIURI> uri;
> +  {
> +    nsresult rv = NS_NewURI(getter_AddRefs(uri), mURI);
> +
> +    // We crash here because we are sure that mURI is a valid URI, so either we

is this 80chars?

@@ +1469,5 @@
> +    nsresult rv = NS_NewURI(getter_AddRefs(uri), mURI);
> +
> +    // We crash here because we are sure that mURI is a valid URI, so either we
> +    // are OOM'ing or something else bad is happening.
> +    if (NS_FAILED(rv)) {

MOZ_ASSERT(NS_SUCCEEDED(rv));

@@ +1472,5 @@
> +    // are OOM'ing or something else bad is happening.
> +    if (NS_FAILED(rv)) {
> +      MOZ_CRASH();
> +    }
> +  }

All this code is already in m-c. Can you rebase this patch?
Attachment #8614872 - Flags: review?(amarchesini) → review-
(In reply to Andrea Marchesini (:baku) from comment #49)
> > +    // We crash here because we are sure that mURI is a valid URI, so either we
> > +    // are OOM'ing or something else bad is happening.
> > +    if (NS_FAILED(rv)) {
> 
> MOZ_ASSERT(NS_SUCCEEDED(rv));

I can certainly do that, but that's not my code, it's already there, but happy to fix that legacy code up.

> @@ +1472,5 @@
> > +    // are OOM'ing or something else bad is happening.
> > +    if (NS_FAILED(rv)) {
> > +      MOZ_CRASH();
> > +    }
> > +  }
> 
> All this code is already in m-c. Can you rebase this patch?

I think I don't fully understand what you mean. I haven't written most of that code, but in order to make upgrade-insecure-requests work I had to move the contentPolicy check a little further up in that function because otherwise insecure websockets (ws) will fail within https pages (see mSecure). That's why I moved the contentPolicy check up so that the CSP directive upgrade insecure can potentially update the websocket from ws to wss. I added a comment explaining what we are doing. I am happy to extend that comment if needed. Rebasing will not change any of the code here. Do you still want me to add a new patch and flag you again for review?
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #48)
> Comment on attachment 8614873 [details] [diff] [review]
> bug_1139297_upgrade_insecure_forms.patch
> Do we have a test for this?

Certainly do - see bug_1139297_upgrade_insecure_test_2_simple_upgrades.patch
Comment on attachment 8614872 [details] [diff] [review]
bug_1139297_upgrade_insecure_websocket.patch

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

Sorry, my fault for the previous review. Ignore the previous comments.
Can you use nsIURI to replace the scheme instead working with strings?

::: dom/base/WebSocket.cpp
@@ +8,5 @@
>  #include "mozilla/dom/WebSocketBinding.h"
>  #include "mozilla/net/WebSocketChannel.h"
>  
> +#include "mozilla/dom/nsCSPUtils.h"
> +#include "mozilla/dom/nsCSPContext.h"

move this in the alphabetic order into the rest of the headers.

@@ +22,5 @@
>  #include "mozilla/dom/WorkerScope.h"
>  #include "nsIScriptGlobalObject.h"
>  #include "nsIDOMWindow.h"
>  #include "nsIDocument.h"
> +#include "nsIDocument.h"

this is not needed.

@@ +1505,5 @@
> +  // the scheme is about to be upgraded.
> +  bool upgradeInsecureRequests =
> +    nsCSPContext::UpgradeInsecureRequestsFromNestedContext(originDoc);
> +  if (upgradeInsecureRequests) {
> +    MOZ_ASSERT(mURI.Find("ws://") != -1, "websocket must have ws:// scheme");

SchemeIs on uri?

@@ +1511,5 @@
> +    // let's use the old specification before the upgrade for logging
> +    NS_ConvertUTF8toUTF16 reportSpec(mURI);
> +
> +    // upgrade the request from ws:// to wss:// and mark as secure
> +    mURI.ReplaceSubstring("ws://", "wss://");

can you use nsIURI instead replacing the string?
(In reply to Andrea Marchesini (:baku) from comment #52)
> can you use nsIURI instead replacing the string?

Yes I can certainly do that. In fact I was about to do that. In fact I did that in my initial implementation, but then other patches made me realize that some nsURIs are immutable. I assume this is not the case here and will also not be the case in the future. If so, I will go ahead and incorporate your suggestions. Thanks for the reviews!
Attachment #8614866 - Flags: feedback+
Comment on attachment 8614868 [details] [diff] [review]
bug_1139297_upgrade_insecure_loadinfo.patch

+  /**
+   * Whether the toplevel document uses the CSP directive
+   * 'upgrade-insecure-requests' to indicate that all subresource
+   * loads should be upgraded to https. Used to indentify upgrade
s/indentify/identify

+   * requests in e10s where the loadingDocument is not available.
+   *
Attachment #8614868 - Flags: feedback+
Comment on attachment 8614869 [details] [diff] [review]
bug_1139297_upgrade_insecure_mcb.patch

You could potentially put this block of code higher up.  Either right after we check if the parent is HTTPS:
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#587 or even as high up as http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#492 where we are checking the scheme of aContentLocation.  If you put it at #492, then we would skip all the code that looks for the principal and location of the loadingContext.  Not sure which of these is more expensive.  Most of the time, we will get the principal right away; if its https we skip the rest of the code.  And most of the time, the CSP directive will be false and hence a no-op.


Some nits on the comment:
+  // Potentially the page uses the CSP directive 'upgrade-insecure-requests'. In
The page might have set the CSP directive 'upgrade-insecure-requests'.  In

+  // such a case allow the http: load to succeed with the promise that the channel
+  // gets upgraded to https before fetching any data from the netwerk. Please see:
s/gets/will get/

+  // --> nsHttpChannel::Connect()
+  // Please note that the CSP directive 'upgrade-insecure-requests' only applies
+  // to http: and ws: (for websockets). Websockets are not subject to mixed content
+  // blocking since insecure websockets are not allowed within secure pages. Hence,
+  // only skip mixed content blocking if the subresource load uses http: and the
we only have to check against http: here. Skip mixed content blocking if the subresource load uses http: and the

+  // CSP directive 'upgrade-insecure-requests' is present on the page.
Attachment #8614869 - Flags: review?(tanvi) → feedback+
Comment on attachment 8614871 [details] [diff] [review]
bug_1139297_upgrade_insecure_netwerk.patch

+        // It's time to fulfill the promise to CSP and mixed content blocking to
the promise to CSP, mixed content blocker, and CORS to
Attachment #8614871 - Flags: feedback+
Comment on attachment 8614869 [details] [diff] [review]
bug_1139297_upgrade_insecure_mcb.patch

I've feedback+'ed for now, and would like to review the next version of this patch.  Thanks!
Comment on attachment 8614877 [details] [diff] [review]
bug_1139297_upgrade_insecure_test_2_simple_upgrades.patch

Add two more tests.
1) Add a test where the upgrade directive is the only directive set (so not affected by default-src http: or default-src https:)

2) Add a test for the following case:
* the top level page is https
* the page has a csp that doesn't restrict http or https.  i.e. default-src * without the upgrade directive

This will test that the Mixed Content Blocker isn't bypassed when there is a csp without an upgrade directive.
Comment on attachment 8614877 [details] [diff] [review]
bug_1139297_upgrade_insecure_test_2_simple_upgrades.patch

The behavior of checkResults() and queryResults() is a little confusing.  I talked to Christoph over vidyo and he will add some comments to make it more clear what they do and potentially change the method names.

Also, they seem to call each other in an infinite loop.  So after checkResults() is done checking the last test, it calls queryResult() which does an XHR to the server that will never get a response.  Add a check to see if the last test is finished line to 

+function queryResults() {
if (counter == tests.length) {
return;
}
+  var myXHR = new XMLHttpRequest();
+  myXHR.open("GET", "file_upgrade_insecure_server.sjs?queryresult");

r- for now, mostly because of the two other tests I mentioned in comment 58.
Attachment #8614877 - Flags: review?(tanvi) → review-
Hey Smaug, as discussed, the CSP directive upgrade-insecure-requests also applies to subdocuments. I was wondering whether you can help me out a little. Lets assume something like:

<toplevel A [uses CSP 'upgrade-insecure-requests']
  <iframe B
    <iframe C
      <img D

When we start loading D, we have to figure whether C, B, or A makes use of upgrade-insecure. We would have to do that within mixedContentBlocker, CORS and when we create the loadInfo for the channel, which seems semi-optimal to me.

What would be great is a flag on the document which tells us if any of the document's ancestors makes use of the directive. In the attached patch I am setting a flag on the document which consults nsCSPContext::UpgradeInsecureRequestsFromNestedContext. Than function returns true if either the current document's CSP makes us of upgradeInsecure or any of the document's ancestors (B,A). The problem is, at the time we consult UpgradeInsecureRequestsFromNestedContext the document has no link to it's parent yet. In other words, |aDocument->GetParentDocument()| returns a nullptr. Any suggestions when to set the flag or any other idea which would avoid walking the ancestor chain for every subresource load? Thanks for you help!
Flags: needinfo?(bugs)
Attachment #8620709 - Flags: feedback?(bugs)
Could you use docshell tree to access the parent? Docshell tree sure should be there when
loading an iframe.  One can access document from docshell (nsCOMPtr<nsIDocument> d = do_GetInterface(docShell); )
Flags: needinfo?(bugs)
Comment on attachment 8620709 [details] [diff] [review]
bug_1139297_upgrade_insecure_document.patch

(I think my needinfo comment was enough as a feedback for this patch too.)
Attachment #8620709 - Flags: feedback?(bugs)
Sid, here is a new patch, please note that the upgrade-insecure-request flag is now set on the document and propagated down to nested contexts, so we dont' have to traverser the document tree ever (see *_document.patch).
Attachment #8614866 - Attachment is obsolete: true
Attachment #8621344 - Flags: review?(sstamm)
Olli, let's propagate the information down from parent to nested contexts using the docshell. I haven't had time to address any of you concerns regarding CORS, those will follow soon.
Attachment #8620709 - Attachment is obsolete: true
Attachment #8621345 - Flags: review?(bugs)
Carrying over r+ from sworkman.
Attachment #8614868 - Attachment is obsolete: true
Attachment #8614868 - Flags: review?(jonas)
Attachment #8621347 - Flags: review+
Comment on attachment 8621347 [details] [diff] [review]
bug_1139297_upgrade_insecure_loadinfo.patch

Jonas, that change should be fine, right?
Attachment #8621347 - Flags: review?(jonas)
(In reply to Tanvi Vyas [:tanvi] from comment #55)
> Comment on attachment 8614869 [details] [diff] [review]
> bug_1139297_upgrade_insecure_mcb.patch
> 
> You could potentially put this block of code higher up.  Either right after
> we check if the parent is HTTPS:
> http://mxr.mozilla.org/mozilla-central/source/dom/security/
> nsMixedContentBlocker.cpp#587 or even as high up as
> http://mxr.mozilla.org/mozilla-central/source/dom/security/
> nsMixedContentBlocker.cpp#492

Well, we don't have the docshell available further up, we could of course rearrange all of that code, but i would rather not touch it. Putting it here, right after the docshell was queried by MixedContentBlocker seems like the right place to me. I hope you agree.
Attachment #8614869 - Attachment is obsolete: true
Attachment #8621350 - Flags: review?(tanvi)
Carrying over r+ from sworkman.
Attachment #8614871 - Attachment is obsolete: true
Attachment #8621351 - Flags: review+
Hey Baku, I addressed all of your concerns and refactored the code to use an nsIURI instead of a string for the URI.
Attachment #8614872 - Attachment is obsolete: true
Attachment #8621353 - Flags: review?(amarchesini)
Carrying over r+ from baku.
Attachment #8614873 - Attachment is obsolete: true
Attachment #8621354 - Flags: review+
Comment on attachment 8621345 [details] [diff] [review]
bug_1139297_upgrade_insecure_document.patch

>+  // The CSP directive upgrade-insecure-requests not only applies to the
>+  // toplevel document, but also to nested documents. Let's propagate that
>+  // flag from the parent to the nested document.
>+  nsCOMPtr<nsIInterfaceRequestor> ir(do_QueryInterface(this->GetDocShell()));
>+  nsCOMPtr<nsIDocShellTreeItem> treeItem(do_GetInterface(ir));
Why the use of nsIInterfaceRequestor? and since nsIDocShell extends nsIDocShellTreeItem, no
need for the QI either.

So, 
nsIDocShell* treeItem = GetDocShell();

>+  if (treeItem) {
>+    nsCOMPtr<nsIDocShellTreeItem> parentTreeItem;
>+    nsresult rv = treeItem->GetParent((getter_AddRefs(parentTreeItem)));
You want to use GetSameTypeParent

>+    NS_ENSURE_SUCCESS(rv, rv);
I would drop this.

>+    if (parentTreeItem) {
>+       mUpgradeInsecureRequests =
Please use 2 spaces for indentation consistently.

>+   * If True, this flag indicates that all subresource loads for this
s/True/true/
Attachment #8621345 - Flags: review?(bugs) → review+
Comment on attachment 8621353 [details] [diff] [review]
bug_1139297_upgrade_insecure_websocket.patch

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

Thanks!

::: dom/base/WebSocket.cpp
@@ +12,5 @@
>  #include "jsfriendapi.h"
>  #include "mozilla/DOMEventTargetHelper.h"
>  #include "mozilla/net/WebSocketChannel.h"
> +#include "mozilla/dom/nsCSPUtils.h"
> +#include "mozilla/dom/nsCSPContext.h"

alphabetic order. Move them after line 17

@@ +535,5 @@
>    }
>  
> +  nsAutoCString spec;
> +  nsresult rv = mURI->GetSpec(spec);
> +  NS_ENSURE_SUCCESS(rv, rv);

I still don't know if we like or not this macros, but the rest of the code doesn't use them (too much)... so
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}
Attachment #8621353 - Flags: review?(amarchesini) → review+
Comment on attachment 8621344 [details] [diff] [review]
bug_1139297_upgrade_insecure_csp.patch

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

Happy to see you got the "upgrade" status into the document to speed stuff up.  r=me

::: dom/security/nsCSPContext.cpp
@@ +14,5 @@
>  #include "nsIAsyncVerifyRedirectCallback.h"
>  #include "nsIClassInfoImpl.h"
>  #include "nsIDocShell.h"
>  #include "nsIDocShellTreeItem.h"
> +#include "nsIDocument.h"

I don't think you need this include anymore.
Attachment #8621344 - Flags: review?(sstamm) → review+
Comment on attachment 8621350 [details] [diff] [review]
bug_1139297_upgrade_insecure_mcb.patch

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #67)
> Well, we don't have the docshell available further up, we could of course
> rearrange all of that code, but i would rather not touch it. Putting it
> here, right after the docshell was queried by MixedContentBlocker seems like
> the right place to me. I hope you agree.
Ah, I see.  Then lets move it to before the call to GetAllowMixedContentAndConnectionData.  That method doesn't need to be called if we are going to upgrade-insecure-requests.

To be specific, add your code after line 590:
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#590

Also, please fix spacing and newlines in the comments.
Attachment #8621350 - Flags: review?(tanvi) → review-
Clearing the needinfo for baku - he already answered all of my questions and reviewed all of the patches he was flagged for - thanks!
Flags: needinfo?(amarchesini)
Hey Tanvi, here we go, I moved the code up a few lines and cleaned up the comment.
Attachment #8621350 - Attachment is obsolete: true
Attachment #8622785 - Flags: review?(tanvi)
Hey Olli, thanks for all your help and feedback. I am still not completely sure about the originalURI check, maybe we don't even need to that because we already do that check right before we call checkUpgradeInsecureRequestsPreventsCORS?
Attachment #8614870 - Attachment is obsolete: true
Attachment #8622788 - Flags: review?(bugs)
And there are the promised tests for CORS.
Attachment #8622789 - Flags: review?(bugs)
Comment on attachment 8614877 [details] [diff] [review]
bug_1139297_upgrade_insecure_test_2_simple_upgrades.patch

Hey Sid, I haven't had the time to incorporate Tanvi's suggestions. Clearing flag for now and will re-flag once I have incorporated Tanvi's feedback.
Attachment #8614877 - Flags: review?(sstamm)
Attachment #8622789 - Flags: review?(bugs) → review+
Comment on attachment 8622788 [details] [diff] [review]
bug_1139297_upgrade_insecure_cors.patch

>+// Please note that the CSP directive 'upgrade-insecure-requests' relies
>+// on the promise that channels get updated from http: to https: before
>+// the channel fetches any data from the netwerk. Such channels should
>+// not be blocked by CORS and marked as cross origin requests. E.g.:
>+// toplevel page: https://www.example.com loads
>+//           xhr: http://www.example.com/foo which gets updated to
>+//                https://www.example.com/foo
>+// In such a case we should bail out of CORS and rely on the promise that
>+// nsHttpChannel::Connect() upgrades the request from http to https.
>+bool
>+checkUpgradeInsecureRequestsPreventsCORS(nsIPrincipal* aRequestingPrincipal,
>+                                         nsIChannel* aChannel)
method names should start with capital letter, so CheckUpgrade...


Comment 40 is still unanswered, well, also 38.
"In other words, what is the behavior of OnRedirectVerifyCallback (which calls UpdateChannel) in this new world?"
Who calls OnRedirectVerifyCallback and when and based on what? Could you point to the code doing the calls?
How should we deal with mWithCredentials check at the end of UpdateChannel?
Attachment #8622788 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #80)
> Comment on attachment 8622788 [details] [diff] [review]
> bug_1139297_upgrade_insecure_cors.patch
> >+checkUpgradeInsecureRequestsPreventsCORS(nsIPrincipal* aRequestingPrincipal,
> >+                                         nsIChannel* aChannel)
> method names should start with capital letter, so CheckUpgrade...

Sure, will fix that for the next revision.

> Comment 40 is still unanswered, well, also 38.
> "In other words, what is the behavior of OnRedirectVerifyCallback (which
> calls UpdateChannel) in this new world?"
> Who calls OnRedirectVerifyCallback and when and based on what? Could you
> point to the code doing the calls?
> How should we deal with mWithCredentials check at the end of UpdateChannel?

All of that remains unchanged, whoever called OnRedirectVerifyCallback before
still calls that function. More precisely, whenever a channel gets redirected
the CORS code receives a call to ::AsyncOnChannelRedirect which then calls
OnRedirectVerifyCallback which then calls UpdateChannel. Now, if the request
is not cross site then then UpdateChannel returns NS_OK without performing
any CORS. The added code for upgrade insecure bsically performs the same
task but bails out of CORS if:
* the upgrade insecure flag is set, and
* the request would have been marked as cross site just because the scheme is
different. To be precise, the new added code only bails out of CORS if the
flag upgrade insecure is set, the host matches, and only the scheme is http
instead of https. Otherwise CORS should be performed as usual. Similarly,
the same code path is used for redirects on the preflight channel, where
OnRedirectVerifyCallback is called from within nsCORSPreflightListener which
gets notified whenever a preflight channel is redirected.

Let me also address comment 38 and 40 to make sure I cover all of your questions.

(In reply to Olli Pettay [:smaug] from comment #38)
> So even in case of redirection we don't need CORS handling for upgrades?

As explained above, whenever CORS is needed we should perform cors. The only
difference is that we don't should classify a request to be cross site if
only the scheme is http instead of https. Please note, the host must still
match, even for upgrade insecure requests, otherwise CORS needs to be performed.

> I mean, 
> domain A (which has 'upgrade insecure requests' flag) does cors to domain B,
> and domain redirects to (http) domain A. So we should do http->https for
> that latter case. We get UpdateChannel call for
> domain B -> domain A(http) redirection, (and the patch then just bypasses
> part of the CORS handling since it is http and we have the upgrade flag),
> and I assume we'll get another UpdateChannel call for 
> the http->https upgrade, right?

Yes, I added a testcase exactly for this scenario, please see:
bug_1139297_upgrade_insecure_test_4_cors.patch

> In other words, what is the behavior of OnRedirectVerifyCallback (which
> calls UpdateChannel) in this new world?

I think I already addressed that question on the top of that comment.

(In reply to Olli Pettay [:smaug] from comment #40)
> right now the patch seems to bypass cors whenever doing
> someurl->http_url_of_requesting_domain, and
> I guess the spec hints that http->https should happen always, if requestor
> has the flag.

I am not sure I can quite follow the question here. But again, CORS should
only be bypassed if the request would be classified as cross site but in fact
is not cross site because the request gets upgraded from http to https.

Let's do a little example, just to hightlight the idea:
* Page https://a.com performs a cors request to http://b.com, the cors performs
CORS as usual.
* Page https://a.com performs a cors request to http://a.com and exactly here is
the difference. In this case CORS should not be performed, because the request
to http://a.com gets upgraded to https://a.com in httpchannel::Connect(). So
we should bail out of CORS because the request is actually not cross site.

> So also someDomain->someOtherDomain_http should be bypassed, since
> someOtherDomain_http should get
> upgraded to someOtherDomain_https. So if that is the wanted behavior, you
> need to compare originalURI to url

This is the question I am still not sure, do we need to compare against the
original URI withn CheckUpgradeInsecureRequestsPreventsCORS?


Hopefuly I was able to answer all of your questions, please let me know otherwise.
I am also happy to hop on vidyo if that makes things easier. Thanks for looking
so closely.
Flags: needinfo?(bugs)
So, if https-domain A loads http-domain B, but that does redirect to http-domain A which is then
upgraded to https-domain A request, how many OnRedirectVerifyCallback calls we'll we get? 1 or 2? And what is the url of the channel when such call is made? And what code does the OnRedirectVerifyCallback call?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #82)
> So, if https-domain A loads http-domain B, but that does redirect to
> http-domain A which is then
> upgraded to https-domain A request,

So let's walk through the example scenario in detail; which is by the way the example in the testcase, so let's use the values of the testcase:

> (A) CLIENT https://test1.example.com loads http://test2.example.com
(a) nsCORSListenerProxy::Init -> UpdateChannel -> CheckUpgradeInsecureRequestsPreventsCORS
(b) CheckUpgradeInsecureRequestsPreventsCORS
      channelURI  : http://test2.example.com/tests/dom/security/test/csp/file_upgrade_insecure_cors_server.sjs?test2
      principalURI: https://test1.example.com/tests/dom/security/test/csp/file_testserver.sjs?file=tests/dom/security/test/csp/file_upgrade_insecure_cors.html&csp=upgrade-insecure-requests%3B%20script-src%20%27unsafe-inline%27
      originalURI : http://test2.example.com/tests/dom/security/test/csp/file_upgrade_insecure_cors_server.sjs?test2
(c) CheckUpgradeInsecureRequestsPreventsCORS returns false - domains are different!
(d) HttpChannel::Connect() upgrades http://test2.example.com/tests/dom/security/test/csp/file_upgrade_insecure_cors_server.sjs?test2 to use https
(e) OnRedirectVerifyCallback -> UpdateChannel -> CheckUpgradeInsecureRequestsPreventsCORS
(f) CheckUpgradeInsecureRequestsPreventsCORS
      channelURI  : https://test2.example.com/tests/dom/security/test/csp/file_upgrade_insecure_cors_server.sjs?test2
      principalURI: https://test1.example.com/tests/dom/security/test/csp/file_testserver.sjs?file=tests/dom/security/test/csp/file_upgrade_insecure_cors.html&csp=upgrade-insecure-requests%3B%20script-src%20%27unsafe-inline%27
      originalURI : https://test2.example.com/tests/dom/security/test/csp/file_upgrade_insecure_cors_server.sjs?test2
(g) CheckUpgradeInsecureRequestsPreventsCORS returns false - channel is not http!

> (B) SERVER
(a) Sets CORS headers, and
(b) responds with a 302 to
    http://test1.example.com/tests/dom/security/test/csp/file_upgrade_insecure_cors_server.sjs?redir-test2

> (C) CLIENT
(a) OnRedirectVerifyCallback -> UpdateChannel -> checkUpgradeInsecureRequestsPreventsCORS
(b) checkUpgradeInsecureRequestsPreventsCORS
      channelURI  : http://test1.example.com/tests/dom/security/test/csp/file_upgrade_insecure_cors_server.sjs?redir-test2
      principalURI: https://test1.example.com/tests/dom/security/test/csp/file_testserver.sjs?file=tests/dom/security/test/csp/file_upgrade_insecure_cors.html&csp=upgrade-insecure-requests%3B%20script-src%20%27unsafe-inline%27
      originalURI : http://test1.example.com/tests/dom/security/test/csp/file_upgrade_insecure_cors_server.sjs?redir-test2
(c) CheckUpgradeInsecureRequestsPreventsCORS returns true - http channel as well as upgrade insecure flag found!
(d) HttpChannel::Connect() upgrades http://test1.example.com/tests/dom/security/test/csp/file_upgrade_insecure_cors_server.sjs?redir-test2 to use https
(e) OnRedirectVerifyCallback -> UpdateChannel -> CheckUpgradeInsecureRequestsPreventsCORS
(f) CheckUpgradeInsecureRequestsPreventsCORS
    channelURI  : https://test1.example.com/tests/dom/security/test/csp/file_upgrade_insecure_cors_server.sjs?redir-test2
    principalURI: https://test1.example.com/tests/dom/security/test/csp/file_testserver.sjs?file=tests/dom/security/test/csp/file_upgrade_insecure_cors.html&csp=upgrade-insecure-requests%3B%20script-src%20%27unsafe-inline%27
    originalURI : https://test1.example.com/tests/dom/security/test/csp/file_upgrade_insecure_cors_server.sjs?redir-test2
(g) CheckUpgradeInsecureRequestsPreventsCORS returns false - channel not http!

> (D) SERVER
(a) sets CORS headers, and
(a) responds with "test2-cors-ok"

> (E) CLIENT
(a) test ends!


> how many OnRedirectVerifyCallback calls we'll we get? 1 or 2?
Actually 3 (if we include the inital one which is called from httpChannel::Connect()).

> And what is the url of the channel when such call is made?
I included the channelURI, the principalURI as well as the originalURI in the explanation above.

> And what code does the OnRedirectVerifyCallback call?
Whenever a channel gets redirected, CORS gets a callback to nsCORSListenerProxy::AsyncOnChannelRedirect
which then call OnRedirectVerifyCallback.
Flags: needinfo?(bugs)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #83)
> (c) CheckUpgradeInsecureRequestsPreventsCORS returns false - domains are
> different!
> (d) HttpChannel::Connect() upgrades
> http://test2.example.com/tests/dom/security/test/csp/
> file_upgrade_insecure_cors_server.sjs?test2 to use https
> (e) OnRedirectVerifyCallback -> UpdateChannel ->
> CheckUpgradeInsecureRequestsPreventsCORS
> (f) CheckUpgradeInsecureRequestsPreventsCORS
>       channelURI  :
> https://test2.example.com/tests/dom/security/test/csp/
> file_upgrade_insecure_cors_server.sjs?test2
>       principalURI:
> https://test1.example.com/tests/dom/security/test/csp/file_testserver.
> sjs?file=tests/dom/security/test/csp/file_upgrade_insecure_cors.
> html&csp=upgrade-insecure-requests%3B%20script-src%20%27unsafe-inline%27
>       originalURI :
> https://test2.example.com/tests/dom/security/test/csp/
> file_upgrade_insecure_cors_server.sjs?test2
> (g) CheckUpgradeInsecureRequestsPreventsCORS returns false - channel is not
> http!

And my question has been mostly what code calls OnRedirectVerifyCallback in (e), 
since we're not doing really a redirect but some internal http -> https magic...

> > how many OnRedirectVerifyCallback calls we'll we get? 1 or 2?
> Actually 3 (if we include the inital one which is called from
> httpChannel::Connect()).
Ok, so httpchannel::connect calls it, even though there isn't any redirection happening.
Fine. Odd, but I guess it is fine.




> > And what code does the OnRedirectVerifyCallback call?
> Whenever a channel gets redirected, CORS gets a callback to
> nsCORSListenerProxy::AsyncOnChannelRedirect
> which then call OnRedirectVerifyCallback.
Well, I was trying to ask that code actually calls AsyncOnChannelRedirect.
Flags: needinfo?(bugs)
Comment on attachment 8621347 [details] [diff] [review]
bug_1139297_upgrade_insecure_loadinfo.patch

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

This looks good. However it doesn't feel like this approach scales. There are too many places where we need to add every flag that we add to loadinfo.

Can we refactor things so that it's more contained to just the loadinfo code?

Ben said he will comment on how to do this.
(In reply to Jonas Sicking (:sicking) from comment #85)
> Comment on attachment 8621347 [details] [diff] [review]
> bug_1139297_upgrade_insecure_loadinfo.patch
> 
> Review of attachment 8621347 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good. However it doesn't feel like this approach scales. There
> are too many places where we need to add every flag that we add to loadinfo. 
> Can we refactor things so that it's more contained to just the loadinfo code?
> Ben said he will comment on how to do this.

Jonas, I agree it's not great that we have to propagate so many flags around, but that's how we have done it so far. I would rather not pollute this bug with that refactoring. If you don't mind I would like to land as is and immediately refactor those bits within a separate bug. How would you feel about that?
Flags: needinfo?(jonas)
Comment on attachment 8621347 [details] [diff] [review]
bug_1139297_upgrade_insecure_loadinfo.patch

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

I can live with that as long as you make sure to collaborate with bent to fix this stuff up.
Attachment #8621347 - Flags: review?(jonas) → review+
Comment on attachment 8622785 [details] [diff] [review]
bug_1139297_upgrade_insecure_mcb.patch

Thanks Christoph!
Attachment #8622785 - Flags: review?(tanvi) → review+
(In reply to Jonas Sicking (:sicking) from comment #87)
> Comment on attachment 8621347 [details] [diff] [review]
> bug_1139297_upgrade_insecure_loadinfo.patch
> 
> Review of attachment 8621347 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can live with that as long as you make sure to collaborate with bent to
> fix this stuff up.

Yes, I will make sure that happens - thanks Jonas! (Clearing your needinfo here as well).
Flags: needinfo?(jonas)
(In reply to Tanvi Vyas [:tanvi] from comment #58)
> Add two more tests.
> 1) Add a test where the upgrade directive is the only directive set (so not
> affected by default-src http: or default-src https:)

I added a test for this, but since we are using inline scripts we have to somehow whitelist that inline script usage within the CSP, so I ended up using "script-src * 'unsafe-inline'". Since we make use of script-src we also have to whitelist the external loads of scripts, otherwise they would be blocked by CSP. At least we are not making use of default-src, which is basically what you wanted, right?
 
> 2) Add a test for the following case:
> * the top level page is https
> * the page has a csp that doesn't restrict http or https.  i.e. default-src
> * without the upgrade directive
> 
> This will test that the Mixed Content Blocker isn't bypassed when there is a
> csp without an upgrade directive.

This one is not quite easy to accomplish with the current test setup. As agreed over IRC, we can skip that test. When upgrade insecure requests is not used within CSP then mixed content blocker is not performing any additional tasks anyway. So we should be good here.
Attachment #8614877 - Attachment is obsolete: true
Attachment #8623350 - Flags: review?(tanvi)
Attachment #8623350 - Flags: review?(sstamm)
Addressed Sid's nits, carrying over r+ from Sid.
Attachment #8614878 - Attachment is obsolete: true
Attachment #8623352 - Flags: review+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #44)
> Comment on attachment 8614879 [details] [diff] [review]
> bug_1139297_upgrade_insecure_test_4_referrer.patch
> > +  myXHR.send();
> > +
> > +  // give it some time and run the testpage
> > +  SimpleTest.executeSoon(loadNextTest);
>
> I think you want to do this "do next test" inside the xhr onload/onerror. 
> Right now there could be a race if the xhrs take a long time to complete
> (where counter = tests.length and we're still waiting on the xhrs to come
> back).

So, I slightly refactored that code, but please keep in mind that the XHR request is handled async on the server. To be more precise the test works as follows:
* Send xhr to the server
* load testpage which loads an image
* once the server receives the image request, the server checks the referrer and send the result back to the client using the inital xhr request.
* xhr-onload checks the result and loads the next test.

Using SimpleTest.executesoon should give the xhr request more than enough time to be handled by the server before the testpage is loaded, so I don't think this will cause any race conditions.
Attachment #8614879 - Attachment is obsolete: true
Attachment #8623356 - Flags: review?(sstamm)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #89)
> (In reply to Jonas Sicking (:sicking) from comment #87)
> > Comment on attachment 8621347 [details] [diff] [review]
> > bug_1139297_upgrade_insecure_loadinfo.patch
> > 
> > Review of attachment 8621347 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I can live with that as long as you make sure to collaborate with bent to
> > fix this stuff up.

Files Bug 1175352 and flaged Ben for suggestions!
> bug_1139297_upgrade_insecure_cors.patch

Hey Olli, I am not sure whether you want any more information (debug info) at the moment. I am happy to provide whatever information we need to getting this patch ready for a review. Can you propose a way forward?
Flags: needinfo?(bugs)
I don't need more information I think, except a pointer to the code which calls AsyncOnChannelRedirect.
That would help me reviewing cors handling, if there is still something to review.
Flags: needinfo?(bugs)
Comment on attachment 8622788 [details] [diff] [review]
bug_1139297_upgrade_insecure_cors.patch

(In reply to Olli Pettay [:smaug] from comment #95)
> I don't need more information I think, except a pointer to the code which
> calls AsyncOnChannelRedirect.
> That would help me reviewing cors handling, if there is still something to
> review.

So, this is kind of magic, maybe that stacktrace provides the information you are looking for:

#01: nsCORSListenerProxy::AsyncOnChannelRedirect(nsIChannel*, nsIChannel*, unsigned int, nsIAsyncVerifyRedirectCallback*) (/home/ckerschb/moz/mc/dom/security/nsCORSListenerProxy.cpp:718)
#02: non-virtual thunk to nsCORSListenerProxy::AsyncOnChannelRedirect(nsIChannel*, nsIChannel*, unsigned int, nsIAsyncVerifyRedirectCallback*) (/home/ckerschb/moz/mc-obj-dbg/dom/security/Unified_cpp_dom_security0.cpp:793)
#03: nsAsyncRedirectVerifyHelper::DelegateOnChannelRedirect(nsIChannelEventSink*, nsIChannel*, nsIChannel*, unsigned int) (/home/ckerschb/moz/mc/netwerk/base/nsAsyncRedirectVerifyHelper.cpp:146)
#04: nsAsyncRedirectVerifyHelper::Run() (/home/ckerschb/moz/mc/netwerk/base/nsAsyncRedirectVerifyHelper.cpp:240)
#05: nsThread::ProcessNextEvent(bool, bool*) (/home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:849)
#06: NS_ProcessNextEvent(nsIThread*, bool) (/home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:265)
#07: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:95)
#08: MessageLoop::RunInternal() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:235)
#09: MessageLoop::RunHandler() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:228)
#10: MessageLoop::Run() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:201)
#11: nsBaseAppShell::Run() (/home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:165)
#12: nsAppStartup::Run() (/home/ckerschb/moz/mc/toolkit/components/startup/nsAppStartup.cpp:280)
#13: XREMain::XRE_mainRun() (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4263)
#14: XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4347)
#15: XRE_main (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4436)
#16: do_main(int, char**, nsIFile*) (/home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:214)


Re-setting the review flag just so that this bug shows up in your review queue. Thanks Olli!
Attachment #8622788 - Flags: review?(bugs)
What ends up creating nsAsyncRedirectVerifyHelper for the http->https upgrade?
(sorry being so annoying with the review, but I kind of want to understand what is happening in the background ;) )
(In reply to Olli Pettay [:smaug] from comment #97)
> What ends up creating nsAsyncRedirectVerifyHelper for the http->https
> upgrade?
> (sorry being so annoying with the review, but I kind of want to understand
> what is happening in the background ;) )

No, not at all annoying, thanks for putting in so much effort making sure we do the right thing here. I assume this is the stacktrace you are looking for, which is created the first time we do the upgrade form http->https within httpchannel::connect():


#01: nsAsyncRedirectVerifyHelper::Init(nsIChannel*, nsIChannel*, unsigned int, bool) (/home/ckerschb/moz/mc/netwerk/base/nsAsyncRedirectVerifyHelper.cpp:85)
#02: mozilla::net::nsHttpHandler::AsyncOnChannelRedirect(nsIChannel*, nsIChannel*, unsigned int) (/home/ckerschb/moz/mc/netwerk/protocol/http/nsHttpHandler.cpp:577)
#03: mozilla::net::nsHttpChannel::StartRedirectChannelToURI(nsIURI*, unsigned int) (/home/ckerschb/moz/mc/netwerk/protocol/http/nsHttpChannel.cpp:1875)
#04: mozilla::net::nsHttpChannel::StartRedirectChannelToHttps() (/home/ckerschb/moz/mc/netwerk/protocol/http/nsHttpChannel.cpp:1814)
#05: mozilla::net::nsHttpChannel::HandleAsyncRedirectChannelToHttps() (/home/ckerschb/moz/mc/netwerk/protocol/http/nsHttpChannel.cpp:1782)
#06: void nsRunnableMethodArguments<>::apply<mozilla::net::nsHttpChannel, void (mozilla::net::nsHttpChannel::*)()>(mozilla::net::nsHttpChannel*, void (mozilla::net::nsHttpChannel::*)()) (/home/ckerschb/moz/mc-obj-dbg/netwerk/protocol/http/../../../dist/include/nsThreadUtils.h:619)
#07: nsRunnableMethodImpl<void (mozilla::net::nsHttpChannel::*)(), true>::Run() (/home/ckerschb/moz/mc-obj-dbg/netwerk/protocol/http/../../../dist/include/nsThreadUtils.h:825)
#08: nsThread::ProcessNextEvent(bool, bool*) (/home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:849)
#09: NS_ProcessNextEvent(nsIThread*, bool) (/home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:265)
#10: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:95)
#11: MessageLoop::RunInternal() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:235)
#12: MessageLoop::RunHandler() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:228)
#13: MessageLoop::Run() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:201)
#14: nsBaseAppShell::Run() (/home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:165)
#15: nsAppStartup::Run() (/home/ckerschb/moz/mc/toolkit/components/startup/nsAppStartup.cpp:280)
#16: XREMain::XRE_mainRun() (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4263)
#17: XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4347)
#18: XRE_main (/home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4436)
#19: do_main(int, char**, nsIFile*) (/home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:214)
#20: main (/home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:478)
#21: __libc_start_main (/build/buildd/eglibc-2.19/csu/libc-start.c:321)
#22: _start (/home/ckerschb/moz/mc-obj-dbg/dist/bin/firefox)
Comment on attachment 8623356 [details] [diff] [review]
bug_1139297_upgrade_insecure_test_4_referrer.patch

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

Looks good!

::: dom/security/test/csp/file_upgrade_insecure_referrer_server.sjs
@@ +33,5 @@
> +      referrer = "";
> +    }
> +    // make sure the received image request was upgraded
> +    // otherwise we return not only the referrer but also
> +    // indicate that the request was not upgraded to https.

Please add a comment here that reminds the reader that the "upgrade" happens before the original, non-secure request hits the wire.  (Thus, there *should not* be an incoming HTTP request before the HTTPS one).

@@ +39,5 @@
> +                 "" : " but request is not https";
> +
> +    getObjectState("queryResult", function(queryResponse) {
> +      if (!queryResponse) {
> +        return;

Should this be an error?  Would it make sense to throw here if there's an error to get the test to stop earlier than the client might detect?

::: dom/security/test/csp/test_upgrade_insecure_referrer.html
@@ +48,5 @@
> +}
> +
> +function runNextTest() {
> +  // sends a request to the server which is processed async, which only
> +  // returns after the server has received all the expected requests.

In this case, "all the expected results" is 1, right?
Attachment #8623356 - Flags: review?(sstamm) → review+
[Carrying over r+ for testcase from smaug].


Hey Olli, as discussed in person last week, I added another test for redirecting to a different port. Unfortunately the way our *.sjs files work is tricky, because we can not listen to a different port for https other than 443, see [1]. Anyway, I manually verified that the port remains untouched if other than port 80, as the spec defines here [2]. I added a redirect to http using port 443. I know it's not great, but we should keep the test anyway. To sum it up, checkUpgradeInsecureRequestsPreventsCORS should *not* account for the port, because the SOP treats same scheme and host but different port as a different origin and the spec says only update to port 443 if port is 80. So no new surprises and expected behavior. Let me know if you have any further questions.

[1] http://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt#100
[2] https://w3c.github.io/webappsec/specs/upgrade/#upgrade-request
Attachment #8622789 - Attachment is obsolete: true
Flags: needinfo?(bugs)
Attachment #8627287 - Flags: review+
Comment on attachment 8622788 [details] [diff] [review]
bug_1139297_upgrade_insecure_cors.patch

Thanks.
So, just update c -> C


s/netwerk/network/
Flags: needinfo?(bugs)
Attachment #8622788 - Flags: review?(bugs) → review+
Comment on attachment 8623350 [details] [diff] [review]
bug_1139297_upgrade_insecure_test_2_simple_upgrades.patch

+    // images, scripts, etc. get queried twice, do not
+    // confuse the server by storing preloads
+    if (receivedQueries.contains(queryString)) {
+      return;
+    }
The comment says don't confuse the server by storing preloads, but I think the result of this is that we will be checking preloads without checking actual requests.  Christoph had a work around for this in mind, where we check both preloads and the actual request, and only override the preloaded result in the receivedQueries string if it is not "ok"?

+  // handle img redirect (https->http)
+  if (queryString == "img-redir") {
+    var newLocation =
+      "http://example.com/tests/dom/security/test/csp/file_upgrade_insecure_server.sjs?img-redir";
+    response.setStatusLine("1.1", 302, "Found");
+    response.setHeader("Location", newLocation, false);
+    return;
+  }
Given the code block above checks for queryString=="img-redir" and then returns, I'm not sure this code ever gets called.
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #99)
> ::: dom/security/test/csp/file_upgrade_insecure_referrer_server.sjs
> Please add a comment here that reminds the reader that the "upgrade" happens
> before the original, non-secure request hits the wire.  (Thus, there *should
> not* be an incoming HTTP request before the HTTPS one).

Makes sense, I extended the comment.


> > +    getObjectState("queryResult", function(queryResponse) {
> > +      if (!queryResponse) {
> > +        return;
> 
> Should this be an error?  Would it make sense to throw here if there's an
> error to get the test to stop earlier than the client might detect?

Other tests do it the same way, and honestly I would have to restructure the test to make that work. In case that really fails, then we would have to wait till the test times out. But as I said, other tests in our codebase do it the same way. I think that should be ok to use in this testcase too.

> > +function runNextTest() {
> > +  // sends a request to the server which is processed async, which only
> > +  // returns after the server has received all the expected requests.
> 
> In this case, "all the expected results" is 1, right?

That is absoultely correct - copy/paste error. Got that comment fixed.
Attachment #8623356 - Attachment is obsolete: true
Attachment #8627749 - Flags: review+
(In reply to Tanvi Vyas [:tanvi] from comment #102)
> +    // images, scripts, etc. get queried twice, do not
> +    // confuse the server by storing preloads
> +    if (receivedQueries.contains(queryString)) {
> +      return;
> +    }
> The comment says don't confuse the server by storing preloads, but I think
> the result of this is that we will be checking preloads without checking
> actual requests.  Christoph had a work around for this in mind, where we
> check both preloads and the actual request, and only override the preloaded
> result in the receivedQueries string if it is not "ok"?

Hey Tanvi, I extended the comment so it's not that confusing. In fast if any (the preload or the actual load) is not https, then we would store that in the results array. We only ignore the result if we have seen it already in which case this indicates that the preload as well as the actual load use the same scheme, in which case this should be fine. Agreed?
 
> +  // handle img redirect (https->http)
> +  if (queryString == "img-redir") {
> +    var newLocation =
> +     
> "http://example.com/tests/dom/security/test/csp/file_upgrade_insecure_server.
> sjs?img-redir";
> +    response.setStatusLine("1.1", 302, "Found");
> +    response.setHeader("Location", newLocation, false);
> +    return;
> +  }
> Given the code block above checks for queryString=="img-redir" and then
> returns, I'm not sure this code ever gets called.

I restructured the code, so this request appears at the very top and we have all the error handling further up in handleRequest. It should be easier to read and to follow now. So what you pointed out is actually correct, because the img-redir was only inspected before the redirected. That was a copy/paste error. Thanks for catching. Now it should all be fine.
Attachment #8623350 - Attachment is obsolete: true
Attachment #8623350 - Flags: review?(tanvi)
Attachment #8623350 - Flags: review?(mozbugs)
Attachment #8627753 - Flags: review?(tanvi)
Attachment #8627753 - Flags: review?(mozbugs)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #104)
> Created attachment 8627753 [details] [diff] [review]
> bug_1139297_upgrade_insecure_test_2_simple_upgrades.patch
> 
> (In reply to Tanvi Vyas [:tanvi] from comment #102)
> > +    // images, scripts, etc. get queried twice, do not
> > +    // confuse the server by storing preloads
> > +    if (receivedQueries.contains(queryString)) {
> > +      return;
> > +    }
> > The comment says don't confuse the server by storing preloads, but I think
> > the result of this is that we will be checking preloads without checking
> > actual requests.  Christoph had a work around for this in mind, where we
> > check both preloads and the actual request, and only override the preloaded
> > result in the receivedQueries string if it is not "ok"?
> 
> Hey Tanvi, I extended the comment so it's not that confusing. In fast if any
> (the preload or the actual load) is not https, then we would store that in
> the results array. We only ignore the result if we have seen it already in
> which case this indicates that the preload as well as the actual load use
> the same scheme, in which case this should be fine. Agreed?
>  

So if we got a preload that succeeded and the actual request failed (or vice versa), the queryString would have "iframe-ok, ... , iframe-error".  If both succeeded the queryString would have just one "iframe-ok" and if both failed the queryString would have just one "iframe-error"?  Is that right?
(In reply to Tanvi Vyas [:tanvi] from comment #105)
> So if we got a preload that succeeded and the actual request failed (or vice
> versa), the queryString would have "iframe-ok, ... , iframe-error".  If both
> succeeded the queryString would have just one "iframe-ok" and if both failed
> the queryString would have just one "iframe-error"?  Is that right?

That is exactly right, the test only passes if both, the preload and the actual request use an https scheme. In the case of an image for example, if the preload is https, then we would store 'img-ok' in the array. If then the real load is not https, then we would store 'img-error' in addition in the array. The test would fail in the end because it wouldn't expect 'img-error' but got it.
Comment on attachment 8627753 [details] [diff] [review]
bug_1139297_upgrade_insecure_test_2_simple_upgrades.patch

Thanks for all the work in updating this patch Christoph!
Attachment #8627753 - Flags: review?(tanvi) → review+
Comment on attachment 8627753 [details] [diff] [review]
bug_1139297_upgrade_insecure_test_2_simple_upgrades.patch

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

Looks good!
Attachment #8627753 - Flags: review?(mozbugs) → review+
Hey Baku,

when you implemented websockets in workers (Bug 504553) [1] you changed mURI from being an nsIURI to a nsCString [2].
Within the review of this patch you suggested to use nsIURI for mURI again (see Comment 52 bottom). I just ran mochitests and realized I get the following crash when running dom/workers/test/test_websocket.html:
> Hit MOZ_CRASH(nsStandardURL not thread-safe) at /home/ckerschb/moz/mc/netwerk/base/nsStandardURL.cpp:955

I assume that's why you changed mURI from being an nsURI to nsCString in the first place, right? I changed the code back to using nsCstring for mURI. I agree, replacing a String is not great, but I think it's the best option we have at the moment to perform the upgrade for websockets.

Do you agree?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=504553
[2] http://hg.mozilla.org/mozilla-central/diff/f23cea600bd0/content/base/src/WebSocket.cpp#l1.210
Attachment #8621353 - Attachment is obsolete: true
Attachment #8630074 - Flags: review?(amarchesini)
Comment on attachment 8630074 [details] [diff] [review]
bug_1139297_upgrade_insecure_websocket.patch

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

::: dom/base/WebSocket.cpp
@@ +1467,5 @@
> +    nsresult rv = NS_NewURI(getter_AddRefs(uri), mURI);
> +
> +    // We crash here because we are sure that mURI is a valid URI, so either we
> +    // are OOM'ing or something else bad is happening.
> +    if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +1496,5 @@
> +    return;
> +  }
> +
> +  // Potentially the page uses the CSP directive 'upgrade-insecure-requests'. In
> +  // such a case we have to upgrade ws: to wss: and also update mSecure to reflect

is this comment 80chars per line?
Attachment #8630074 - Flags: review?(amarchesini) → review+
Looking at the spec, http://www.w3.org/TR/upgrade-insecure-requests/#examples example 2 shows that navigations also get upgraded from http to https when the navigation is same origin.  cross origin TOP level navigations don't require the upgrade.  This patch doesn't implement a cross origin check on navigation.

in nsHttpChannel::Connect(), we have to check if the load is for TYPE_DOCUMENT.  If it is we, we need to compare the loadingPrincipal and the channel's principal to see if they match.  If they don't, don't require the upgrade.

Christoph, please add a separate patch for this quick change.
Hey Steve, apparently upgrade-insecure-requests does not apply to cross origin navigations; since you reviewed the initial patch, can you also review those bits where we special case that upgrade insecure does not apply to cross origin navigations? Thanks!

Please note, this patch applies on top of bug_1139297_upgrade_insecure_netwerk.patch within that bug.
Attachment #8630209 - Flags: review?(sworkman)
Comment on attachment 8630209 [details] [diff] [review]
bug_1139297_upgrade_insecure_netwerk_cross_origin_navigation.patch

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

LGTM - Thanks Chris!
Attachment #8630209 - Flags: review?(sworkman) → review+
Target Milestone: --- → mozilla42
Is there a flag/option I need to set right now in order to get this to work as expected.

It doesn't seem to working off-the-shelf for me in the latest Nightly. When I navigate to, e.g., https://www.w3.org/TR/wai-aria/states_and_properties (a page that has mixed content), I still get a mixed-content warning indicator in Nightly, but if I navigate to the same page in Chrome, I get no mixed-content warning indicator in Chrome.
(In reply to Michael[tm] Smith from comment #116)
> seem to working off-the-shelf for me in the latest Nightly. When
> I navigate to, e.g., https://www.w3.org/TR/wai-aria/states_and_properties (a
> page that has mixed content), I still get a mixed-content warning indicator

I found that the problem seems to be intermittent; reloading the document sometimes causes the mixed-content warning indicator to go away and be replaced by the full green lock as expected. But then reloading it again sometimes causes the browser to go back to showing the grey-lock-with-caution indicator.

I raised bug #1183563
Depends on: 1183563
Depends on: 1185072
Whiteboard: [adv-main42-]
Depends on: 1218773
Depends on: 1243586
Depends on: 1251043
Depends on: 1271173
Depends on: 1273430
Hi,

I am facing a issue with CORS and Upgrade-insecure-request headers. I am trying to provide cors support to my project. Also, i have set minimum restriction of "x-frame-options:same-origin" so that any client/customer who tries to frame my app without making cors request will fail to do so. In order to test it i have a test page that makes cors request and loads my app into an iframe. Now when i try to navigate my app, by clicking on a tab, the new page fails to load and can see that the browser throws "same-origin" policy error. Analyzing the network traffic, shows that, when the tab was clicked first a http:// request was made with upgrade-insecure as one of the headers. then the response has location header with "http://" only(not https because my app does not process this header yet). When the browser redirects to the page in the location header, then the CORS headers ("origin") is not present in the request and browser then sees the same-origin policy and hence the iframe fails to load the page.

I came across this issue, which mentions that the presence of upgrade-insecure header will stop the CORS. But I have not enabled this header any where in my app either through web-server or using meta-data. Then how is the browser adding this header? I have been looking at disabling this header but am not successful till now. I have been following https://www.w3.org/TR/upgrade-insecure-requests/#upgrade-request this page in order to understand in what scenarios is this header getting added.
sec 3:Upgrading Insecure Resource Requests "Environment settings objects and browsing contexts are given an insecure requests policy which has two potential values Do Not Upgrade and Upgrade. It is set to Do Not Upgrade unless otherwise specified.". It is mentioned that the default policy is "Do Not Upgrade". But i am not sure how this header is present then? 
In short could you please help me in disabling this header. 
There is also another issue that will explain in detail the issue i am facing:https://bugzilla.mozilla.org/show_bug.cgi?id=1302695
Depends on: 1391011
Depends on: 1391600
You need to log in before you can comment on or make changes to this bug.