(Related to Bug 262689) See the testcase 2 in Bug 277554. http://beta51.minutedesign.com/test2.html Steps to Reproduce: 1. Load test2.html 2. Click the link 3. Notice that the content appears to be secure, and the certificate from https://login.yahoo.com/ is shown See the source of testcase. Content is written by function rewrite(). Succeed: window.opener.document.open(); window.opener.stop(); window.opener.document.write(spoofing); Failed: window.opener.document.open(); window.opener.document.write(spoofing); window.opener.document.close(); To insure spoofing, you have to use "stop()" instead of "document.close()".

To verify the real URI, click on location bar and push Escape key. This should be "wyciwyg://*/http://beta51.minutedesign.com/test2.html".

This sounds similar to some Johnny recently fixed (but this is still in ff1.0 and moz1.7.5): bug 253121 and bug 262689 Also similar lock icon issues, Darin fixed bug 257308 and bug 268483.

Setting as a 1.8a6 blocker. Time is very short. Who can tackle this? Johnny?

Better testcase http://beta51.minutedesign.com/test2-2.html In function rewrite(), "window.opener.document.URL" - Error: uncaught exception: Permission denied to get property HTMLDocument.URL "window.opener.document.open()" - No error. This should be denied.

(In reply to comment #4) > In function rewrite(), > "window.opener.document.URL" > - Error: uncaught exception: Permission denied to get property HTMLDocument.URL > "window.opener.document.open()" > - No error. This should be denied. Sorry, this comment may have nothing to do with this bug. This is for Bug 277554.

What happened here is that the document.open() call from the new temporary window that's opened here informed the secure browser UI about the new URI (wyciwyg:), but the following window.stop() call confused the secure browser UI code so that it never updated the security UI, so we're left showing the lock icon from the Yahoo page. One way to fix this might be to change the secure browser UI code to deal with this situation, but it made more sense to me to make the DOM code close() the document if we're stopped while we're writing into a document.

Comment on attachment 170873 [details] [diff] [review] Make sure the document is closed if it's stopped while document.writing r+sr=brendan@mozilla.org, don't need dveditz on this one (but he's welcome to 2nd the patch). /be

Comment on attachment 170873 [details] [diff] [review] Make sure the document is closed if it's stopped while document.writing a=asa (on behalf of drivers) for checkin to 1.8a6.

(In reply to comment #0) > Succeed: > window.opener.document.open(); > window.opener.stop(); > window.opener.document.write(spoofing); > > Failed: > window.opener.document.open(); > window.opener.document.write(spoofing); > window.opener.document.close(); > > To insure spoofing, you have to use "stop()" instead of "document.close()". If "stop()" doesn't exist, loading of document cannot stop. But certificate spoofing seems successful :-( http://beta51.minutedesign.com/test2-3.html

Argh, we need more than my initial fix. I've been looking at it, but no clean diff yet. More tomorrow.

Sorry for the trouble... (Learn from Bug 91403) Make document.open be sameOrigin to prevent another site from rewriting content. http://lxr.mozilla.org/mozilla/source/modules/libpref/src/init/all.js#260 - pref("capability.policy.default.HTMLDocument.close.get", "allAccess"); - pref("capability.policy.default.HTMLDocument.open.get", "allAccess"); ... can be solved the problem?

Oops. See Bug 91043.

correct diff option

It amazes me that this is needed, but I can't see how this code would work w/o a change like this. The secure browser UI code depends on nsIWebProgressListener notifications to keep the security state in sync with reality, but there's nothing that fires those events when we do document.write() since we never get any data off the network, and that's what drives the nsIWebProgressListener notifications. This patch fixes this problem by making sure that we update the security state in the one nsIWebProgressListener notification that we do get (OnLocationChange()) when we do document.open() etc, and I had to tweak some code in nsDocShell to make sure the request is passed to OnLocationChange() when we do document.open() etc.

Comment on attachment 170955 [details] [diff] [review] Make sure we update the security state when doing document.open()/write()... r/sr=me for 1.8a6, but i think we should find a better solution to this bug in the long-term. i'd rather leverage the existing onStateChange-based mechanism for passing info to the SecureBrowserUI. i believe we should be able to solve this bug alternatively by simulating a onStateChange(STATE_TRANSFERRING) event somehow.

Looks good to me too, ?=dveditz We should get this for 1.7.6 and any ff1.0.x

> this bug alternatively by simulating a onStateChange(STATE_TRANSFERRING) > event somehow. OK, I did some more investigation, and it seems to me that it would require a STATE_STOP event in addition to the STATE_TRANSFERRING event. The SecureBrowserUI unfortunately waits for STATE_STOP before updating the lock icon. That seems a little bit troubling to me. Instead, it should update the lock icon on the first STATE_TRANSFERRING event. Otherwise, I fear that other mechanisms besides wyciwyg could be used to trigger the same spoof. Investigating that possibility now.

It probably waits for STATE_STOP so it doesn't show a lock icon on a page and then switch to broken lock if near the end some random insecure content is loaded...

This fixes a problem caught by sicking where loading a secure wyciwyg request into a subframe would mark the whole window secure even if it isn't. This adds more of the existing logic in OnStateChange() into OnLocationChange() to take care of that.

Comment on attachment 170973 [details] [diff] [review] Do the right thing for sub-requests that are wyciwyg requests sr=me if sicking likes it. One nit: the big then clause with one-line else clause for "if (channel) {" in nsSecureBrowserUIImpl::EvaluateAndUpdateSecurityState would be clearer if you inverted the sense, testing 'if (!channel)' -- that shows the + mNewToplevelSecurityState = nsIWebProgressListener::STATE_IS_INSECURE; path taken in the absence of a channel more clearly. /be

Comment on attachment 170973 [details] [diff] [review] Do the right thing for sub-requests that are wyciwyg requests r=dveditz

Comment on attachment 170973 [details] [diff] [review] Do the right thing for sub-requests that are wyciwyg requests a=asa

Comment on attachment 170973 [details] [diff] [review] Do the right thing for sub-requests that are wyciwyg requests In nsSecureBrowserUIImpl::OnLocationChange >+ if (isWyciwyg) { >+ nsCOMPtr<nsIDOMWindow> windowForProgress; >+ aWebProgress->GetDOMWindow(getter_AddRefs(windowForProgress)); >+ >+ if (windowForProgress.get() == mWindow.get()) { >+ // For toplevel wyciwyg channels, upate the security state right >+ // away. >+ return EvaluateAndUpdateSecurityState(aRequest); >+ } >+ >+ // For wyciwyg channels in subdocuments we only update our >+ // subrequest state members. >+ UpdateSecurityState(aRequest); This call should be to UpdateSubrequestMembers. Other then that r=me And I agree with brendans comment, no biggie though.

Oh, and I think darins suggestion sounds like a better solution, though the word 'simulating' sounds a bit scary to me. The codepath in OnStateChange should have gotten it's fair share of testing so the more we can rely on it rather then writing new code, the better.

as for that STATE_STOP, bug 258048 is about changing that, it seems

OK, so as I feared, this bug can be reproduced without using document.open (i.e., it is not particular to wyciwyg). See bug 278003 for details.

Please keep this bug confidential until bug 278003 is fixed at least. Thanks!

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b) Gecko/20050114 Firefox/1.0+ The patch (attachment 170973 [details] [diff] [review]) seems to cause a crash of browser when you try testcases.

+ for 1.0.1

Need this for 1.7.6 too. Plussing.

Darin, should this fix be ported to the branches, or does a fix for bug 278003 supersede it?

Comment on attachment 170973 [details] [diff] [review] Do the right thing for sub-requests that are wyciwyg requests I think we want this patch on the aviary 1.0.1 and moz 1.7 branches. The patch for bug 258048 applies on top of this patch, and should be taken for those branches as well.

Comment on attachment 170973 [details] [diff] [review] Do the right thing for sub-requests that are wyciwyg requests a=dveditz for the 1.0.1 and 1.7 branches

fixed1.7.6, fixed-aviary1.0.1 marking this bug FIXED

jst: please verify that these are the correct deltas based on brendan and sicking's review comments. I applied these to attachment 170973 [details] [diff] [review].

Comment on attachment 174112 [details] [diff] [review] review comments delta Looks right to me.

Comment on attachment 174112 [details] [diff] [review] review comments delta sr=dveditz a=dveditz for branches, required to go with the other patch

Verified Fixed. Aviary 1.0.1 Branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20050217 Firefox/1.0 M18b1/Trunk: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050217 M176 Branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050217

I'm using Mozilla 1.7.6 on Windows. I tried http://beta51.minutedesign.com/test2-3.html. After I clicked the link, "Welcome to Yahoo" showed, and finally I got the screenshot. The url showed http://beta51.minutedesign.com/test2-3.html, but bottom-right lock icon appeared, any way the page info showed the connection was not encrypted.

WFM on 1.7.7 windows. Have any extensions or customized settings?

(In reply to comment #41) > WFM on 1.7.7 windows. Have any extensions or customized settings? No, it happens on fresh install Mozilla 1.7.7 windows, too. Maybe it depends on network, proxy and cpu speed. Sometimes WFM on Solaris. Here's the steps 1. go to test2-3.html. Click "Sign In - Yahoo!" 2. A dialog warns you're requesting an encrypted page. Click OK. 3. Lock icon appears. A dialog warns you're leaving an encrypted page. Click OK. 4. Unlock icon appears. Yahoo loads in the window. A dialog warns you're requesting an encrypted page. Click OK. 5. Lock icon appears. Click the lock icon, it says "Conntection Not Encrypted" like the screenshot. (BUG) 6. Enter "http"//www.google.com". A dialog warns you're leaving an encrypted page. (BUG)

not secure content in 1.4. so no patch needed for 1.4