secure sites appear with broken lock icon, resource:// incorrectly treated as insecure content

VERIFIED FIXED in mozilla1.9.1b1

Status

Core Graveyard
Security: UI
--
major
VERIFIED FIXED
10 years ago
a year ago

People

(Reporter: whimboo, Assigned: kaie)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla1.9.1b1
regression
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/20080816032044 Minefield/3.1a2pre ID:20080816032044

When you visit a website which has an EV cert and you restart Firefox with session restore enabled the EV button will only be shown split a second. It gets reverted back to the gray button. This issue is only present on Windows. I'm not able to reproduce it on OS X.

Steps to reproduce:
1. Start Firefox and enable Session Restore
2. Open https://www.paypal.com/ => Green EV button is shown
3. Restart Firefox and wait until the above site is restored => Gray button
4. Reload the page => Green EV button is visible again

During step 3 you can see that the EV button is shown split a second and is immediately replaced by the gray button. Clicking on the gray button tells that no identity information is available. That's not true because after the reload you can see the identity information. Sometimes this also happens when opening such an URL the first time.

I'll try to find the regression range for that issue.
Flags: blocking-firefox3.1?
(Reporter)

Comment 1

10 years ago
This doesn't happen on 1.9.0. Regression window is from 20080713 - today. Will have a closer look now.
(Reporter)

Comment 2

10 years ago
It fails between the following builds:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.2pre) Gecko/2008081601 GranParadiso/3.0.2pre ID:2008081601

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/20080816032044 Minefield/3.1a2pre ID:20080816032044

Checkins:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1218848940&maxdate=1218873059

I'm a bit confused. There is only one check-in (bug 423176) in this time frame which doesn't really seem to be raised this regression. CC'ing Marco who fixed bug 423176.
(Reporter)

Comment 3

10 years ago
Sorry, I have to query hg instead of using Bonsai.
(Reporter)

Comment 4

10 years ago
Same with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080817025217 Minefield/3.1a2pre ID:20080817025217
OS: Windows Vista → All
Hardware: PC → All
(Reporter)

Comment 5

10 years ago
hg checkins in this timeframe:

http://hg.mozilla.org/mozilla-central/index.cgi/pushloghtml?startdate=2008-08-16+00%3A00%3A00&enddate=2008-08-16+05%3A00%3A00

The only bug which handles some security and image code is bug 135007. So cc'ing Kai and Honza.
(Assignee)

Comment 6

10 years ago
confirming bug.

For me, it's not necessary to use session restore.

I start the browser, manually go to https://www.paypal.com, and see the reported problem. On reload, I get EV.
(Assignee)

Comment 7

10 years ago
I think it's indeed very likely that this is caused by bug 135007.

When the EV disappears, note that the status bar icon reports "broken lock icon" aka "mixed content".
I remember I noticed this problem (with broken icon) during fixing of that bug. However it seemed to me not happen from one repository update and I tough it were fixed then.

Kai, I am free to take this bug, but not sooner then on Wednesday next week.
(Reporter)

Comment 9

10 years ago
Thanks Kai. You are correct => Updating the summary and adding bug 135007 to blocker list.
Blocks: 135007
Summary: EV identity button not sticky after restart with session restore enabled → EV identity button not sticky everytime when opening secure websites
(Reporter)

Updated

10 years ago
Assignee: nobody → honzab
(Assignee)

Comment 10

10 years ago
This seems to be related to URIs like resource://gre/res/arrow.gif

When I ignore state change events for those URIs, this bug is fixed.
But we must solve two details:

- right know I don't know of an efficient way to test whether a channel is a resource:// URI. The channel neither gives me an URI nor an OriginalURI. In my testing I use nsXPIDLCString reqname; aRequest->GetName(reqname); StringBeginsWith(reqname, NS_LITERAL_CSTRING("resource://")); - But that seems too expensive to me.

- answer the question: is it always correct to ignore resource:// URIs for the security state of the document? In other words: Are such URIs always strictly local application data?
(Assignee)

Comment 11

10 years ago
updating summary, because this is not limited to EV, it's for all https (where loading of the page triggers access to a resource:// image)
Blocks: 130949
Summary: EV identity button not sticky everytime when opening secure websites → https, broken lock icon, resource://images should be ignored
(Reporter)

Comment 12

