135007 - Transfer mode of images should be relevant for shown lock icon state (mixed content)

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Kai Engert [:KaiE:]]

Currently, when the security code decides whether the lock icon should be shown
as secure, mixed, or insecure, the transfer mode of images gets ignored. Whether
displayed images have been loaded over https or http, this does not influence
the state of the lock icon.

Comment 1

[Kai Engert [:KaiE:]]

*** Bug 181072 has been marked as a duplicate of this bug. ***

Comment 2

[John Unruh]

*** Bug 178591 has been marked as a duplicate of this bug. ***

Comment 3

[Kai Engert [:KaiE:]]

*** Bug 169289 has been marked as a duplicate of this bug. ***

Comment 4

[Daniel Veditz [:dveditz]]

*** Bug 234663 has been marked as a duplicate of this bug. ***

Comment 5

[Daniel Veditz [:dveditz]]

*** Bug 219502 has been marked as a duplicate of this bug. ***

Comment 6

[Daniel Veditz [:dveditz]]

*** Bug 301275 has been marked as a duplicate of this bug. ***

Comment 7

[Chris Beard]

is this being worked on?  if not, who should be working on it?  (or should this
be deferred?)

/cb

Comment 8

[Chris Beard]

should be looked at.  not going to make 1.8b4.  deferring.

/cb

Comment 9

[Darin Fisher]

Does this bug apply to all image loads or only ones that are "background" loaded?  i.e., loads that use nsIRequest::BACKGROUND_LOAD so that they do not block "onload".  Such loads do not generate nsIWebProgressListener events, which would explain why the secure browser UI would not get to observe them.  For non-background image loads, it may be the case that the secure browser UI does not know how to deal with a nsIRequest that is a imgIRequest (and not a nsIChannel).

So, two things probably: (1) WPL is not a sufficient observer API, and (2) imgIRequest does not expose nsIChannel::securityInfo.

Comment 10

[Kai Engert [:KaiE:]]

This bug applies at least to images loaded using <img src=""> tags. I haven't yet looked into the background loading you mention.

You are right wrt imgIRequest.

I talked to Stuart today. I learned that the nsIRequest objects sent with the listener events are imgRequestProxy objects that do not implement nsIChannel. The associated channel must be obtained indirectly.

However, Stuart does not want us to allow obtaining the channel from the proxy, but we agreed on a new interface that allows to obtain the associated security info, only.

I'm attaching a very experimental initial patch, that I played with today. There are a couple of problems with the patch.

It seems, we don't seem to get "data is tranfering" events on image requests. So maybe we should always assume that data has been transfered. Not sure if that's the right thing to do.

It seems we have to filter out resource:// image requests, I hope that's fine because those are non-network requests.

The img requests tries to release its pointer to the associated channel as soon as possible. For initial testing I changed that to keep it. Maybe we should store a pointer to the security info at the time the channel is released.

While the patch detects the mixed mode situation, the mixed mode does not get set when using the back/forward buttons.

I'm primarily attaching the patch to make sure the code won't get lost until I have time to work on it more.

Comment 11

[Kai Engert [:KaiE:]]

Attachment: experimental code

Comment 12

[Mike Shaver (:shaver emeritus)]

blocking: integrity of the lock icon is important.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do Web pages depend on this?  Is this bug common in other browsers?  If so, should we consider fixing this only for a new higher-security lock icon rather than the current one?

Comment 14

[Kai Engert [:KaiE:]]

