1491543 - SSL lock / indicator spoof is possible

Status: RESOLVED DUPLICATE of bug 1490982

(Firefox :: Site Identity, defect, P1)

Description

[James Lee]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.92 Safari/537.36

Steps to reproduce:

If we use JavaScript location property with setTimeout() function to relocate the page, then use Iframe to load the page inside, Mozilla Firefox confuses parent address as relocated site after redirection.

So while we are on same page, SSL lock is added to our desired redirected site. For this example, I used google blog so it'll show following Site Security information: "Verified by: Google Trust Services"

Test live on: http://pwning.click/fflockspoof.html

Proof Of Concept:

fflockspoof.html:

<iframe src="/ffspoofloc.html" width=0 height=0 style="visibility:hidden;"></iframe>

ffspoofloc.html:

<script>setTimeout ("window.location='https://xorigintest.blogspot.com'", 1);</script>


Tested on Firefox Nightly 64.0a1 (2018-09-14) (64-bit)



Actual results:

SSL lock / indicator is spoofed.


Expected results:

SSL lock / indicator should not be spoofed after redirection.

Comment 1

[:Gijs (he/him)]

Thanks for the report. I can reproduce on a recent nightly, but not on Firefox 63. Does that match what you're seeing (ie can you reproduce on 62 (release), 63 (beta and devedition) or 60.2 esr) ?

Also, I can only reproduce if I access the poc over http, not if I do so over https (so http://pwning.click/fflockspoof.html spoofs a lock, with incorrect info, and https://pwning.click/fflockspoof.html has a lock but the information in it is correct). Is that what you're seeing, too?

Comment 2

[:Gijs (he/him)]

This should really have been caught by automated tests, setting in-testsuite flag to make sure we add regression tests.

Comment 3

[:Gijs (he/him)]

Looks like bug 832834 regressed this, still confirming.

Comment 4

[:Gijs (he/him)]

Yeah, this regressed in bug 832834.

Comment 5

[James Lee]

Yes, this seems only works on Firefox Nightly and over http.

Comment 6

[:Gijs (he/him)]

Looks like after the refactoring the onLocationChange handler never checks if the channel it gets is for a toplevel frame or something else. I don't understand how it doesn't also update/change/break the tls info for https cases (ie why do the popup details still report Let's Encrypt instead of the google CA if you open https://pwning.click/fflockspoof.html ), but I guess that's another issue...

Offhand, I see no reason this wouldn't also work for EV certs.

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #6)
> Looks like after the refactoring the onLocationChange handler never checks
> if the channel it gets is for a toplevel frame or something else. I don't
> understand how it doesn't also update/change/break the tls info for https
> cases (ie why do the popup details still report Let's Encrypt instead of the
> google CA if you open https://pwning.click/fflockspoof.html ), but I guess
> that's another issue...

Oh, it's OK, the page info dialog (open dropdown panel, click arrow, click "more information") *is* affected/broken in the https case, too. I guess the panel doesn't update the tls info except when the lock status or toplevel location changes, so it gets the tls info from the initial navigation and then rightfully ignores further updates that don't alter the toplevel page, but the page info dialog actually queries the parent process browser for the latest security info.

Comment 8

[:Gijs (he/him)]

:keeler, I think this is effectively the same issue as bug 1490982. Can you confirm?

I have a local patch that looks like this:

$ hg export -r .
# HG changeset patch
# User Gijs Kruitbosch <[e-mail]>
# Date 1537033822 -3600
#      Sat Sep 15 18:50:22 2018 +0100
# Node ID 8cae40b1b5f0fbea9b5d44702ebe698b41a79b9a
# Parent  750e71a8f79b3b745a6653e555e60acb122f1241
Bug 1491543 - ignore non-toplevel updates in nsSecureBrowserUI, r?keeler,johannh

diff --git a/security/manager/ssl/nsSecureBrowserUIImpl.cpp b/security/manager/ssl/nsSecureBrowserUIImpl.cpp
--- a/security/manager/ssl/nsSecureBrowserUIImpl.cpp
+++ b/security/manager/ssl/nsSecureBrowserUIImpl.cpp
@@ -253,16 +253,22 @@ nsSecureBrowserUIImpl::OnLocationChange(
 
   NS_ENSURE_ARG(aWebProgress);
   NS_ENSURE_ARG(aLocation);
 
   MOZ_LOG(gSecureBrowserUILog, LogLevel::Debug,
           ("%p OnLocationChange: %p %p %s %x", this, aWebProgress, aRequest,
            aLocation->GetSpecOrDefault().get(), aFlags));
 
+  // Ignore non-toplevel updates:
+  bool isTopLevel = false;
+  if (NS_FAILED(aWebProgress->IsTopLevel(&isTopLevel)) || !isTopLevel) {
+    return NS_OK;
+  }
+
   // Clear any state that varies by location.
   if (!(aFlags & LOCATION_CHANGE_SAME_DOCUMENT)) {
     mState = 0;
     mTopLevelSecurityInfo = nullptr;
   }
 
   // NB: aRequest may be null. It may also not be QI-able to nsIChannel.
   nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest));

which seems to work. I'll leave a review comment on your patch in the other bug, I guess. I think the comment above onLocationChange is a lie.

Comment 9

[Johann Hofmann [:johannh]]

Yeah, pretty sure that comment isn't correct :)

Does the patch for bug 1490982 fix this bug, too? I.e. is the DOM window for the progress guaranteed to be the frame window? I remember from bug 1451307 that we were surprised to find that the window associated with a channel which loaded an iframe was the top-level window.

It might be sensible to add Gijs' patch as an extra precaution. I don't think it provides a lot of overhead and it's an easy safeguard. Maybe Gijs patch is entirely enough to fix both issues (e.g. why do we need to protect against about:blank at the top-level)?.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I added a test for this case in the patch for bug 1490982 (and the fix for that fixes this).

Comment 11

[James Lee]

Note that this still works on latest Firefox Nightly.

Comment 12

[Johann Hofmann [:johannh]]

(In reply to James Lee from comment #11)
> Note that this still works on latest Firefox Nightly.

I assume you tried 2018-09-19? It didn't land there. Can you try again with 2018-09-20? It's fixed in that version for me.

Comment 14

[James Lee]

Sorry for the late reply, I confirmed this issue has been fixed on latest Firefox Nightly.

Comment 15

[Johann Hofmann [:johannh]]

No worries thanks for checking! Opening this up since it only affected Nightly and has been fixed in a public bug.