If a secure (SSL) page includes a script from a non-SSL URI the security state correctly shows the "mixed" state. But once a secure page is loaded if it later dynamically adds a script node loaded from a non-SSL URI the secure state is not degraded to "mixed". By contrast, adding a non-SSL iframe node does cause the icon to change to "mixed".

We should also check for dynamic insecure CSS, and adding secure CSS that includes non-SSL bindings etc.

And XMLHttpRequest or SOAP request to an insecure URL should flip it to mixed. We currently restrict XMLHttpRequest to same-origin so we don't need to do anything on that right now. SOAP can break out of the same-origin restriction if the 3rd party site allows it, and a similar mechanism is proposed for XMLHttpRequest in the future.

A fix seems unlikely for the 1.8.0 branch, it would involve some pretty deep changes in the security UI listener design that would be more appropriate for a final release with betas etc. to shake out problems.

Keeping this bug hidden doesn't protect users, so I'm making it public.

Not going to block FF2 for this bug, but we'd happily consider a safe patch.

Dan, the testcase no longer works for me (though I confirm the script it uses still exists.) Is this still an issue?

Updated testcase, the mozilla.com script used by the old one had been moved.

This is still an issue in the latest nightly, please reconsider the blocking decision. Added a different testcase that better shows why it's important -- I've loaded insecure content from ha.ckers.org that can steal the bugzilla cookies, and it's still showing a "secure" lock icon. Ideally we would fix bug 62178 and bug 321022 instead which would make this case not even come up (assuming we don't prompt). If we do the prompt (or have a pref bypass) though we'd then still have to fix this case.

sg:low, no patch, won't block.

So the first problem is that docloader eats all notifications for loads which happen after onload. This patch should fix that. I still don't see the right behavior with it, though.

We learned that we never retrieve OnStateChange with TRANSFERRING for the plain text JS. Boris patch fixes that. In fact, with his patch applied, I can see that we do switch to "mixed state", however, only for a fraction of a second. Because immediately afterwards, we receive a new notification that indicates a new top level load is starting. It is a wyciwyg load, which is the modified document (produced by document.write). As soon as we receive that new toplevel START, we reset our state tracking (and forget all previous insecure loads). We find the wyciwyg channel has a security info associated with a secure state and decide to switch a good lock icon. So, after having a chat with Boris, he recommends we should remember the broken state, and carry it around in the security info associated to the original document. This is a good idea. And actually something like this has already been done recently! Honza had worked on it, we already have nsIAssociatedContentSecurity. So that's good news. I was able to produce a very small patch that will attempt to restore our state, if the new request is a wyciwyg channel. That helps, but it's still not sufficient. I'll attach a patch, and write another comment...

So, with both patches applied, I load the test page, I click the button, and the lock icon immediately switches to broken. That's good. Unfortunately, if I click "back", the page reverts to "secure", and if I click "forward", the page remains at "secure" :-/

We load the first page, we go secure. Click the button, we load the script, we remember one insecure item and go insecure. we see the new toplevel load. before we reset, we save the state (one insecure item) to the security info. we reset the state. we load the state. it's still the same security info. we succesfully retrieve the remembered insecure item. We remain at insecure. click the back button. we save the sate (still having one insecure item). we reset the state. now, the new load (the original https address) is neither a bfcache load, nor a wyciwyg load. therefore, we don't attempt to restore. we go secure. click the forward button. as always, we save our state (secure, no insecure sub items). we reset the state. we discover it's now wyciwyg. we load, but the associated info now says: secure

and another problem: - visit the test page - click the button (we switch to the broken icon) - cancel that dialog - hit ESC to stop the load - quit, with session restore enabled - start again - we incorrectly show the page as secure

I changed the code to always attempt to retrieve the remembered insecure sub items. When going backward to the original https, we still switch back to secure. This probably makes sense. But it means, the original page has its own security info, separate from the one that stores info for the modified page. When going forward (to the modified page), we get a security info with "zero insecure". This surprises me. When going forward, I would have expected to get the same security info object that we had had when we originally left the modified page. So, maybe, when going forward to the modified page, we no longer have the old security info object from the modified page, but get a fresh copy from the good page?

-> me

work in progress. this is additional patch (dependent on the two previous) that solves the problem. Someone (me :)) forget to add the associated content info to nss socket info serialization. Problem is that Wyciwyg channel loads on forward from cache and also restores the security info object from the cache. I use the fact the nss socket info is always load from separate metadata field, i.e. there is no data behind the nss data. I append the four numbers (number of hi, low, broken and no security subloads) at the end of the metadata.

I think this bug is dependent on tests for mixed content. There could be scenario we don't see now for which these patches could still be inefficient.

Comment on attachment 349436 [details] [diff] [review] patch 3 Honza, does this patch fix the bug? >+ >+ PRUint32 avail; >+ nsresult rv = stream->Available(&avail); >+ if (NS_SUCCEEDED(rv) && avail) { >+ stream->Read32((PRUint32*)&mSubRequestsHighSecurity); >+ stream->Read32((PRUint32*)&mSubRequestsLowSecurity); >+ stream->Read32((PRUint32*)&mSubRequestsBrokenSecurity); >+ stream->Read32((PRUint32*)&mSubRequestsNoSecurity); >+ } I guess, you query for "available" bytes, because the file on disk could contain an old object version, that did not contain those additional bytes. Is my guess correct? Does the stream contain multiple objects? If the stream contains multiple objects, how do you know the "available bytes" are really what you expect? Could the next bytes belong to the next object in the stream? I guess we must use some form of object versioning. Is there anythink we can use to distinguish old streams from new streams?

