1082837 - Add ImageLib infrastructure to allow correct mixed content blocking

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Tanvi Vyas[:tanvi]]

While working on bug 418354, I found that cached images that involved redirects don't go through AsyncOnChannelRedirect.

Here is an example:
https://people.mozilla.org/~tvyas/mixeddisplayredir.html

The page includes an image sourced from https://people.mozilla.org/~tvyas/greenplus.jpg.  The server returns a 302 to http://people.mozilla.org/~tvyas/redminus.jpg.  The redirect results in a call to AsyncOnChannelRedirect().  If you open a new tab and visit https://people.mozilla.org/~tvyas/mixeddisplayredir.html again, the image will be served from the cache with a location of https://people.mozilla.org/~tvyas/greenplus.jpg and will not go through AsyncOnChannelRedirect.

This causes problems with Mixed Content Blocker detection.  If AsyncOnChannelRedirect is not called, Mixed Content Blocker cannot detect that the image came from an insecure location and hence sets the wrong security state on the page.

Comment 1

[Tanvi Vyas[:tanvi]]

This bug blocks 418354 and bug 947079.  
* We might be able to fix 418354 without this bug, because some redirect detection in Mixed Content Blocker is better than none.  I can disable the test that is failing because of this bug and re-enable it later.
* I'm less confident in landing bug 947079 without a fix to this issue.  Bug 947079 is about giving the user false positive mixed content warnings.  If we fix that bug without fixing this one, we will instead be giving the user false negatives (where the page appears secure when it is actually not).

Comment 2

[Tanvi Vyas[:tanvi]]

Seth, any update on this bug?  Do you want to assign it to yourself?

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: Track insecure redirects on imgRequest

I will assign this to myself, though I know nothing about mixed content blocking or security generally, so I'm going to confine myself to providing infrastructure in this bug.

Per our previous email discussion, it appears to be sufficient that we track for each imgRequest:

(1) Whether the redirect chain contained any non-HTTPS URLs.
(2) What the final URI is.

This patch adds code that implements this. For (1), we record any insecure redirects in imgCacheValidator and imgRequest. You can check whether an imgRequest had an insecure redirect via imgRequest::HadInsecureRedirect(). (Easy!)

We deliberately do not consider the final URI (imgRequest::mCurrentURI), because as I understand it, it needs a more complicated test: we can simply check for "https" everywhere else, but for the final URI other schemes like "about" are also considered secure. Please let me know if my understanding is wrong.

We already supported (2) - just call imgRequest::GetCurrentURI() to get the final URI for that imgRequest.

Tanvi, does this meet your requirements? Do you see any obvious issues?

Comment 4

[Tanvi Vyas[:tanvi]]

Comment on attachment 8536912 [details] [diff] [review]
Track insecure redirects on imgRequest

Thank you Seth for this patch!  I'm going to add it to my patch queue and see how it works with Mixed Content Blocker, so leaving the feedback flag for now.

Can you expand on this section:
-    request->Init(originalURI, uri, channel, channel, entry,
-                  aCX, nullptr, imgIRequest::CORS_NONE, RP_Default);
+    // XXX(seth): Since we're handed a preexisting channel, we have no way of
+    // knowing if there was an insecure redirect somewhere in the redirect
+    // chain. Since LoadImageWithChannel is only used for image documents, this
+    // should not be a serious issue.
+    request->Init(originalURI, uri, /* aHadInsecureRedirect = */ false,
+                  channel, channel, entry, aCX, nullptr,
+                  imgIRequest::CORS_NONE, RP_Default);

By image documents do you mean images that are top level loads?

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Tanvi Vyas [:tanvi] from comment #4)
> By image documents do you mean images that are top level loads?

Yeah. When you navigate to "http://foo.com/image.jpg" in the address bar, what you get is an image document. The same goes for loading "http://foo.com/image.jpg" in an iframe.

Comment 6

[Tanvi Vyas[:tanvi]]

After talking to Christoph and thinking more about this, the issue also exists for CSP.  So instead of just calling MixedContentBlocker on the final URI, I'm going to call all content policies on it.

Note that for a 3 stage redirect on a cached image, only the first hop and the last hop will go through our content policies.
https://a.com -> http://b.com -> https://c.com

Mixed Content Blocker will still block the load because it will check the HadInsecureRedirect flag, which would be set to true because of the http hop.

