1329940 - 'not secure' message shown on "secure" HTTPS popup login page

Status: VERIFIED FIXED

(Firefox :: Security, defect, P1)

Description

[Katelyn Gadd (:kael)]

Attachment: Screenshot 2017-01-10 01.53.03.png

Visiting the login page for my local grocery store's delivery site, Firefox showed a scary 'not secure' message in the little dropdown that appears when attempting to invoke password autocomplete (this is very unnerving and weird, also - see bug 1329188)

Naturally, I saw the message and tried to figure out what was up. The address bar showed a green lock (secure?) and the url was https. However, clicking the green lock showed a little message suggesting the page was insecure... with no explanation.

I never found any explanation for why Firefox was saying the secure page was also insecure. When I revisited a few minutes later, no security warnings appeared.

Temporary OCSP failure? I don't really know what happened here, but in my opinion it's DEFINITELY a bad thing that Firefox will call a page both secure and insecure at the same time. It worries me that I could be seeing the green lock on pages that are actually hijacked or compromised.

Comment 1

[Katelyn Gadd (:kael)]

Attachment: Screenshot 2017-01-10 01.53.20.png

Screenshot of cert info

Comment 2

[Johann Hofmann [:johannh]]

Thanks for reporting, this is indeed problematic.

There's also a thread on Reddit https://www.reddit.com/r/firefox/comments/5mwydb/why_does_logging_into_my_google_account_show_this/?sort=confidence

The first thing that comes to my mind that changed things recently is the work on insecure HTTP login warnings. Matt, Tanvi, do you know what could be causing this?

Comment 3

[Matthew N. [:MattN]]

For anyone who thinks they are seeing a false warning:

When you see this problem can you look at what was logged in the "security" category of the web console? Can you also evaluate `window.isSecureContext` in the login frame? This may be a bug with isSecureContext or a race synchronizing the message with the correct tab/page.

Comment 4

[Matthew N. [:MattN]]

STR of how you ended up on the page/tab when the warning was incorrect would also be good. e.g. "clicked a link on example.com and it opened a new tab to this page". Indicating whether you switched away from the tab and other tab switch data would also be good.

Comment 5

[Andrew McCreight [:mccr8]]

I've also seen this on Safeway's website.

STR:
1. go to http://www.safeway.com/
2. click on "grocery delivery" near the top middle of the page.
3. click on "sign in" in the upper right of the page that loads ( https://shop.safeway.com/ecom/home )
4. Click on the input box for the email address.

Note that if I directly go to https://shop.safeway.com/ecom/home and click on sign in (ie skipping steps 1 and 2) then I get no warning message.

Comment 6

[Andrew McCreight [:mccr8]]

This is the console message I got (when I clicked on "sign in" in step 3):

Password fields present on an insecure (http://) page. This is a security risk that allows user login credentials to be stolen.

Comment 7

[Matthew N. [:MattN]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> I've also seen this on Safeway's website.

Thanks, this is what I expected is happening:
http://www.safeway.com/ is window.open'ing https://shop.safeway.com/ecom/home
and so the opened tab is considered a non-secure context:
https://w3c.github.io/webappsec-secure-contexts/#examples-top-level

"Top-level documents are secure as long as they don’t have a non-secure opener browsing context. This is a bit convoluted, so let’s go straight to the examples:"

Comment 8

[Matthew N. [:MattN]]

Bug 1269568 is what caused us to be stricter and it's not arguably wrong that the page is marked as insecure but there is a mismatch between the address bar security indicator and the form warning. There is discussion on the spec at https://github.com/w3c/webappsec-secure-contexts/issues/42 about whether window.opener should be taken into consideration.

Comment 9

[Jonathan Watt [:jwatt]]

Seems like this is an evangelization issue to get such sites to use rel=noopener when they call window.open.

Comment 10

[Tanvi Vyas[:tanvi]]

I currently see a few solutions

1) Roll back https://bugzilla.mozilla.org/show_bug.cgi?id=1269568.  Will require some refactoring/rebasing.  We may end up loosening our restrictions (showing insecure warning on less pages than we should), but I would be okay with that for the short term.

2) Make an API for isSecureContext that takes an argument - boolean considerWindowOpener.  true by default, but false for passwords.

3) Add an about:config pref security.securecontext.considerWindowOpener = false by default.  Change isSecureContext to consider the value of that pref before doing a check on the opener.

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Tanvi Vyas - behind on bugmail [:tanvi] from comment #10)
> I currently see a few solutions
> 
> 1) Roll back https://bugzilla.mozilla.org/show_bug.cgi?id=1269568.  Will
> require some refactoring/rebasing.  We may end up loosening our restrictions
> (showing insecure warning on less pages than we should), but I would be okay
> with that for the short term.