This is why all consumers of nsBinaryInput/OutputStream should really be versioning their data files (and dropping the deserialization on the floor on version mismatch, since it might cause security issues to deserialize an incorrect array length, say). Does that not happen here?

(In reply to comment #22) (In reply to comment #24) I count on fact the object is always in a separate file. I believe it is wrong, with your objections even more. True solution compatible in all ways would be to have the version in the file name or stream identifier. If we create a record in old FF version it must not be accessible to newer and vice-versa.

Comment on attachment 349436 [details] [diff] [review] patch 3 I don't see any kind of versioning in the ::Write and ::Read functions. Maybe there is something as part of the WriteCompoundObject, but there are variables written directly, in addition. Also I conclude the existing stream carries multiple object. r- I think in order to fix this bug, versioning must be introduced (and a mechanism to identify old streams without versioning/object identifiers).

Added versioning, confirmed by the tests.

Comment on attachment 365331 [details] [diff] [review] patch 3, v2 [Checkin comment 37] Thanks Honza! r=kaie Unless Boris wants a different approach... this seems like a reasonable approach to introduce versioning and handle the old non-versioned streams.

That looks good to me. Very nice solution!

Comment on attachment 362601 [details] [diff] [review] Version of "necessary but sufficient" patch that actually builds This was checked in as part of bug 480713.

Ops, sorry, there is still a patch to land.

Comment on attachment 365331 [details] [diff] [review] patch 3, v2 [Checkin comment 37] This patch was forgotten to land on both m-c and 1.9.1.

(In reply to comment #32) > (From update of attachment 365331 [details] [diff] [review]) > This patch was forgotten to land on both m-c and 1.9.1. Are you going to land it on m-c then?

Very soon.

(In reply to comment #32) > (From update of attachment 365331 [details] [diff] [review]) > This patch was forgotten to land on both m-c and 1.9.1. Does that mean this bug isn't fixed1.9.1?

(In reply to comment #35) > Does that mean this bug isn't fixed1.9.1? Exactly. This confusion comes from more then one patch attached to this bug. One of them was landed as part of another bug.

Comment on attachment 365331 [details] [diff] [review] patch 3, v2 [Checkin comment 37] http://hg.mozilla.org/mozilla-central/rev/4851a9d9a60c

Comment on attachment 365331 [details] [diff] [review] patch 3, v2 [Checkin comment 37] Does this patch still apply cleanly to 1.9.2?

(In reply to comment #38) > Does this patch still apply cleanly to 1.9.2? s/1.9.2/1.9.1.2

(In reply to comment #39) > (In reply to comment #38) > > Does this patch still apply cleanly to 1.9.1.2? Yes it does and builds. I just have to confirm it fixes the problem (there is no automated test).

Comment on attachment 365331 [details] [diff] [review] patch 3, v2 [Checkin comment 37] Not for 1.9.1.2.

Honza: Have you confirmed that your patch fixes the problem on 1.9.1?

Comment on attachment 335617 [details] [diff] [review] Patch v2 - more code, still not sufficient [NEVER LANDED ON M-C] This bug has never been by (my) mistake fixed as per the description on either the trunk or branch. To explain: There are 3 patches for this bug: 1. Attachment 335617 [details] [diff] "Patch v2 - more code, still not sufficient". This patch has never landed anywhere and is necessary to fix this bug! 2. Attachment 335595 [details] [diff] "Necessary but not sufficient". This patch has been landed as part of bug 480713 comment 24. 3. Attachment 365331 [details] [diff] "Patch 3". It is a regression fix caused by landing the second patch. There were a test for this bug, created and landed BEFORE this bug got fixed with TODO check for the issue in the description. That test has been disabled because I locally figured out it was causing leaks. The test was never run on the product machines. Now I figure out the test is wrong, there have to be regular check, which is failing. After I apply the first patch, the test works as expected and also the manual test is working as expected. Only after that this bug is really fixed. I'm giving this patch r+ and I am about to attach two patches of all missing changes in the code and tests for the trunk and for the branch.

Kai, please check the test changes, fairly simple.

Same as for m-c, excluding the tests. Checked by the test ported to 1.9.1 and by the manual test.

Comment on attachment 393752 [details] [diff] [review] Missing changes for mozilla-central [Checkin comment 48] [Checkin comment 1.9.2 comment 52] r=kaie

Comment on attachment 393752 [details] [diff] [review] Missing changes for mozilla-central [Checkin comment 48] [Checkin comment 1.9.2 comment 52] Adding my r+ for nsSecureBrowserUIImpl changes.

Comment on attachment 393752 [details] [diff] [review] Missing changes for mozilla-central [Checkin comment 48] [Checkin comment 1.9.2 comment 52] http://hg.mozilla.org/mozilla-central/rev/76c02c19390d

Comment on attachment 393752 [details] [diff] [review] Missing changes for mozilla-central [Checkin comment 48] [Checkin comment 1.9.2 comment 52] This is the one we want for 1.9.2, we don't need to reland the bits landed in comment 37

Comment on attachment 393754 [details] [diff] [review] Missing changes for 1.9.1 branch [Checkin 1.9.1 comment 51] Approved for 1.9.1.4, a=dveditz for release-drivers

Comment on attachment 393754 [details] [diff] [review] Missing changes for 1.9.1 branch [Checkin 1.9.1 comment 51] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c96e5e667a3b

Comment on attachment 393752 [details] [diff] [review] Missing changes for mozilla-central [Checkin comment 48] [Checkin comment 1.9.2 comment 52] http://hg.mozilla.org/releases/mozilla-1.9.2/rev/10595ea48572

Verified for 1.9.1 using DVeditz's testcase and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.4pre) Gecko/20090930 Shiretoko/3.5.4pre. Mixed icon now appears.