10 years ago
After talking with Kai on IRC we should make the summary a bit clearer.
Summary: https, broken lock icon, resource://images should be ignored → EV sites (https) appear with broken lock icon, resource:// incorrectly treated as insecure content
again, not limited to EV
Assignee: honzab → kaie
Component: Location Bar and Autocomplete → Security: UI
Flags: blocking-firefox3.1?
Product: Firefox → Core
QA Contact: location.bar → ui
Summary: EV sites (https) appear with broken lock icon, resource:// incorrectly treated as insecure content → secure sites appear with broken lock icon, resource:// incorrectly treated as insecure content

Comment 14

10 years ago
I agree.  Unless it's some different issue, I see the mixed content indications problem displaying this very bug.

Updated

10 years ago
Flags: blocking1.9.1?

Updated

10 years ago
Assignee: kaie → honzab
(Assignee)

Comment 15

10 years ago
should we back it out?

or check in the inefficient workaround?
(Assignee)

Comment 16

10 years ago
Boris, Christian, can we assume that resource:// are always local trusted content, and thus can be ignored for tracking the secure state of a web page?

I've been unable so far to find an efficient way to query a nsIRequest/nsIChannel to "resource://". See also comment 10 (no URI, no originalURI, can not use schemesis).
Any nsIChannel will always have a URI and an originalURI, unless its implementation is _very_ busted.  If it's that busted, please treat it as untrusted no matter what.

Imagelib nsIRequests are not nsIChannels, but they're imgIRequests, and provide the URI from that interface.

resource: is basically equivalent to file: as far as the security manager is concerned.  Once you have the nsIURI, you can test its chain for the URI_IS_LOCAL_FILE protocol bit.  That will also handle jar:resource: for you, etc.

Does that help?
(Assignee)

Comment 18

10 years ago
Boris, that really helped very much!

I'm using the following snippet to make a decision about resource:// and it works great to fix this bug.

      // ignore resource:// image URIs
      nsCOMPtr<nsIURI> uri;
      imgRequest->GetURI(getter_AddRefs(uri));
      if (uri) {
        PRBool res;
        if (NS_SUCCEEDED(uri->SchemeIs("resource", &res)) && res) {
          isSubDocumentRelevant = PR_FALSE;
        }
      }

This snippet patch overlaps / depends on the one in bug 135007.
I used the ->schemeis approach, because that seemed easier.

Are you ok with this patch?

It might be less efficient than your proposal to test for the URI_IS_LOCAL_FILE bit.
When you said "chain", I suppose I would have to use NS_URIChainHasFlags. That uses do_GetService each time, so I would have to cache the I/O service in each nsSecureBrowserUI object.
If you think that would be better, I can adjust it.
I think caching an IOService in nsSecureBrowserUI and using the flag is the way to go.  It'll be a little slower than the SchemeIs check, but a lot more correct and extensible.
(Assignee)

Comment 20