But if CSP has a policy image-src 'a.com c.com' then we will allow the image even though it went through the b.com redirect.  If CSP had a policy image-src 'https:', then checking the HadInsecureRedirect flag would make sense.

I don't believe we have the redirect history (https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIRedirectHistory.idl) here because that was stored on the previous channel that cached the content and is long gone by the time we try and retrieve the cached content.  So I think the only thing we can do for CSP is live with this edge case security hole until the imgLoader rewrite fixes these problems.  We should file a separate bug on that.

Comment 7

[Tanvi Vyas[:tanvi]]

Attachment: Bug1082837-mixed-01-22-15.patch

Work in progress patch that calls content policies on the final URI (mCurrentURI) and checks the mHadInsecureRedirect Flag.

I'm having a few problems getting this to work because I don't think the code I've written is in the right part of imgLoader.cpp.  Will followup in a separate comment.

Comment 8

[Tanvi Vyas[:tanvi]]

Comment on attachment 8536912 [details] [diff] [review]
Track insecure redirects on imgRequest

>Bug 1082837 - Track insecure redirects on imgRequest. r=?
>
>diff --git a/image/src/imgLoader.cpp b/image/src/imgLoader.cpp
>@@ -2271,18 +2272,23 @@ nsresult imgLoader::LoadImageWithChannel
>     // inherited on the channel.
>     NewRequestAndEntry(true, this, getter_AddRefs(request), getter_AddRefs(entry));
> 
>     // We use originalURI here to fulfil the imgIRequest contract on GetURI.
>     nsCOMPtr<nsIURI> originalURI;
>     channel->GetOriginalURI(getter_AddRefs(originalURI));
> 
>     // No principal specified here, because we're not passed one.
>-    request->Init(originalURI, uri, channel, channel, entry,
>-                  aCX, nullptr, imgIRequest::CORS_NONE, RP_Default);
>+    // XXX(seth): Since we're handed a preexisting channel, we have no way of
>+    // knowing if there was an insecure redirect somewhere in the redirect
>+    // chain. Since LoadImageWithChannel is only used for image documents, this
>+    // should not be a serious issue.
If LoadImageWithChannel is only used for top level documents, that's fine.  If it can be used inside iframes, then we may be loading a url that is blocked by CSP or is insecure into the top level page.  Should we at least call content policies on the final URI?  I haven't looked at this part of the code closely.  (Note that it says that there are many comments here talking about not having a principal.  But you do have one now... channel->loadInfo->loadingPrincipal().)

>+    request->Init(originalURI, uri, /* aHadInsecureRedirect = */ false,
>+                  channel, channel, entry, aCX, nullptr,
>+                  imgIRequest::CORS_NONE, RP_Default);
> 
>@@ -2512,17 +2518,18 @@ NS_IMPL_ISUPPORTS(imgCacheValidator, nsI
>                   nsIAsyncVerifyRedirectCallback)
> 
> imgCacheValidator::imgCacheValidator(nsProgressNotificationProxy* progress,
>                                      imgLoader* loader, imgRequest *request,
>                                      void *aContext, bool forcePrincipalCheckForCacheEntry)
What is the imgCacheValidator and what does it try and validate?

