170 bytes, text/html
4.20 KB, patch
|Details | Diff | Splinter Review|
1.44 KB, patch
|Details | Diff | Splinter Review|
1.04 KB, patch
|Details | Diff | Splinter Review|
This bug was reported to email@example.com by Jonathan Morgan: I moved his PoC code into the attached testcase which requires being signed in to a Google account to get the requisite 204 response from https://www.google.com/calendar/perf. Note the SSL indicators appear when viewing the testcase over http: as well as file:. From the email: ----- If a secure (https) page returns HTTP status 204 (no content), then the current page will show a verified security certificate for the current page. One such page is part of Google Calendar, which will give users a verified certificate for the site verified by Thawte. Quite a lot of people are perpetually signed in to their Google account, meaning that adding the following script to your site would make it appear verified and secure to any user who is currently signed into the Google account (even if it was an http site, not an https site). -----
I noticed that when you load the testcase from file:, Larry starts out with blank, but valid, SSL info. If you switch tabs to another SSL page and back, the SSL info from the last tab stays populated in the Larry dialog for the spoofed page.
Yeah, the UI is getting the wrong info from onSecurityChange - cc'ng people who have looked at that recently. We should fix this.
Similar to bug 514232, do they have the same underlying cause in the UI code?
I doubt it; in this case the docshell URI never changes. I'm not sure when the UI code gets onSecurityChange here or why....
BTW: Could anyone cc me please to bug 514232? Thanks.
Boris, looks like we broke this in bug 480713, 204 responses notifies STATE_TRANSFERRING that makes security UI take it into account and be driven by it as the request is also flagged as a document load (top level) which is correct. See http://hg.mozilla.org/mozilla-central/annotate/dbebe1771118/uriloader/base/nsDocLoader.cpp#l630. This patch https://bugzilla.mozilla.org/show_bug.cgi?id=329869#c11 is causing it. 204/205 responses should not change the document view (what correctly as spec'ed by RFC 2616 happens). One idea is that those should be taken by the security UI as subrequests. The other one is to not set the transferring flag in case of 204/205 responses as a quick fix, but then a new tab loading 204 will never have a ssl ui. However, the way security UI treats these request must change anyway.
We should probably just make security UI ignore 204/205 responses for document loads.
Created attachment 405898 [details] [diff] [review] v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16] Doing location = "http://..." from a secure page doesn't drop the security state. Question is if it has to or not as we would like to tell the user prior to making the request, bug 62178. If not then this is a sufficient patch.
How does imagelib (say) handle 204/205 responses with a body? Or CSSLoader, or scriptloader? How does this patch affect such responses when they're not the document request, if at all?
(In reply to comment #11) > How does imagelib (say) handle 204/205 responses with a body? > Or CSSLoader, or > scriptloader? Body is ignored on http transaction level: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpTransaction.cpp#857 >How does this patch affect such responses when they're not the > document request, if at all? It doesn't affect them at all. isToplevelProgress is checked (used for a condition) always with nsIChannel::LOAD_DOCUMENT_URI that cannot be set for anything other then a document.
Comment on attachment 405898 [details] [diff] [review] v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16] > Body is ignored on http transaction level: Ah, excellent. r=bzbarsky
Comment on attachment 405898 [details] [diff] [review] v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16] http://hg.mozilla.org/mozilla-central/rev/0eb9fe3674d5
Comment on attachment 405898 [details] [diff] [review] v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16] http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ddb2cb8d1d51
Comment on attachment 405898 [details] [diff] [review] v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1369259affe9
Created attachment 407003 [details] [diff] [review] v1 merge for 1.9.0 [Checkin 1.9.0 comment 17] /cvsroot/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp,v <-- nsSecureBrowserUIImpl.cpp new revision: 1.75; previous revision: 1.74
Uh, why did the 1.9.1 and 1.9.0 patches land without approval? blocking+ does not give you permission to land on anything <1.9.2. Explicit approval is required, as per the rules listed on tinderbox.
(In reply to comment #18) > Uh, why did the 1.9.1 and 1.9.0 patches land without approval? blocking+ does > not give you permission to land on anything <1.9.2. Explicit approval is > required, as per the rules listed on tinderbox. Ups, someone in the past told me that blocking flag gives that permission. Didn't know this has changed.
Nothing has changed. Maintenance branches always require explicit approval, as has been listed on the tinderbox pages for every maintenance branch for years.
Comment on attachment 405898 [details] [diff] [review] v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16] Approved for 22.214.171.124, a=dveditz for release-drivers
Comment on attachment 407003 [details] [diff] [review] v1 merge for 1.9.0 [Checkin 1.9.0 comment 17] Approved for 126.96.36.199, a=dveditz for release-drivers
Running the attached testcase in 188.8.131.52 and in a 184.108.40.206 nightly build, I receive a page that states "This is not a secure page." I notice that if I explicitly load http://www.google.com/calendar/render?tab=yc first and then visit the testcase, in 220.127.116.11, the lock icon shows an alert and the detailed view shows that there is unencrypted content. This does not *show* in 18.104.22.168. This only happens if I explicitly load Google Calendar first. If I just load the testcase, I see no visible difference between 22.214.171.124 and 126.96.36.199. In *both* instances, I was already signed into Google before running any tests. Looking at the bug history, it isn't clear to me whether we shipping this fixed in 188.8.131.52 because of the short cycle on the release. Is this message and behavior the correct ones for post-fix? It seems unlikely.
We did not fix this in 184.108.40.206.
Which then begs the question about the behavior I'm seeing.
This is kind of a weird bug to verify. The testcase first needs to be loaded as either a local file: resource or moved to a http: (not secure) server in order to be a valid test. The testcase, in unfixed versions, spoofs SSL indicators even though it is loaded as an insecure resource. The only reason the Google URL is being used is it responds with the 204 and empty response body. To get the 204, you also need to be logged in to a Google account, otherwise they redirect you to the login page which breaks the testcase. So to verify: 1. Log in to a Google account 2. Download testcase as local file: or put on http: server 3. Load the testcase a. if SSL indicators present -> broken b. if no SSL indicators -> correct
Running this locally, I get a lock icon in 220.127.116.11 but not 18.104.22.168 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:22.214.171.124pre) Gecko/2009111921 GranParadiso/3.0.16pre (.NET CLR 3.5.30729)). In 126.96.36.199 and 188.8.131.52 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:184.108.40.206pre) Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729)), I see the same behavior. So this is fixed for 220.127.116.11 and 18.104.22.168.
Created attachment 415093 [details] [diff] [review] 1.8.0 version window.get() -> mWindow.get() change
Marking the 14 bugs that are both: * nominated for blocking1.9.3:? * fixed on the 1.9.2 branch (according to status1.9.2) as blocking1.9.3:alpha1, so that we don't have to go through the nominations individually. They're all fixed already (so there's no work to do), and being fixed on 1.9.2 means they probably do block 1.9.3.