(In reply to comment #13)
> Is this bug common in other browsers?  

No. Other browser like IE and Netscape 4.x do it right.
Insecure content is either shown or hidden.
Fixing this should be done together with bug 62178, so that loading of insecure content (images) can actually be blocked.


> If so,
> should we consider fixing this only for a new higher-security lock icon rather
> than the current one?

IMHO Mozilla's usual lock icon should be fixed.

IMHO by default, on a secure page, we should not display any content loaded over insecure URLs, notify in the chrome that insecure content is not being shown, and allow to use that chrome to explicitly display it.

Comment 15

[Chandrasekar Kannan]

Kai, Should I re-assign this bug to you ?.

Comment 16

[Kai Engert [:KaiE:]]

Shaver asked this bug should block 1.8.1.

Dan, you had assigned this bug to yourself about a year ago.
Do you intend to work on it in time?

Some ideas can be found in the attached patch and in comment 10.

Comment 17

[Darin Fisher]

Not going to block FF2 for this.  We need a well tested trunk patch first.  If a low-risk patch appears (seems unlikely), then we'll consider it for FF2 beta2.

Comment 18

[Doug Turner (:dougt)]

*** Bug 364482 has been marked as a duplicate of this bug. ***

Comment 19

[earthsound]

Kai: have you (or anyone else) played with the year-old experimental code attached to this bug?

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: Fix 1

This patch is fully inspired by Kaie's patch. It stores securityInfo of the underlying data channel to each image proxy request and makes it accessible with new interface nsISecurityInfoProvider. There are following changes in security UI implementation:
- accepts imgIImageRequest as being subrequest relevant
- stores such request in mTransferringRequests hashtable; image requests do not report TRANSFERING state and it is not a simple (even if possible) to allow them to report it and security UI works only with requests that transferred any data

This works also for 302 status image redirections. 
Back-forward issue has already been fixed in another bug.

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 319541 [details] [diff] [review]
Fix 1

This patch doesn't work for case #3: request found in cache. there is no channel associated and just OnStopRequest is directly called to notify the image load completeness.

Comment 22

[Honza Bambas (:mayhemer)]

Attachment: Fix 2

This caches the security info of the channel within cached imgRequest.

Comment 23

[Stuart Parmenter]

this seems reasonable, although if an image is shared between documents, i assume the lock icon will be wrong?

Comment 24

[Kai Engert [:KaiE:]]

I get a build error with patch v2:

/home/kaie/moz/head/mozilla/modules/libpr0n/src/imgRequestProxy.cpp: In member function 'virtual nsresult imgRequestProxy::GetSecurityInfo(nsISupports**)':
/home/kaie/moz/head/mozilla/modules/libpr0n/src/imgRequestProxy.cpp:390: error: 'class nsDerivedSafe<imgRequest>' has no member named 'mSecurityInfo'

Comment 25

[Honza Bambas (:mayhemer)]

Attachment: Fix 2.1

Added missing header file

Comment 26

[Kai Engert [:KaiE:]]

Thanks, this patch compiles for me, and it passes my little old test.

I guess it would be good to set up a test for Stuart's comment 23.

Comment 27

[Kai Engert [:KaiE:]]

Stuart, this test page opens (after 1 second) two new windows, which both load the same 3.7 MB image.

https://kuix.de/misc/insecure-image/multi.php

Comment 28

[Kai Engert [:KaiE:]]

Comment on attachment 319802 [details] [diff] [review]
Fix 2.1

>-          isSubDocumentRelevant = PR_FALSE;
>+          nsCOMPtr<imgIRequest> imgRequest(do_QueryInterface(aRequest));
>+          if (!imgRequest) {
>+            PR_LOG(gSecureDocLog, PR_LOG_DEBUG,
>+                   ("SecureUI:%p: OnStateChange: not relevant for sub content\n", this));
>+            isSubDocumentRelevant = PR_FALSE;
>+          } else {
>+            // For image request emulate data transfer

Please say in the comment that we are not getting a transfer notification for images,
and possibly refer to an open bug number.

Can you please change this code, so we only do this once?
I'm worried we do this (unnecessarily) multiple times, once for each kind of notification we get.

You told me, the stop notification is the only reliable notification we get?

If so, can you please set a boolean here (isImage) and move the PL_DHASH_ADD further down, to the place where we already process the stop request for sub?



>+            nsAutoMonitor lock(mMonitor);
>+            PL_DHashTableOperate(&mTransferringRequests, aRequest, PL_DHASH_ADD);
>+          }
>         }
>       }
>     }
>   }
>

Comment 29

[Honza Bambas (:mayhemer)]

Attachment: Fix 3

Changed according to Kai's comment.

Comment 30

[Kai Engert [:KaiE:]]

Comment on attachment 319858 [details] [diff] [review]
Fix 3

Look ok to me, thanks! r=kengert for the changes in mozilla/security/manager

Before you ask for superreview, you might want to get these portions fixed with up-to-date information:

>+++ netwerk/base/public/nsISecurityInfoProvider.idl	2008-05-07 19:25:44.248009400 +0200
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *

Comment 31

[Honza Bambas (:mayhemer)]

Attachment: Final version

Just modified the comment in IDL.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 321736 [details] [diff] [review]
Final version

>+++ modules/libpr0n/src/imgRequestProxy.cpp

>+NS_IMPL_ISUPPORTS4(imgRequestProxy, imgIRequest, nsIRequest, nsISecurityInfoProvider,
>                    nsISupportsPriority)

Was there a reason not to put the new interface at the end?

>+NS_IMETHODIMP imgRequestProxy::GetSecurityInfo(nsISupports** _retval)
>+  *_retval = nsnull;
>+  if (mOwner)
>+    NS_IF_ADDREF(*_retval = mOwner->mSecurityInfo);