>diff --git a/image/src/imgRequest.cpp b/image/src/imgRequest.cpp
>--- a/image/src/imgRequest.cpp
>+++ b/image/src/imgRequest.cpp
>@@ -116,16 +118,31 @@ nsresult imgRequest::Init(nsIURI *aURI,
>   mRequest = aRequest;
>   mChannel = aChannel;
>   mTimedChannel = do_QueryInterface(mChannel);
> 
>   mLoadingPrincipal = aLoadingPrincipal;
>   mCORSMode = aCORSMode;
>   mReferrerPolicy = aReferrerPolicy;
> 
>+  // If the original URI and the current URI are different, check whether the
>+  // original URI is secure. We deliberately don't take the current URI into
>+  // account, as it needs to be handled using more complicated rules than
>+  // earlier elements of the redirect chain.
>+  if (aURI != aCurrentURI) {
>+    bool isHttps = false;
>+    if (NS_FAILED(aURI->SchemeIs("https", &isHttps)) || !isHttps) {
>+      mHadInsecureRedirect = true;
>+    }
>+  }
>+
>+  // imgCacheValidator may have handled redirects before we were created, so we
>+  // allow the caller to let us know if any redirects were insecure.
>+  mHadInsecureRedirect = mHadInsecureRedirect || aHadInsecureRedirect;
>+

Is aURI the original URI and aCurrentURI the current redirect?  So we are checking if the very first uri (the one the webpage sourced) is insecure?  Or is aURI one of the redirect hops?  This section confuses me.  How would we have handled redirects before we created the image request?

Comment 9

[Tanvi Vyas[:tanvi]]

Hi Seth,

Ideally, all the uri we go through to get from the original uri to the final uri should go through a content policy check.  Right now, only the original uri goes through one.  I've written code to make the final uri also go through a content policy check.  For the hops in between, there is nothing we can do unless we change the way the imagelib cache works or use the necko cache for images.  In the meantime, I can at least check for mixed content hops using the mHadInsecureRedirect flag you added in your patch.

I'm having trouble determining where I should be adding the content policy and mixed content checks.  I'm looking for the place in the code where an image is going to load from cache.  I started off with the code in imgCacheValidator::OnStartRequest (as you can see in the patch).  But that doesn't seem to work.  Moving it to imgLoader::LoadImage() http://mxr.mozilla.org/mozilla-central/source/image/src/imgLoader.cpp#2220, the content policy check works, but the flag for mHadInsecureRedirect isn't set when it should be.  Also, I'm not sure that LoadImage() catches all places where we go through the cache.  Where should I be calling ShouldLoadCachedImage()?

Thanks!

Comment 10

[Tanvi Vyas[:tanvi]]

Attachment: Tracking insecure redirects on imgRequest v2

Comment 11

[Tanvi Vyas[:tanvi]]

Attachment: Bug1082837-mixed-02-19-15.patch

Comment 12

[Tanvi Vyas[:tanvi]]

Hi Seth,

I’ve updated your patch slightly to check if the scheme is chrome when determining whether a cached image went through an insecure redirect hop.  While testing, I noticed a number of chrome images where the flag was getting set and then later check when they were retrieved from the cache:
chrome://browser/skin/searchbar-dropmarker.png
chrome://browser/skin/urlbar-history-dropmarker.png
etc.

I’ve also added a second patch that does the extra content policy and mixed content checks.

A couple of questions
1) Does ValidateSecurityInfo only get called for cached images?  I don’t want to add the burden of extra calls to content policy when we don’t need them.  Also, should we add a check for aURI != mCurrentURI or something to check if the image went through a redirect?

2) How do we handle image documents that are in an iframe (also see comment 8)?  Assume. https://foo.com loads https://foo.com/image1.png in an iframe.  https://foo.com/image1.png redirects to http://bar.com/image2.png.  The first time we load https://foo.com Mixed Content Blocker will detect the mixed content image.  The second time when the image is already in the cache, it sounds like it won’t report it because .  We have to deal with this case.  At best, we can somehow populate mHadInsecureRedirect for image document loads and at worse we just check the final hop against content policies.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8566328 [details] [diff] [review]
Bug1082837-mixed-02-19-15.patch

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

Looks good with the tweaks below.

::: dom/security/nsMixedContentBlocker.cpp
@@ +49,5 @@
>  bool nsMixedContentBlocker::sBlockMixedDisplay = false;
>  
> +// For cached image requests, did the image request go through an insecure
> +// image redirect?
> +bool nsMixedContentBlocker::mHadInsecureImageRedirect = false;

It doesn't seem like mHadInsecureImageRedirect should be static (as I note below), so it probably shouldn't get initialized here.