10 years ago
Boris, either I'm doing something wrong, or your proposal does not work.
The following snippet never gives me a "has flag" result.

      imgRequest->GetURI(getter_AddRefs(uri));
      if (uri) {
        if (!mIOService) {
          mIOService = nsGetServiceByContractID(NS_IOSERVICE_CONTRACTID);
        }
  
        if (mIOService) {
          PRBool hasFlag;
          nsresult rv = 
            mIOService->URIChainHasFlags(uri, 
                                         nsIProtocolHandler::URI_IS_LOCAL_FILE, 
                                         &hasFlag);
          if (NS_SUCCEEDED(rv) && hasFlag) {
            isSubDocumentRelevant = PR_FALSE;
          }
        }

As the proposal from comment 18 works, should we do that?
You probably want to check for UI_RESOURCE too, not just LOCAL_FILE, no?  That will cover chrome: and resource:.... (I thought resource: was LOCAL_FILE already, but clearly I was wrong; there are some XXX comments about this.)
(Assignee)

Comment 22

10 years ago
Created attachment 335641 [details] [diff] [review]
Patch v1

We already have code that filters out nsIFileChannel and declares those as not relevant for sub content.

So I guess it's sufficient to test for UI_RESOURCE, only, in order to fix this bug. With that change my testing shows it's fixed.


This (unreal) patch contains some overlapping code, from bug 135007.
The real merged patch is available over there, attachment 335640 [details] [diff] [review].
Attachment #335641 - Flags: review?(bzbarsky)
(Assignee)

Comment 23

10 years ago
Basically the new portions in the attached patch are equivalent to comment 20, LOCAL_FILE => UI_RESOURCE, plus the code to cache the IOService object.
Comment on attachment 335641 [details] [diff] [review]
Patch v1

>+++ b/security/manager/boot/src/nsSecureBrowserUIImpl.cpp
>+  nsCOMPtr<nsINetUtil> aIOService;

Name this ioService?

>+    aIOService = nsGetServiceByContractID(NS_IOSERVICE_CONTRACTID);

Is there a reason not to use do_GetService here?

>+      // ignore resource:// image URIs

Why just image URIs?  Why not everything that's resource:// ?
(Assignee)

Comment 25

10 years ago
(In reply to comment #24)
> >+  nsCOMPtr<nsINetUtil> aIOService;
> 
> Name this ioService?

renamed...


> >+    aIOService = nsGetServiceByContractID(NS_IOSERVICE_CONTRACTID);
> 
> Is there a reason not to use do_GetService here?

I had copied code from net-util. Changed to your preference...


> >+      // ignore resource:// image URIs
> 
> Why just image URIs?  Why not everything that's resource:// ?

I'm ok with that change from the functional point of view, but what about performance?

Will we see a performance regression when we start calling ioService->URIChainHasFlags on every OnStateChange notification?
(Assignee)

Comment 26

10 years ago
Created attachment 335693 [details] [diff] [review]
unreal patch v2

Updated patch to include Boris proposed changes.

Again, contains some overlapping code, from bug 135007.
The real merged patch is available over there, attachment 335692 [details] [diff] [review]
Attachment #335641 - Attachment is obsolete: true
Attachment #335693 - Flags: review?(bzbarsky)
Attachment #335641 - Flags: review?(bzbarsky)
> but what about performance?

That's a good question...  I suspect that if we could replace the entire QI chain with a single URIChainHasFlags call that would be a performance win or at least not a loss.  Otherwise, hard to say.  But really, this should be a pretty fast call, so I doubt it would be noticeable.
In that last patch, why are we checking for an image request only when !httpRequest?
And seriously, I think that the code as it works right now (with interface-checking) on the channels is seriously broken: it will miss some types of insecure content, especially where extensions are involved.  Please get a followup bug filed to use self-reported protocol flags or something instead of this QI approach?
(Assignee)

Comment 30

10 years ago
(In reply to comment #29)
> And seriously, I think that the code as it works right now (with
> interface-checking) on the channels is seriously broken: it will miss some
> types of insecure content, especially where extensions are involved.  Please
> get a followup bug filed to use self-reported protocol flags or something
> instead of this QI approach?

The current implementation is a hack.

Rather than trying to put one band aid on top of the other, we must finally get bug 62178 done. I don't want to invent a new solution to detect the type of load that "already happened" when we really want to switch to an implementation that has the ability to actually block loading of insecure content.
OK.  I can live with that.  What about comment 28?
(Assignee)

Comment 32

10 years ago
Created attachment 335737 [details] [diff] [review]
unreal patch v3

(In reply to comment #28)
> In that last patch, why are we checking for an image request only when
> !httpRequest?

No special reason. I simply didn't change the old series of actions. I'm attaching a new patch that does the test for image first.
Assignee: honzab → kaie
Attachment #335693 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #335737 - Flags: review?(bzbarsky)
Attachment #335693 - Flags: review?(bzbarsky)
Attachment #335737 - Attachment is patch: true
Attachment #335737 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 335737 [details] [diff] [review]
unreal patch v3

r=bzbarsky
Attachment #335737 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 34

10 years ago
pushed to hg with revision f95e6897b536, marking fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080902041619 Minefield/3.1b1pre and the equivalent Windows XP build. I verified using the steps in Comment 0.
Status: RESOLVED → VERIFIED
(Reporter)

Comment 36

10 years ago
Could this be added to test-suite? Looks more reasonable as having a Litmus testcase for this issue.
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.1b1

Updated

9 years ago
Flags: blocking1.9.1?

Comment 37

9 years ago
I dont think this bug is fixed. I use NoScript and the latest nighly trunk version of Firefox and I am getting pages saying they are not secure and when looking at the Media tab under Page Info I see the following:

resource://noscript_0.6457643962686347/icon32.png

The same issue I am having with another extension which uses resource icons and that too is causing the page to show up as insecure.
Could you give an exact example of a secure page you see as broken, please?

Comment 39

9 years ago
https://virtualoffice.lss.ku.edu/NetStorage/ you will need to use the following NoScript settings to simulate my setup and see if the error can be reproduced:

ser_pref("capability.policy.maonoscript.javascript.enabled", "allAccess");
user_pref("capability.policy.maonoscript.sites", "about: about:certerror about:config about:neterror about:plugins about:privatebrowsing about:sessionrestore chrome: https://virtualoffice.lss.ku.edu resource:");
user_pref("noscript.allowURLBarJS", false);
user_pref("noscript.autoReload", false);
user_pref("noscript.autoReload.allTabs", false);
user_pref("noscript.autoReload.global", false);
user_pref("noscript.blockCssScanners", true);
user_pref("noscript.blockNSWB", true);
user_pref("noscript.clearClick.exceptions", "");
user_pref("noscript.confirmUnblock", false);
user_pref("noscript.consoleDump", 1);
user_pref("noscript.consoleLog", true);
user_pref("noscript.contentBlocker", true);
user_pref("noscript.ctxMenu", false);
user_pref("noscript.default", "chrome: resource: about:");
user_pref("noscript.docShellJSBlocking", 2);
user_pref("noscript.filterXExceptions", "^http://([a-z]+)\\.google\\.(?:[a-z]{1,3}\\.)?[a-z]+/(?:search|custom|\\1)\\?\n^http://[a-z]+\\.wikipedia\\.org/wiki/[^\"<>\\?%]+$");
user_pref("noscript.firstRunRedirection", false);
user_pref("noscript.forbidBookmarklets", true);
user_pref("noscript.forbidChromeScripts", true);
user_pref("noscript.forbidFrames", true);
user_pref("noscript.forbidIFrames", true);
user_pref("noscript.forbidIFramesContext", 0);
user_pref("noscript.forbidImpliesUntrust", true);
user_pref("noscript.forbidJarDocumentsExceptions", "");
user_pref("noscript.forbidMetaRefresh", true);
user_pref("noscript.forbidXBL", 5);
user_pref("noscript.gtemp", "");
user_pref("noscript.httpsForced", "virtualoffice.lss.ku.edu");
user_pref("noscript.httpsForcedExceptions", "");
user_pref("noscript.ignorePorts", false);
user_pref("noscript.injectionCheck", 3);
user_pref("noscript.intranetMaskRx", "^(1(27|0|92)\\.[\\d.]+)");
user_pref("noscript.lockPrivilegedUI", true);
user_pref("noscript.notify", false);
user_pref("noscript.notify.bottom", false);
user_pref("noscript.notify.hidePermanent", false);
user_pref("noscript.nselForce", false);
user_pref("noscript.nselNever", true);
user_pref("noscript.opacizeObject", 3);
user_pref("noscript.options.tabSelectedIndexes", "1,0,1");
user_pref("noscript.policynames", "");
user_pref("noscript.secureCookies", true);
user_pref("noscript.secureCookiesForced", ".virtualoffice.lss.ku.edu");
user_pref("noscript.showAllowPage", false);
user_pref("noscript.showBlockedObjects", false);
user_pref("noscript.showDistrust", false);
user_pref("noscript.showDomain", true);
user_pref("noscript.showGlobal", false);
user_pref("noscript.showPermanent", false);
user_pref("noscript.showTempToPerm", false);
user_pref("noscript.showUntrusted", false);
user_pref("noscript.showUntrustedPlaceholder", false);
user_pref("noscript.temp", "");
user_pref("noscript.toolbarToggle", 0);
user_pref("noscript.untrusted", "");
user_pref("noscript.version", "1.9.0.8");

Comment 40

9 years ago
Sorry, forgot to tell you to just cancel out of any login prompt and then NoScript should have the frames blocked from displaying click each one so they are activated, then refresh page. The error should occur then.

Comment 41

9 years ago
Has anyone been able to reproduce the issue using the above settings and url?
This is not caused by resource: but by data: as i can seen so far. It's unrelated to this bug nor to bug 477118.

I filed a new bug 482245 about this issue.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.