Why do the first assignment if mOwner?

Also, I would prefer a getter for the security info on the imgRequest over just grabbing its members.  Please check with stuart to see which he prefers.

>+++ modules/libpr0n/src/imgRequestProxy.h	>+class imgRequestProxy : 
>+    public imgIRequest, 
>+    public nsISupportsPriority,
>+    public nsISecurityInfoProvider

I'd leave those hanging off to the right of the ':' unless there's a good reason to reformat this way...

>+++ netwerk/base/public/nsISecurityInfoProvider.idl

>+ * The Initial Developer of the Original Code is mozilla.org

I would think the initial developer is you.

>+++ security/manager/boot/src/nsSecureBrowserUIImpl.cpp

>+// static
>+nsresult 
>+nsSecureBrowserUIImpl::ExtractSecurityInfo(nsIRequest* aRequest, nsISupports** _retval)

This should have the signature:

  already_AddRefed<nsISupports> ExtractSecurityInfo(nsIRequest* aRequest)

since the nsresult is in fact completely pointless and ignored by all callers.

>+            // Remeber this is an image request. Because image loads doesn't

"remember".

Please add a note in bug 432685 explaining what code here needs to be removed once that bug is fixed?

>@@ -944,27 +974,32 @@ nsSecureBrowserUIImpl::OnStateChange(nsI
>+    if (isImageRequest) 
>+      requestHasTransferedData = PR_TRUE;
>+    else
>     {

I'd prefer curlies around the if body in this case, since the else has them.

>@@ -1131,21 +1166,21 @@ nsSecureBrowserUIImpl::OnStateChange(nsI
>-    
>+

Please don't make random whitespace changes?

>+++ security/manager/boot/src/nsSecureBrowserUIImpl.h

>+  static nsresult ExtractSecurityInfo(nsIRequest* aRequest, nsISupports** _retval);

Is there a need for this to be a static class method instead of just a static in the file?

Comment 33

[Honza Bambas (:mayhemer)]

Attachment: Final version, 2

I fixed all bz's comments except the last one - I believe the method should be in class because there are other helper method already.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 322109 [details] [diff] [review]
Final version, 2

s/_retval/retval/ and looks good.

Comment 36

[Kai Engert [:KaiE:]]

I'm very happy this work is now done, thanks a lot to Honza for working on the patch, and to Boris and Stuart for their reviews.

I have pushed the patch to mozilla-central as changeset cf3eae029913.

Marking fixed.

Comment 37

[Dave Townsend [:mossop]]

I've backed this out since it appears to have caused a Tp regression on OSX http://graphs.mozilla.org/#show=911655&sel=1218767385,1219005881

Comment 38

[Kai Engert [:KaiE:]]

Dave, what's next? Did you verify/confirm that this patch was responsible for the tp increase?

(Furthermore, the increase you quoted was very small. There seems to have been a much higher increase in the recent past, that got accepted?)

Comment 39

[Dave Townsend [:mossop]]

(In reply to comment #38)
> Dave, what's next? Did you verify/confirm that this patch was responsible for
> the tp increase?

Yes, we are pretty confident that it was the cause of the Tp regression, and while small it still needs to verified that the fix this patch brings is worth the regression (by module owner/driver types).

> (Furthermore, the increase you quoted was very small. There seems to have been
> a much higher increase in the recent past, that got accepted?)

That hasn't been accepted, it is still under investigation, bug 450401

Comment 40

[Boris Zbarsky [:bzbarsky]]

Do we have any indication which pages actually got slower?  Or was it across the board?

Comment 41

[Kai Engert [:KaiE:]]

I set up https://wiki.mozilla.org/StandaloneTalos on a Mac Mini with OSX 10.4, thanks a lot to Alice for getting me started.

I downloaded the nightly Firefox builds 
from Aug 17 (including the patch) and
from Aug 19 (patch backed out).

After running standalone talos with both, I did some manual preparation in order to compare the numbers.

rm all.csv; for i in tsvg.csv tsspider.csv tp.csv tjss.csv tgfx.csv tdhtml.csv ; do cat $i >> all.csv; done
cat all.csv |awk -F, '{print $2 " " $4}' > all2.csv

I will attach both versions of all2.csv
I used tkdiff to visually compare the numbers from both runs.

My manual inspection shows the following pages as the ones with the biggest differences:

 replaceimages 20%
       layers5 15%
          zoom 10%
   gearflowers 10%
    imageslide  9%
        layers1 5%
  composite-*  0-7%

Comment 42

[Kai Engert [:KaiE:]]

The numbers are based on the "mean values".

Comment 43

[Kai Engert [:KaiE:]]

Attachment: without 135007, 2008-08-19, talos standalone, all2.csv

Comment 44

[Kai Engert [:KaiE:]]

Attachment: with 135007, 2008-08-17, talos standalone, all2.csv

Comment 45

[Boris Zbarsky [:bzbarsky]]

Interesting.  Some of those pages load a whole slew of images...  Want to try running replaceimages and seeing whether you hit your new code?

Comment 46

[Kai Engert [:KaiE:]]

No solution yet, this comment only has some description and thoughts around the old and the new code.

IIUC, the Tp tests do not involve any SSL.
Yesterday I made the statement:

"The only new non-SSL action I can see in the patch: Each time we run imgRequest::OnStartRequest we call channel->GetSecurity, which is limited to some addref."

Now I realize this was wrong.
PSM's state tracking tracks the state of all elements of a page, even if the top page is plain http.
That means, all new code of the patch gets executed for non-SSL pages, too.

Each time in OnStateChange, each time in OnLocationChange, we attempt to extract the security info from the channel.

Without patch, this is limited to nsIRequest->QI->nsIChannel->GetSecurityInfo
In addition with the patch, if that operation fails, we attempt nsIRequest->QI->nsIChannel->GetSecurityInfo

With the patch, we use QI to detect whether a request is an image request.

Both old/new code track the requests that have transferred data in a hashtable.

There is a change in our processing of STOP progress events.
Without the patch, we always do the hashtable lookup, and if found, remove.

However, with the patch, if we know it's an image request, we don't try to remove from the hashtable.

But the new code simply assumes we don't see TRANSFERRING notifications. It does not enforce consistency. Should we get such notifications, we'll add the request to the hashtable, but never remove it.
This inconsistency should get fixed in a new revision of the patch.
I'll test whether we already run into it.

Comment 47

[Kai Engert [:KaiE:]]

Testing replaceimages.html

Without patch, we call OnSecurityChange 6 times.
With patch, we call it 3759 times.
That should explain the performance regression.

We should avoid calling OnSecurityChange when the state doesn't change.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Yes, please!  Every OnSecurityChange call hits the UI, running JS and so forth, right?

Comment 49

[Kai Engert [:KaiE:]]

I have a potential fix in bug 451420.
I probably should do a local test with both patches and repeat the local talos test.

Comment 50

[Honza Bambas (:mayhemer)]

Attachment: Final version, 3

Addressing comment 46: "Should we get such notifications, we'll add the
request to the hashtable, but never remove it."

Also rearranged QI to search for type of the request - imgRequst is tested sooner because it is much often present while common browsing then ftp or other types of requests.

Comment 51

[Kai Engert [:KaiE:]]

Comment on attachment 334745 [details] [diff] [review]
Final version, 3

Your idea to reorder is probably helpful. 

Your patch does not yet address BZ's review request from comment 34. I previously did before checking in.
We should attach a patch that contains that change.

r=kaie with that change

Comment 52

[Honza Bambas (:mayhemer)]

Attachment: Final version, 3.0.1

Includes that change.

Comment 53

[Kai Engert [:KaiE:]]

(In reply to comment #49)
> I have a potential fix in bug 451420.
> I probably should do a local test with both patches and repeat the local talos
> test.

According to my tests the patch fixes the performance issue.

Comment 54

[Kai Engert [:KaiE:]]

Attachment: patch 301 merged with patch from 450912

As the code from this patch overlaps with the proposed code for bug 450912, I'm attaching this merged version.

I'll ask for review for the new (overlapping) subset in that bug.

Comment 55

[Kai Engert [:KaiE:]]

Attachment: patch 301 merged with patch v2 from 450912

Comment 56

[Kai Engert [:KaiE:]]

Attachment: patch 301 merged with patch v3 from 450912

Comment 57

[Kai Engert [:KaiE:]]

Re-landed, combined with bug 450912 (the functional regression) and bug 451420 (avoid performance regression).

With all 3 patches combined, my local performance testing still gave me a slight performance regression on some pages, namely on those that were affected most badly.

I haven't yet tried to calculate the overall performance change locally. Let's have a look at the chart (see comment 37) later today or tomorrow.

Comment 58

[Kai Engert [:KaiE:]]

pushed to hg with revision 2dc9da1db912, marking fixed, second attempt

Comment 59

[Benjamin Smedberg]

Fixed before 1.9.1 branched

Comment 60

[Gordon P. Hemsley [:GPHemsley]]

Verified on a recent Shiretoko build.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b3pre) Gecko/20090220 Shiretoko/3.1b3pre