That would be sad. Option #3 would be easier and keep things more restricted, so hopefully we can discount this option.

> 2) Make an API for isSecureContext that takes an argument - boolean
> considerWindowOpener.  true by default, but false for passwords.

Exposed to content? The WebIDL WindowOrWorkerGlobalScope.isSecureContext is a boolean attribute, so we'd need a separate function. Anyway, if we're going to relax things at all, creating a separate API that can ignore the opener state for password fields and other UI related functionality would be my preferred option.

This API could also be used to provide more informative error messages when devs find APIs are not available ("not a secure context because the window's opener was not secure" is more informative than "not a secure context").

> 3) Add an about:config pref security.securecontext.considerWindowOpener =
> false by default.  Change isSecureContext to consider the value of that pref
> before doing a check on the opener.

FYI the code to modify can be found by searching for "mOriginalOpenerWasSecureContext" in the source. Still, I'd prefer #2 since I'd rather we not relax our restrictions for APIs and other non-UI things, which this would do.

Comment 12

[Katelyn Gadd (:kael)]

(In reply to Jonathan Watt [:jwatt] from comment #9)
> Seems like this is an evangelization issue to get such sites to use
> rel=noopener when they call window.open.

From a security policy perspective this seems OK but the UI as-currently-designed/implemented seems profoundly suboptimal. The lock should be gone or the messaging should be way more clear. As-is I can't even figure out what's going on.

If window.opener is a huge security vuln, it seems like the lock should be gone because the page isn't actually secure. Or you specifically call out 'this form is insecure' instead of 'this page is insecure', since the latter is false and there's no UI to explain it.

Comment 13

[Matthew N. [:MattN]]

(In reply to Jonathan Watt [:jwatt] from comment #11)
> (In reply to Tanvi Vyas - behind on bugmail [:tanvi] from comment #10)
> > I currently see a few solutions
> > 
> > 1) Roll back https://bugzilla.mozilla.org/show_bug.cgi?id=1269568.  Will
> > require some refactoring/rebasing.  We may end up loosening our restrictions
> > (showing insecure warning on less pages than we should), but I would be okay
> > with that for the short term.
> 
> That would be sad. Option #3 would be easier and keep things more
> restricted, so hopefully we can discount this option.

I also would prefer #2 or #3 since I don't like when features implement their own security checks… it's just asking for security bugs.

> > 2) Make an API for isSecureContext that takes an argument - boolean
> > considerWindowOpener.  true by default, but false for passwords.
> 
> Exposed to content?

No, chrome-only.

> The WebIDL WindowOrWorkerGlobalScope.isSecureContext is
> a boolean attribute, so we'd need a separate function. Anyway, if we're
> going to relax things at all, creating a separate API that can ignore the
> opener state for password fields and other UI related functionality would be
> my preferred option.

Would it be fine to add the argument to `nsGlobalWindow::ComputeIsSecureContext` and add a chrome-only method to the window for this? Or do you have a better suggestion?

> This API could also be used to provide more informative error messages when
> devs find APIs are not available ("not a secure context because the window's
> opener was not secure" is more informative than "not a secure context").
> 
> > 3) Add an about:config pref security.securecontext.considerWindowOpener =
> > false by default.  Change isSecureContext to consider the value of that pref
> > before doing a check on the opener.
> 
> FYI the code to modify can be found by searching for
> "mOriginalOpenerWasSecureContext" in the source. Still, I'd prefer #2 since
> I'd rather we not relax our restrictions for APIs and other non-UI things,
> which this would do.

Thanks for the pointer.

Comment 14

[Jonathan Watt [:jwatt]]

(In reply to K. Gadd (:kael) from comment #12)
> the UI as-currently-designed/implemented seems profoundly suboptimal.

Agreed.

Comment 15

[Jonathan Watt [:jwatt]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #13)
> Would it be fine to add the argument to
> `nsGlobalWindow::ComputeIsSecureContext` and add a chrome-only method to the
> window for this? 

I think so, but I'd specifically like the new ComputeIsSecureContext call to come right after the current call, and set a member variable (maybe "mIsSecureContextIfOpenerIgnored") on the inner nsGlobalWindow. The chrome-only API would then return that stored value.

Comment 16

[Julien Cristau [:jcristau]]

Tracking for 52, seems like this is too confusing to ship as-is.

Comment 17

[Tanvi Vyas[:tanvi]]

Is this a problem too - https://people-mozilla.org/~tvyas/password/password_test.html ?

Url bar shows secure https page.  But form action is http, and hence we see the in-context warning.

Comment 18

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas - behind on bugmail [:tanvi] from comment #17)
> Is this a problem too -
> https://people-mozilla.org/~tvyas/password/password_test.html ?
> 
> Url bar shows secure https page.  But form action is http, and hence we see
> the in-context warning.

I'm leaning toward no, its not a problem.  The message is accurate.  The connection you would enter your password on is not secure.  And its an edge case; because if you actually click through this and try to submit a password, you get a modal dialog re-warning you about the issue.  That modal has been in Firefox forever, so this is likely to be a very rare issue that already has a poor user experience.

Comment 19

[Johann Hofmann [:johannh]]

(In reply to Tanvi Vyas - behind on bugmail [:tanvi] from comment #18)
> (In reply to Tanvi Vyas - behind on bugmail [:tanvi] from comment #17)
> > Is this a problem too -
> > https://people-mozilla.org/~tvyas/password/password_test.html ?
> > 
> > Url bar shows secure https page.  But form action is http, and hence we see
> > the in-context warning.
> 
> I'm leaning toward no, its not a problem.  The message is accurate.  The
> connection you would enter your password on is not secure.  And its an edge
> case; because if you actually click through this and try to submit a
> password, you get a modal dialog re-warning you about the issue.  That modal
> has been in Firefox forever, so this is likely to be a very rare issue that
> already has a poor user experience.

Yup, Telemetry suggests this virtually never happens:

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-12-09&keys=__none__!__none__!__none__&max_channel_version=release%252F50&measure=PWMGR_LOGIN_PAGE_SAFETY&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-11-04&table=0&trim=1&use_submission_date=0

Comment 21

[Tanvi Vyas[:tanvi]]

Thanks Johann!  Clearing the needinfo's I had added.  Lets not worry about the corner case described in comments 17-19.

Comment 22

[Tanvi Vyas[:tanvi]]

Given https://bugzilla.mozilla.org/show_bug.cgi?id=1331224, I'm starting to think we should just check if the loadingPrincipal is HTTP, and show the warning based on that.  Instead of using the Secure Context spec/definition.  That would be the more conservative approach, and the one Chrome is going to use.  We can switch to Secure Context with more testing and time.

So we could:
Add a pref:
security.insecure_password.securecontext.enabled.

#ifdef Nighty
security.insecure_password.securecontext.enabled = true
#else
security.insecure_password.securecontext.enabled = false
#

In the insecure password code, only use isSecureContext if the pref is enabled.

Comment 23

[Matthew N. [:MattN]]

(In reply to Johann Hofmann [:johannh] from comment #19)
> (In reply to Tanvi Vyas - behind on bugmail [:tanvi] from comment #18)
> > (In reply to Tanvi Vyas - behind on bugmail [:tanvi] from comment #17)
> > > Is this a problem too -
> > > https://people-mozilla.org/~tvyas/password/password_test.html ?
> > > 
> > > Url bar shows secure https page.  But form action is http, and hence we see
> > > the in-context warning.
> > 
> > I'm leaning toward no, its not a problem.  The message is accurate.  The
> > connection you would enter your password on is not secure.  And its an edge
> > case; because if you actually click through this and try to submit a
> > password, you get a modal dialog re-warning you about the issue.  That modal
> > has been in Firefox forever, so this is likely to be a very rare issue that
> > already has a poor user experience.
> 
> Yup, Telemetry suggests this virtually never happens:
> 
> https://telemetry.mozilla.org/new-pipeline/dist.html#!
> cumulative=0&end_date=2016-12-09&keys=__none__!__none__!
> __none__&max_channel_version=release%252F50&measure=PWMGR_LOGIN_PAGE_SAFETY&m
> in_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&star
> t_date=2016-11-04&table=0&trim=1&use_submission_date=0

I would be weary of that data since I think it may use isSecureContext too and therefore be skewed until this bug is fixed.

(In reply to Tanvi Vyas - behind on bugmail [:tanvi] from comment #22)
> Given https://bugzilla.mozilla.org/show_bug.cgi?id=1331224, I'm starting to
> think we should just check if the loadingPrincipal is HTTP, and show the
> warning based on that.  Instead of using the Secure Context spec/definition.
> That would be the more conservative approach, and the one Chrome is going to
> use.  We can switch to Secure Context with more testing and time.

That sounds more complex that the previous plan to use isSecureContext minus the window.opener check. I definitely think we need to continue to take the form action into account.

Comment 24

[Tanvi Vyas[:tanvi]]

Matt brings up a good point.  If we use isOriginPotentiallyTrusthworthy isntead of isSecureContext, then we will have to bring back the frame traversal logic to check every frame up to parent.  Which isn't hard, but does take a few lines of code and adds complexity.

I agree that we need to keep checking the form action.  That part of the code shouldn't change.

Comment 25

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Bug 1329940 - Ignore window.opener for password manager warnings

Review commit: https://reviewboard.mozilla.org/r/105890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105890/

Comment 26

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105890/diff/1-2/

Comment 27

[Tanvi Vyas[:tanvi]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

Adding jwatt, since he is most familiar with Secure Context code.  We will probably need a DOM peer to review too.

Comment 28

[Jonathan Watt [:jwatt]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

https://reviewboard.mozilla.org/r/105890/#review106888

::: dom/base/nsGlobalWindow.h:1759
(Diff revision 2)
>    void DisconnectEventTargetObjects();
>  
>    // Called only on outer windows to compute the value that will be returned by
>    // IsSecureContext() for the inner window that corresponds to aDocument.
> -  bool ComputeIsSecureContext(nsIDocument* aDocument);
> +  //bool ComputeIsSecureContext(nsIDocument* aDocument);
> +  bool ComputeIsSecureContext(nsIDocument* aDocument, bool aIgnoreOpener = false);

Please add an enum with eDefault and eIgnoreOpener values and document on the eIgnoreOpener what this value is for. An enum value makes the call sites clearer and is easier to audit.

::: dom/base/nsGlobalWindow.h:1793
(Diff revision 2)
>    // close us when the JS stops executing or that we have a close
>    // event posted.  If this is set, just ignore window.close() calls.
>    bool                          mHavePendingClose : 1;
>    bool                          mHadOriginalOpener : 1;
>    bool                          mOriginalOpenerWasSecureContext : 1;
> +  bool                          mIsSecureContextIgnoringOpener : 1;

I think mIsSecureContextIfOpenerIgnored would be a name less succeptible to misinterpritation. Similarly in other places such as the aIsSecureContextIgnoringOpener argument.

::: dom/base/nsGlobalWindow.cpp:2390
(Diff revision 2)
>  
>  bool
> -nsGlobalWindow::ComputeIsSecureContext(nsIDocument* aDocument)
> +nsGlobalWindow::ComputeIsSecureContext(nsIDocument* aDocument, bool aIgnoreOpener)
>  {
>    MOZ_ASSERT(IsOuterWindow());
> +  mIsSecureContextIgnoringOpener = false;

Please don't modify the nsGlobalWindow from this method. The method is supposed to return its result without side effects. (I'm not sure why it's not const.)

::: dom/base/nsGlobalWindow.cpp:2734
(Diff revision 2)
>  
>        mCreatingInnerWindow = true;
>        // Every script context we are initialized with must create a
>        // new global.
> +      bool isSecureContext = ComputeIsSecureContext(aDocument);
> +      bool isSecureContextIgnoringOpener = ComputeIsSecureContext(aDocument, true);

I don't think we should pass this into CreateNativeGlobalForInner to store on the global (see below).

::: dom/base/nsGlobalWindow.cpp:2744
(Diff revision 2)
> -                                      ComputeIsSecureContext(aDocument));
> +                                      isSecureContext,
> +                                      isSecureContextIgnoringOpener);
>        NS_ASSERTION(NS_SUCCEEDED(rv) && newInnerGlobal &&
>                     newInnerWindow->GetWrapperPreserveColor() == newInnerGlobal,
>                     "Failed to get script global");
> +      // set the secure context for inner window.

We're not "setting the secure context" here so this comment is confusing. I'd just not comment this personally.

::: dom/base/nsGlobalWindow.cpp:13867
(Diff revision 2)
> +bool
> +nsGlobalWindow::IsSecureContextIgnoringOpener() const
> +{
> +  MOZ_RELEASE_ASSERT(IsInnerWindow());
> +
> +  return JS_GetIsSecureContextIgnoringOpener(js::GetObjectCompartment(GetWrapperPreserveColor()));

Why not return mIsSecureContextIgnoringOpener here?

::: js/src/jsapi.h:2328
(Diff revision 2)
>      bool preserveJitCode_;
>      bool cloneSingletons_;
>      bool experimentalNumberFormatFormatToPartsEnabled_;
>      bool sharedMemoryAndAtomics_;
>      bool secureContext_;
> +    bool secureContextIgnoringOpener_;

The JS changes and this change in particular would need to be reviewed by a JS peer and would adeally mostly be a separate patch. That said I don't think this change is necessary though. Unless we need to propagate this value into workers (I don't think we do since this is only for UI things) then I think storing value on the nsGlobalWindow is sufficient.

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm:78
(Diff revision 2)
>     *
>     * @param {FormLike} aForm A form-like object. @See {LoginFormFactory}
>     * @return {boolean} whether the form is secure
>     */
>    isFormSecure(aForm) {
> -    let isSafePage = aForm.ownerDocument.defaultView.isSecureContext;
> +    // for now, we ignore the question of whether the opener is secure or not

It would be good to expand on this comment. Maybe something like:

We don't want to expose JavaScript APIs in a non-Secure Context even if the context is only insecure because the windows has an insecure opener. This prevents sites from implementing postMessage workarounds to enable an insecure opener to gain access to Secure Context-only APIs. However, in the case of form fields such as password fields we don't need to worry about whether the opener is secure or not. In fact to flag a password field as insecure in such circumstances would unnecessarily confuse our users.

Comment 29

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105890/diff/2-3/

Comment 30

[Matthew N. [:MattN]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

https://reviewboard.mozilla.org/r/105890/#review107222

The JSM change is fine (even if you end up renaming the attribute).

Comment 32

[Tanvi Vyas[:tanvi]]

Is this ready for another review by Jonathan?  If Matt and Jonathan both give you an r+, and I haven't looked at this yet, please proceed without my review.

Jonathan, do we need a DOM peer?  Who do you recommend for Secure Context work?

Comment 33

[Masatoshi Kimura [:emk]]

Any changes to WebIDL files will require a sign off from a DOM peer. Otherwise a commit hook will refuse to push the changeset.

Comment 34

[Jonathan Watt [:jwatt]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

https://reviewboard.mozilla.org/r/105890/#review107772

Getting closer, but still r- based on my nsGlobalWindow::ComputeIsSecureContext comments below. Please needinfo me once you've got a new patch available. Apparently another area where reviewboard is crap is that it didn't notify me that you had made a new patch available.

::: dom/base/nsGlobalWindow.h:195
(Diff revision 3)
>    }
>  };
>  
> +enum SecureContextFlags {
> +  eDefaultSecureContext = 0,
> +  eIgnoreOpener

Rather than declare this in the global scope please put it inside the class directly above the method that uses it (ComputeIsSecureContext). That method is private so the enum should be too.

Also please make this a scoped enum and then shorten the name of the default value to eDefault. So:

enum class SecureContextFlags {
  eDefault,
  eIgnoreOpener
};

You'll need to then change all the places that reference eIgnoreOpener to reference SecureContextFlags::eIgnoreOpener of course.

::: dom/base/nsGlobalWindow.h:1763
(Diff revision 3)
>  
>    void DisconnectEventTargetObjects();
>  
>    // Called only on outer windows to compute the value that will be returned by
>    // IsSecureContext() for the inner window that corresponds to aDocument.
> -  bool ComputeIsSecureContext(nsIDocument* aDocument);
> +  bool ComputeIsSecureContext(nsIDocument* aDocument, SecureContextFlags aIgnoreOpener = SecureContextFlags::eDefaultSecureContext);

Please add some line breaking here. Possibly format it like:

  bool ComputeIsSecureContext(nsIDocument* aDocument,
                              SecureContextFlags aIgnoreOpener =
                                SecureContextFlags::eDefaultSecureContext);

And please now rename aIgnoreOpener to aFlags.

::: dom/base/nsGlobalWindow.cpp:2387
(Diff revision 3)
>  
>    return JS_DefineFunctions(aCx, obj, EnablePrivilegeSpec);
>  }
>  
>  bool
> -nsGlobalWindow::ComputeIsSecureContext(nsIDocument* aDocument)
> +nsGlobalWindow::ComputeIsSecureContext(nsIDocument* aDocument, SecureContextFlags aIgnoreOpener)

Please change the argument name to |aFlags|.

::: dom/base/nsGlobalWindow.cpp:2390
(Diff revision 3)
>  
>  bool
> -nsGlobalWindow::ComputeIsSecureContext(nsIDocument* aDocument)
> +nsGlobalWindow::ComputeIsSecureContext(nsIDocument* aDocument, SecureContextFlags aIgnoreOpener)
>  {
>    MOZ_ASSERT(IsOuterWindow());
> +  bool isSecureContextIgnoringOpener = false;

For consistency, it would be better to name this isSecureContextIfOpenerIgnored?

::: dom/base/nsGlobalWindow.cpp:2397
(Diff revision 3)
>    nsCOMPtr<nsIPrincipal> principal = aDocument->NodePrincipal();
>    if (nsContentUtils::IsSystemPrincipal(principal)) {
>      return true;
>    }
>  
>    // Implement https://w3c.github.io/webappsec-secure-contexts/#settings-object

Please add a second // comment line here:

// With some modifications to allow for aFlags.

::: dom/base/nsGlobalWindow.cpp:2400
(Diff revision 3)
>    }
>  
>    // Implement https://w3c.github.io/webappsec-secure-contexts/#settings-object
>  
> +  bool hadNonSecureWindowOpener = false;
>    bool hadNonSecureContextCreator = false;

To clarify the terminology, "creator" is a catch-all spec term that includes both parent and opener. So if we were to split this variable then hadNonSecureContextCreator should be renamed hadNonSecureContextParent.

::: dom/base/nsGlobalWindow.cpp:2430
(Diff revision 3)
>    } else if (mHadOriginalOpener) {
> -    hadNonSecureContextCreator = !mOriginalOpenerWasSecureContext;
> +    hadNonSecureWindowOpener = !mOriginalOpenerWasSecureContext;
>    }
>  
>    if (hadNonSecureContextCreator) {
> +    // if our parent is not secure, we are never secure

Other than adding the second comment line above, I think the only change you should make to this function is in this |else if| block here. Specifically it should be changed to:

} else if (mHadOriginalOpener) {
  if (aFlags != SecureContextFlags::eIgnoreOpener) {
    hadNonSecureContextCreator = !mOriginalOpenerWasSecureContext;
  }
}

(Personally I think the aFlags check should be a separate |if| block like this since it's a divergence from the spec text, which this function is supposed to align with. It therefore seems nicer to me to to make that divergent check a separate |if| statement.)

::: dom/base/nsGlobalWindow.cpp:2435
(Diff revision 3)
> +    // if our parent is not secure, we are never secure
>      return false;
>    }
>  
> +  // this is true unless we decide the context is insecure below
> +  isSecureContextIgnoringOpener = true;

It doesn't make much sense to declare this variable and initialize it to false above, but then reinitialize it to true here without any references to the variable between these two points.

::: dom/base/nsGlobalWindow.cpp:2731
(Diff revision 3)
>  
>        mCreatingInnerWindow = true;
>        // Every script context we are initialized with must create a
>        // new global.
> +      bool isSecureContext = ComputeIsSecureContext(aDocument);
> +      bool isSecureContextIgnoringOpener = ComputeIsSecureContext(aDocument, eIgnoreOpener);

Again, for consistency, "isSecureContextIfOpenerIgnored". (Although personally since these variable are only once I'd inline the calls.)

::: dom/webidl/Window.webidl:519
(Diff revision 3)
>  };
>  
>  callback IdleRequestCallback = void (IdleDeadline deadline);
> +
> +partial interface Window {
> +  [ChromeOnly] readonly attribute boolean isSecureContextIfOpenerIgnored;

Please comment this, along the lines of:

  /\*\*
   \* Similar to |isSecureContext|, but doesn't pay attention to whether the
   \* window's opener (if any) is a secure context or not.
   \*
   \* WARNING: Do not use this unless you are familiar with the issues that
   \* taking opener state into account is designed to address (or else you may
   \* introduce security issues).  If in doubt, use |isSecureContext|.  In
   \* particular do not use this to gate access to JavaScript APIs.
   \*/

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm:80
(Diff revision 3)
>     * @return {boolean} whether the form is secure
>     */
>    isFormSecure(aForm) {
> -    let isSafePage = aForm.ownerDocument.defaultView.isSecureContext;
> +    // We don't want to expose JavaScript APIs in a non-Secure Context even if
> +    // the context is only insecure because the windows has an insecure opener.
> +    // This prevents sites from implementing postMessage workarounds to enable

On reflection "This prevents sites" would be better as "Doing so prevents sites". (Since at first readthrough you expect "this" to rever to "this section of code".)

And at the very end of the comment can you tack on "Hence we use isSecureContextIfOpenerIgnored here."

Comment 35

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #34)
> > -nsGlobalWindow::ComputeIsSecureContext(nsIDocument* aDocument)
> > +nsGlobalWindow::ComputeIsSecureContext(nsIDocument* aDocument, SecureContextFlags aIgnoreOpener)
> >  {
> >    MOZ_ASSERT(IsOuterWindow());
> > +  bool isSecureContextIgnoringOpener = false;
> 
> For consistency, it would be better to name this
> isSecureContextIfOpenerIgnored?

Ignore this comment, given that I then went on to suggest that the only things you should change about this function is the |if else| block and the comment.

> > +  bool hadNonSecureWindowOpener = false;
> >    bool hadNonSecureContextCreator = false;
> 
> To clarify the terminology, "creator" is a catch-all spec term that includes
> both parent and opener. So if we were to split this variable then
> hadNonSecureContextCreator should be renamed hadNonSecureContextParent.

Again, not relevant given the |if else| thing.

> > +  // this is true unless we decide the context is insecure below
> > +  isSecureContextIgnoringOpener = true;
> 
> It doesn't make much sense to declare this variable and initialize it to
> false above, but then reinitialize it to true here without any references to
> the variable between these two points.

Ditto.

> Please comment this, along the lines of:
> 
>   /\*\*
>    \* Similar to |isSecureContext|, but doesn't pay attention to whether the
>    \* window's opener (if any) is a secure context or not.
>    \*
>    \* WARNING: Do not use this unless you are familiar with the issues that
>    \* taking opener state into account is designed to address (or else you
> may
>    \* introduce security issues).  If in doubt, use |isSecureContext|.  In
>    \* particular do not use this to gate access to JavaScript APIs.
>    \*/

Looks like reviewboard mangled my comment, but I think you get the idea. ;)

Comment 36

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105890/diff/3-4/

Comment 37

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105890/diff/4-5/

Comment 38

[Jonathan Watt [:jwatt]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

https://reviewboard.mozilla.org/r/105890/#review108082

Comment 39

[Jonathan Watt [:jwatt]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

https://reviewboard.mozilla.org/r/105890/#review108092

::: dom/base/nsGlobalWindow.cpp:2423
(Diff revision 5)
>        return false; // we must be tearing down
>      }
>      MOZ_ASSERT(parentWin ==
>                 nsGlobalWindow::Cast(parentOuterWin->GetCurrentInnerWindow()),
>                 "Creator window mismatch while setting Secure Context state");
>      hadNonSecureContextCreator = !parentWin->IsSecureContext();

Actually this also needs to change to:

  if (aFlags != SecureContextFlags::eIgnoreOpener) {
    hadNonSecureContextCreator = !parentWin->IsSecureContext();
  } else {
    hadNonSecureContextCreator = !parentWin->IsSecureContextIfOpenerIgnored();
  }

Comment 41

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105890/diff/5-6/

Comment 42

[Andrea Marchesini [:baku]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

https://reviewboard.mozilla.org/r/105890/#review108362

Comment 43

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105890/diff/6-7/

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/903538212e6d
Ignore window.opener for password manager warnings r=baku,jwatt,MattN

Comment 45

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/903538212e6d

Comment 46

[Tanvi Vyas[:tanvi]]

Thanks Kate for taking this bug and fixing it quickly!  Assuming all goes well, please request uplift to aurora and beta on Friday.

Comment 47

[Landry Breuil (:gaston)]

Fwiw, got redirected here by jcristau - experienced this bug on 52.0b1 on OpenBSD, visiting http://www.mutuelledesmotards.fr/, clicking the blue 'espace societaire' link on the right opens an https cas login page in a new tab, which indeed shows the 'insecure passwd' warning, even if the form target is on https. I'll retest this once it gets backported to beta...

Comment 48

[Daniel Holbert [:dholbert]]

I can verify that my duplicate bug 1333687 is now partially fixed in Nightly -- the username/password fields no longer show the scary dropdown. But, I'm still seeing "Logins entered on this page may be compromised" in the Site Identity doorhanger (when I click the green EV-cert area) when I follow my STR (from duplicate bug 1333687).

Should I file a new bug on the Site Identity doorhanger? Or should we reopen this bug and try to fix both the form & the doorhanger ~atomically?   (The form + doorhanger warnings here seemed associated with each other to me, when I filed bug 1333687.)

Comment 49

[Daniel Holbert [:dholbert]]

Sorry, never mind about comment 48. After further mozregression research, it seems the Site Identity doorhanger "Logins...may be compromised" UI started appearing much earlier than the username/password-field warnings -- so that's indeed a separate issue, and I'll spin off another bug for it.

Comment 50

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Kate, should we consider uplifting this fix to Beta52 and Aurora53? I noticed it while reviewing 52+ tracked bugs.

Comment 51

[Kate McKinley [:kmckinley, :Kate]]

Ritu, yes, I would like to get this uplifted to Aurora53 and Beta52. The rationale is that the given password manager feature produces confusing results without this patch applied.

Comment 52

[Jonathan Watt [:jwatt]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

Approval Request Comment
[Feature/Bug causing the regression]: bug 1269568
[User impact if declined]: Some sites will incorrectly warn password fields are insecure - see dups also
[Is this code covered by automated tests?]: blocked on bug 1274315
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes. See comment 0 of this bug, and its dups for other sites, please
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Code is well understood and limited in what behavior it changes
[String changes made/needed]: none

Comment 53

[Julien Cristau [:jcristau]]

Comment on attachment 8828516 [details]
Bug 1329940 - Ignore window.opener for password manager warnings

don't warn about insecure password field because of window.opener, beta52+, aurora53+

Let's get this fix in 52.0b2

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/918d36ef8b13

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f28c092abe74

Comment 56

[Julien Cristau [:jcristau]]

This contextual warning is new in 52, marking 51 as unaffected.  The similar issue with the doorhanger should be covered by Daniel's bug 1334201.

Comment 57

[ovidiu boca[:Ovidiu]]

I have tried to test this issue with the steps from comment 5 but unfortunately I can’t go further than step 2 because the site is not working.(also tested on other browsers and the result is the same)

I also so comment 17 with a test case where the url bar shows secure https page but I get the warning message like on http pages. This is still reproducible with the latest Nighlty 54.0a1(2017-02-02).
 
Please advice me or suggest how can I test this issue. Thanks

Comment 58

[Daniel Holbert [:dholbert]]

(In reply to ovidiu boca[:Ovidiu] from comment #57)
> I have tried to test this issue with the steps from comment 5 but
> unfortunately I can’t go further than step 2 because the site is not
> working.(also tested on other browsers and the result is the same)

Huh, I can get past that step. Might be an IP-geolocation thing.
 
> I also so comment 17 with a test case where the url bar shows secure https
> page but I get the warning message like on http pages. This is still
> reproducible with the latest Nighlty 54.0a1(2017-02-02).

Cool -- that is the correct/expected behavior for that case, per comment 21.

> Please advice me or suggest how can I test this issue. Thanks

Could you try STR from duplicate bug 1333687? That should hopefully work.  (Basically: visit http://www.wedsure.com/, click Sign In at top right, and then click in username/password field.)

Comment 59

[ovidiu boca[:Ovidiu]]

Thanks Daniel,

I verified once again following the steps from bug 1333687 and I can't reproduce it any more with the latest FF Nighlty 54.0a1(2017-01-05). Tested on Mac 10.10, Win 10 and Ubuntu 16.04. 
Please note that on Mac this issue is not reproducible, I have tested with Nightly 53.0a1(2017-01-02) before the fix and I couldn't reproduce it. 
Please tell me if I can help with anything else.

Comment 61

[Hossain Al Ikram [:ikram] (QA Contact)]

I have reproduced this bug following the comment 5 with Nightly 53.0a1 (2017-01-10) on Windows 7, 64 bit!

The bug's fix is now verified on Latest Beta 52.0b8.

Build ID   :    20170220070057
User Agent : 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0

Comment 62

[Mohammad Maruf Rahman]

I have reproduced this bug following the comment 5 with Nightly 53.0a1 (2017-01-10) on Ubuntu 16.04 LTS, 64 bit!

The bug's fix is now verified on Latest Beta 52.0b8!

Build   ID :  20170220070057
User Agent :  Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0

[bugday-20170222]

Comment 63

[Cornel Ionce [:noni] [Hubs QA]]

Thanks guys for verifying this issue! Updating the tracking flags per above comments.