@@ +156,5 @@
>  }
>  
> +nsMixedContentBlocker::nsMixedContentBlocker(bool aHadInsecureImageRedirect)
> +{
> +  mHadInsecureImageRedirect = aHadInsecureImageRedirect;

Nit: I'd suggest initializing mHadInsecureImageRedirect in the initializer list, but it's up to you.

@@ +455,5 @@
> +  // TYPE_IMAGE redirects are cached based on the original URI, not the final
> +  // destination and hence cache hits for images may not have the correct
> +  // aContentLocation.  Check if the cached hit went through an http redirect,
> +  // and if it did, we can't treat this as a secure subresource.
> +  if (!mHadInsecureImageRedirect && (schemeLocal || schemeNoReturnData || schemeInherits || schemeSecure)) {

Nit: Looks like this line has more than 80 characters.

::: dom/security/nsMixedContentBlocker.h
@@ +39,4 @@
>    NS_DECL_NSICHANNELEVENTSINK
>  
>    nsMixedContentBlocker();
> +  nsMixedContentBlocker(bool aHadInsecureImageRedirect);

This constructor needs to be marked explicit, or else the static analysis test will reject this code when you try to land it.

@@ +43,3 @@
>    static bool sBlockMixedScript;
>    static bool sBlockMixedDisplay;
> +  static bool mHadInsecureImageRedirect;

This shouldn't be static, right? It doesn't seem like we want this to be global. (And if we did, it'd be extremely misleading to initialize it in the constructor for this class.)

::: image/src/imgLoader.cpp
@@ +592,5 @@
>  }
>  
> +/* Call content policies on cached images that went through a redirect */
> +static nsresult
> +ShouldLoadCachedImage(imgRequest* aImgRequest, nsISupports* aLoadingContext, nsIPrincipal* aLoadingPrincipal, int16_t* aDecision)

Nit: Looks like this might be over 80 characters.

@@ +625,5 @@
> +                           contentLocation,
> +                           requestingLocation,
> +                           aLoadingContext,
> +                           EmptyCString(), //mime guess
> +                           nullptr,     

Nit: There's whitespace at the end of the line here.

@@ +649,5 @@
> +  if (NS_FAILED(rv) || !NS_CP_ACCEPTED(*aDecision)) {
> +    return rv;
> +  }
> +
> +  return rv;

Nit: We may as well just return rv unconditionally here, right?

@@ +693,5 @@
> +  nsresult rv = ShouldLoadCachedImage(request, aCX, loadingPrincipal, &decision);
> +
> +  if (NS_FAILED(rv) || !NS_CP_ACCEPTED(decision)) {
> +      return false;
> +  }

If the only caller of ShouldLoadCachedImage just returns a boolean, I'd suggest making ShouldLoadCachedImage return a boolean as well.

Comment 14

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8566326 [details] [diff] [review]
Tracking insecure redirects on imgRequest v2

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

LGTM.

::: image/src/imgLoader.cpp
@@ +2279,5 @@
>      // No principal specified here, because we're not passed one.
> +    // XXX(seth): Since we're handed a preexisting channel, we have no way of
> +    // knowing if there was an insecure redirect somewhere in the redirect
> +    // chain. Since LoadImageWithChannel is only used for image documents, this
> +    // should not be a serious issue.

I wonder if we should update this comment to include the thing we talked about in person today: we only look for the final URI in the ImageLib cache in the LoadImageWithChannel case, which means that we don't need to worry about insecure redirects in this case at all. (Is that correct?)

::: image/src/imgRequest.cpp
@@ +1102,5 @@
> +  // If the previous URI is a non-HTTPS URI, record that fact for later use by
> +  // security code, which needs to know whether there is an insecure load at any
> +  // point in the redirect chain.
> +  bool isHttps = false;
> +  if (NS_FAILED(mCurrentURI->SchemeIs("https", &isHttps)) || !isHttps) {

Should we check for chrome:// here as well?

Comment 15

[Seth Fowler [:seth] [:s2h]]

(In reply to Tanvi Vyas [:tanvi] from comment #12)
> I’ve updated your patch slightly to check if the scheme is chrome when
> determining whether a cached image went through an insecure redirect hop. 
> While testing, I noticed a number of chrome images where the flag was
> getting set and then later check when they were retrieved from the cache:
> chrome://browser/skin/searchbar-dropmarker.png
> chrome://browser/skin/urlbar-history-dropmarker.png
> etc.

Sounds good! I wonder if that means we should treat resource:// as secure as well?

> 1) Does ValidateSecurityInfo only get called for cached images?  I don’t
> want to add the burden of extra calls to content policy when we don’t need
> them.

Yeah, only for cached images.

> Also, should we add a check for aURI != mCurrentURI or something to
> check if the image went through a redirect?

Not sure. Where were you thinking of adding it?

> 2) How do we handle image documents that are in an iframe (also see comment
> 8)?  Assume. https://foo.com loads https://foo.com/image1.png in an iframe. 
> https://foo.com/image1.png redirects to http://bar.com/image2.png.  The
> first time we load https://foo.com Mixed Content Blocker will detect the
> mixed content image.  The second time when the image is already in the
> cache, it sounds like it won’t report it because .  We have to deal with
> this case.  At best, we can somehow populate mHadInsecureRedirect for image
> document loads and at worse we just check the final hop against content
> policies.

We talked about this in person and concluded that it didn't matter because for image documents, we are only looking up the final URI in the image cache, after all redirects have already happened. Is that correct?

(You mentioned you were going to test to verify that that's true, which I think is a good idea.)

Comment 16

[Tanvi Vyas[:tanvi]]

Attachment: Tracking insecure redirects in imagelib

Hi Seth,
Thank you for your feedback and comments.  I've updated the patches and am uploading them now.

Who should review this imagelib patch?  You wrote most of the code, and I've updated it a bit.  Please flag someone for review.

What's left:
* tests
* confirm that image documents don't have this cache redirect problem.

Comment 17

[Tanvi Vyas[:tanvi]]

Attachment: Call content policies on imagelib cache hits.patch

Hi Olli,

This patch is to correct a problem in imagelib where redirects are not cached and hence don't go through content policy checks.  We have to add a call to content policies and a special call just to mixed content blocker with information about whether the initial load went through an insecure redirect.  Please take a look.  Thanks!

Comment 18

[Tanvi Vyas[:tanvi]]

Image documents are usually top level loads, in which case we aren't worried about mixed content loads.

Image documents could, however, be in iframes.  If you iframe src an https image that redirects to an http image on an HTTPS page, the iframe will be blocked as mixed active content.  I believe this happens before the request ever makes it to imagelib.  Seth, can you confirm.  Here is an example - https://people.mozilla.org/~tvyas/image_frame.html

You can compare it to the http case where the load will be allowed.  In that case, please confirm the final url (and not the initial url) goes through LoadImageWithChannel - http://people.mozilla.org/~tvyas/image_frame.html.

(In the test case, https://people.mozilla.org/~tvyas/greenplus.jpg redirects to http://people.mozilla.org/~tvyas/redminus.jpg)

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Why do we want the special case for nsMixedContentBlocker in ShouldLoadCachedImage?
Why not all the content policies?
And feels odd to create nsMixedContentBlocker temporarily.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Oh, I see... hmm

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8576969 [details] [diff] [review]
Call content policies on imagelib cache hits.patch

Would it make sense to pass imgRequest as the "extra" param to ShouldLoad
and then always call NS_CheckContentLoadPolicy? nsMixedContentBlocker implementation could then access imgRequest's HadInsecureRedirect.

(I can't recall now in which cases "extra" is normally used. Would need to go through the callers.)


It is anyway odd that if insecureRedirect is true, we return early and
don't call NS_CheckContentLoadPolicy.

Comment 22

[Tanvi Vyas[:tanvi]]

(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8576969 [details] [diff] [review]
> Call content policies on imagelib cache hits.patch
> 
> Would it make sense to pass imgRequest as the "extra" param to ShouldLoad
> and then always call NS_CheckContentLoadPolicy? nsMixedContentBlocker
> implementation could then access imgRequest's HadInsecureRedirect.
> 
> (I can't recall now in which cases "extra" is normally used. Would need to
> go through the callers.)
> 
Extra is a parameter reserved for addons.  If we use it, we might break addons that are expecting that parameter to be something different.  I talked to Dan Veditz about it, because that would be better than creating another mixed content blocker constructor, but that is not an option.


> It is anyway odd that if insecureRedirect is true, we return early and
> don't call NS_CheckContentLoadPolicy.
You are right!  Just because mixed content blocker detects mixed display content, doesn't mean we shouldn't check what the other content policies say about the load!  I'll update the code to change that.

Comment 23

[Tanvi Vyas[:tanvi]]

Attachment: Call content policies on imagelib cache hits

Updated per Olli's comment 21.  Also calling Content Policies first now, since CSP should get checked before mixed content blocker if possible.

Comment 24

[Tanvi Vyas[:tanvi]]

Comment on attachment 8576966 [details] [diff] [review]
Tracking insecure redirects in imagelib

Since Seth wrote most of the code in this patch, flagging jdm for imagelib review.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8578931 [details] [diff] [review]
Call content policies on imagelib cache hits


> nsMixedContentBlocker::nsMixedContentBlocker()
> {
>+  mHadInsecureImageRedirect = false;
>+  // Cache the pref for mixed script blocking
>+  Preferences::AddBoolVarCache(&sBlockMixedScript,
>+                               "security.mixed_content.block_active_content");
>+
>+  // Cache the pref for mixed display blocking
>+  Preferences::AddBoolVarCache(&sBlockMixedDisplay,
>+                               "security.mixed_content.block_display_content");
>+}
So you end up creating many nsMixedContentBlocer instances, and each of them adds bool var cache.
I assume you actually get AssertNotAlreadyCached assertion in debug builds.

>+  if (insecureRedirect) {
>+    if (!nsContentUtils::IsSystemPrincipal(aLoadingPrincipal)) {
>+      // Set the requestingLocation from the aLoadingPrincipal.
>+      nsCOMPtr<nsIURI> requestingLocation;
>+      if (aLoadingPrincipal) {
>+        rv = aLoadingPrincipal->GetURI(getter_AddRefs(requestingLocation));
>+        NS_ENSURE_SUCCESS(rv, false);
>+      }
>+
>+      // reset the decision for mixed content blocker check
>+      *aDecision = nsIContentPolicy::REJECT_REQUEST;
>+      nsRefPtr<nsMixedContentBlocker> mcb = new nsMixedContentBlocker(insecureRedirect);
This feels so wasteful. Couldn't we make the methods in nsMixedContentBlocker static and call such here?
Then the non-static methods would just forward the calls to the static ones.

Comment 26

[Tanvi Vyas[:tanvi]]

Attachment: Call content policies on imagelib cache hits

Thanks Olli for the comments!  I've updated the patch so that it now has a static version of ShouldLoad() that performs all the Mixed Content Blocker logic.  This takes an extra parameter aHadInsecureImageRedirect and we call that directly from imgLoader when we have an insecure redirect in an image cache hit.

Content Policy API and AsyncOnChannelRedirect continues to call non-static ShouldLoad().  non-static ShouldLoad() then calls static ShouldLoad, passing in false for aHadInsecureImageRedirect.

This is messier than I'd like it to be, but it avoids creating a new Mixed Content Blocker object in imgLoader.

When imageLib gets rewritten, we can clean this up again and remove the need for a static ShouldLoad.

Comment 27

[Josh Matthews [:jdm]]

Comment on attachment 8576966 [details] [diff] [review]
Tracking insecure redirects in imagelib

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

::: image/src/imgLoader.cpp
@@ +2744,5 @@
> +  if (NS_FAILED(oldChannel->GetURI(getter_AddRefs(oldURI))) ||
> +      NS_FAILED(oldURI->SchemeIs("https", &isHttps)) ||
> +      NS_FAILED(oldURI->SchemeIs("chrome", &isChrome)) ||
> +      NS_FAILED(NS_URIChainHasFlags(oldURI, nsIProtocolHandler::URI_IS_LOCAL_RESOURCE , &schemeLocal))  ||
> +      (!isHttps && !isChrome && !schemeLocal)) {

It might be nice to throw this block in a utility method somewhere.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8580122 [details] [diff] [review]
Call content policies on imagelib cache hits

>+NS_IMETHODIMP
>+nsMixedContentBlocker::ShouldLoad(bool aHadInsecureImageRedirect,
s/NS_IMETHODIMP/nsresult/

>+  static NS_IMETHODIMP ShouldLoad(bool aHadInsecureImageRedirect,
s/NS_IMETHODIMP/nsresult/

>+ShouldLoadCachedImage(imgRequest* aImgRequest, nsISupports* aLoadingContext, nsIPrincipal* aLoadingPrincipal, int16_t* aDecision)
Couldn't you drop the aDecision param. The caller doesn't use it for anything.
Just have some local variable
int16_t decision = nsIContentPolicy::REJECT_REQUEST;
and pass &decision when needed


>+  // Content Policy Check on Cached Images
>+  int16_t decision = nsIContentPolicy::REJECT_REQUEST;
>+  return ShouldLoadCachedImage(request, aCX, loadingPrincipal, &decision);
... and then here drop int16_t decision = nsIContentPolicy::REJECT_REQUEST;

Comment 29

[Tanvi Vyas[:tanvi]]

Attachment: mochitests v1

Added test cases to browser_mcb_redirect.js.
Test 3 and 4 are added for completeness sake (since we already had redirect script tests, I added redirect image tests).

Test 5 and 6 are added to check that cached images that went through a redirect are considered mixed content.

Test 7 is added to get a double redirected image into the cache.

Test 8 and 9 are added to check that cached images that go through a double redirect are considered mixed content.

Test 6 and 9 fail without this patch.  Test 6 is fixed by the new call to content policies and Test 9 is fixed by the direct call to Mixed Content Blocker.

Comment 30

[Tanvi Vyas[:tanvi]]

Attachment: Call content policies on imagelib cache hits v6

Addressed Olli's suggestions, carrying over r+.

Seth, do you still want to feedback these patches?

Comment 31

[Tanvi Vyas[:tanvi]]

(In reply to Josh Matthews [:jdm] from comment #27)
> Comment on attachment 8576966 [details] [diff] [review]
> Tracking insecure redirects in imagelib
> 
> Review of attachment 8576966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/imgLoader.cpp
> @@ +2744,5 @@
> > +  if (NS_FAILED(oldChannel->GetURI(getter_AddRefs(oldURI))) ||
> > +      NS_FAILED(oldURI->SchemeIs("https", &isHttps)) ||
> > +      NS_FAILED(oldURI->SchemeIs("chrome", &isChrome)) ||
> > +      NS_FAILED(NS_URIChainHasFlags(oldURI, nsIProtocolHandler::URI_IS_LOCAL_RESOURCE , &schemeLocal))  ||
> > +      (!isHttps && !isChrome && !schemeLocal)) {
> 
> It might be nice to throw this block in a utility method somewhere.

See bug https://bugzilla.mozilla.org/show_bug.cgi?id=899099.  For imagelib, we probably don't need to check all the protocol flags that nsMixedContentBlocker sets - http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#417.  For example, we shouldn't redirect to a javascript: url.  Or mailto?

For now, would you like to create a utility function in imageLib for this?  Once the imageLib rewrite happens, it will no longer needed (and pretty much all the code in this bug won't be needed since images will go through the necko cache instead of the imagelib cache).

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8580821 [details] [diff] [review]
mochitests v1

Mostly rs+

Comment 33

[Tanvi Vyas[:tanvi]]

Comment on attachment 8580821 [details] [diff] [review]
mochitests v1

Christoph, can you take a look at these too?

Comment 34

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8580122 [details] [diff] [review]
Call content policies on imagelib cache hits

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

Looks good! Just some minors nits below.

BTW, please update the commit message to say "f=seth" instead of "sfowler".

::: dom/security/nsMixedContentBlocker.cpp
@@ +316,5 @@
> +  return rv;
> +}
> +
> +/* Static version of ShouldLoad() that contains all the Mixed Content Blocker
> + * logic.  Called from non-static ShouldLoad().i

Nit: stray "i" at the end of this line.

::: dom/security/nsMixedContentBlocker.h
@@ +45,5 @@
> +   * Called directly for imageLib when an insecure redirect exists in a cached
> +   * image load.
> +   * @param aHadInsecureImageRedirect
> +   *        boolean flag indicating that an image load went through an http
> +   *        redirect.

Perhaps this is more obvious to the people who usually work on this code, but I'd consider rewording this to more clearly explain that what you're looking for is whether an insecure redirect through HTTP happened. Saying "an http redirect" does indicate the same thing, but someone just skimming might not pick up on it.

::: image/src/imgLoader.cpp
@@ +592,5 @@
>  }
>  
> +/* Call content policies on cached images that went through a redirect */
> +static bool
> +ShouldLoadCachedImage(imgRequest* aImgRequest, nsISupports* aLoadingContext, nsIPrincipal* aLoadingPrincipal, int16_t* aDecision)

This looks over 80 columns. Please reformat it to comply with our style guide.

Comment 35

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8580830 [details] [diff] [review]
Call content policies on imagelib cache hits v6

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

Looks like my comments above still apply.

Comment 36

[Tanvi Vyas[:tanvi]]

Attachment: Call content policies on imagelib cache hits v7

Addressed seth's comments.  Carrying over r+ from smaug and f+ from seth.

Comment 37

[Tanvi Vyas[:tanvi]]

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14c94a65e307

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8580821 [details] [diff] [review]
mochitests v1

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

Tests look good, but I don't think you need to add three *.sjs files - seems like a lot of duplicate code.
You could simplify the loading to:
src="./test_mcb_imagredirect_server.sjs?foo"

and within the *.sjs you could do:

if (request.queryString === "foo") { redirect to that file; return; }
if (request.queryString === "bla") { redirect to other file; return; }

Other than that, all the tests look good - especially the detailed description of each test - that's great!
Canceling the review for now, but will r+ once we have combined the three *.sjs into one!

Comment 39

[Tanvi Vyas[:tanvi]]

Attachment: mochitests v2

Updated tests per Christoph's suggestions and eliminated 3 sjs files.  Carrying over r+ from smaug and reflagging Chris for review.

Try looks good, so if Chris is happy we can push this to inbound!

Comment 40

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #18)
> Image documents are usually top level loads, in which case we aren't worried
> about mixed content loads.
> 
> Image documents could, however, be in iframes.  If you iframe src an https
> image that redirects to an http image on an HTTPS page, the iframe will be
> blocked as mixed active content.  I believe this happens before the request
> ever makes it to imagelib.  Seth, can you confirm.  Here is an example -
> https://people.mozilla.org/~tvyas/image_frame.html
> 
> You can compare it to the http case where the load will be allowed.  In that
> case, please confirm the final url (and not the initial url) goes through
> LoadImageWithChannel - http://people.mozilla.org/~tvyas/image_frame.html.
> 
> (In the test case, https://people.mozilla.org/~tvyas/greenplus.jpg redirects
> to http://people.mozilla.org/~tvyas/redminus.jpg)

Confirmed that image documents go through LoadImageWithChannel with the final url.

* Went to http://people.mozilla.org/~tvyas/redminus.jpg, and confirm we go through LoadImageWithChannel.

* Go to https://people.mozilla.org/~tvyas/greenplus.jpg with a clear cache.  We get a redirect and go to LoadImageWitChannel where the channel's uri is the final redminus uri (not the original green plus)

* Go to https://people.mozilla.org/~tvyas/image_frame.html and we don't hit LoadImageWithChannel because the image document is blocked as a mixed content frame.  If we disable protection, we end up at LoadImageWithChannel with the redminus.jpg url

* Go to http://people.mozilla.org/~tvyas/image_frame.html and we hit LoadImageWithChannel with the redminus url.

I think that's sufficient.  For image documents in iframes, they will be blocked as mixed active content.  If the user has disabled protection or turned off mixed active content blocking, then mixed active content has loaded on the page and the security UI will be degraded anyway.

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8582007 [details] [diff] [review]
mochitests v2

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

::: browser/base/content/test/general/browser.ini
@@ +98,5 @@
>    test_no_mcb_on_http_site_font2.html
>    test_no_mcb_on_http_site_font2.css
>    test_mcb_redirect.html
> +  test_mcb_redirect_image.html
> +  test_mcb_double_redirect_image.html

At some point we should also bundle mcb browser tests into one folder.

::: browser/base/content/test/general/test_mcb_redirect.sjs
@@ +3,2 @@
>  
> +  if (request.queryString === "script") { 

Nit: whitespace

Comment 42

[Tanvi Vyas[:tanvi]]

Attachment: mochitest v3

Thanks Christoph!  Fixed whitespace issue.  Pushing to inbound soon.

Comment 43

[Tanvi Vyas[:tanvi]]

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee8dc146ae82
https://hg.mozilla.org/integration/mozilla-inbound/rev/8da6a781c01a
https://hg.mozilla.org/integration/mozilla-inbound/rev/40a24381b8fb

Comment 44

[Tanvi Vyas[:tanvi]]

Attachment: Call content policies on imagelib cache hits v8

Using the nsresult on the non-static nsIContentPolicy::ShouldLoad and NS_IMETHODIMP for static ShouldLoad in nsMixedContentBlocker.cpp.  These should be flipped.

Comment 45

[Tanvi Vyas[:tanvi]]

Pushed fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/e0260f535d85

Comment 46

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

https://hg.mozilla.org/mozilla-central/rev/ee8dc146ae82
https://hg.mozilla.org/mozilla-central/rev/8da6a781c01a
https://hg.mozilla.org/mozilla-central/rev/40a24381b8fb
https://hg.mozilla.org/mozilla-central/rev/e0260f535d85