616849 - "... sent over an unencrypted connection" dialog should be tab-modal

Status: RESOLVED FIXED

(Firefox :: Security, defect, P2)

Description

[Roman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12 ( .NET CLR 3.5.30729; .NET4.0E)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12 ( .NET CLR 3.5.30729; .NET4.0E)

(Filing this bug as part of an effort to get the window-modal dialogs listed in bug 562258 fixed. The obvious fix is to make them tab-modal instead, but if a better idea has been suggested, please close as duplicate of the better idea.)

The following dialog:

"Although this page is encrypted, the information you have entered is to be
sent over an unencrypted connection and could easily be read by a third party.
... [Continue] [Cancel]"

is currently window-modal, blocking the entire Firefox window and switching tabs if it gets activated in a background tab. This is undesirable.


Reproducible: Always



Expected Results:  
The dialog should be displayed as tab-modal. The tab should remain closeable while the dialog is visible.

Comment 1

[Roman]

STR:
1. navigate to https://www.grc.com/x/ne.dll?bh0bkyd2
2. click the Proceed button.

Comment 2

[Biju]

Attachment: unencrypted_connection.html

trying to make test case

Comment 3

[Manish Goregaokar [:manishearth]]

The code we're trying to change is here[1]. Some instructions for making tab modal prompts can be found here[2], however they're in javascript. Fortunately they use XPCOM so we can do something similar in C++ and it should still work.

basically, we want to change the linked[1] call to confirmEx[3] so that the `allowTabModal


 [1]: http://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecurityWarningDialogs.cpp#134
 [2]: https://developer.mozilla.org/en-US/docs/Using_tab-modal_prompts
 [3]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPromptService#confirmEx%28%29

Comment 4

[Manish Goregaokar [:manishearth]]

(cont'd, accidentally submitted it)

..so that the `allowTabModal` property is set to true via the prompt bag[4]. There's an example of this being done here[5]


 [4]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWritablePropertyBag2
 [5]: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#6303

Comment 5

[Manish Goregaokar [:manishearth]]

Updating the MDN page to include C++ instructions after this bug has been successfully fixed is a good side-project for anyone working on this, btw.

Comment 6

[Sushrut Girdhari (sg345)]

Attachment: sg-v1.patch

Made changes to nsSecurityDialogs.cpp at line 105. Configured it to be tab-modal.

Comment 7

[Sushrut Girdhari (sg345)]

Attachment: name.patch

Comment 8

[Sushrut Girdhari (sg345)]

Attachment: tab.patch

Comment 9

[Sushrut Girdhari (sg345)]

Attachment: tab.patch

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 8541509 [details] [diff] [review]
tab.patch

Review of attachment 8541509 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/boot/src/nsSecurityWarningDialogs.cpp
@@ +104,5 @@
>    if (!prompt) return NS_ERROR_FAILURE;
>  
> +  if (nsCOMPtr<nsIWritablePropertyBag2> promptBag = do_QueryInterface(prompt)) {
> +      promptBag->SetPropertyAsBool(NS_LITERAL_STRING("allowTabModal"), true);
> +    }

Nit: there's an extra space before the brace

Comment 11

[Sushrut Girdhari (sg345)]

Attachment: tab.patch

I've uploaded the patch again as the previous patch was empty.

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8541510 [details] [diff] [review]
tab.patch

Review of attachment 8541510 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/boot/src/nsSecurityWarningDialogs.cpp
@@ +104,5 @@
>    if (!prompt) return NS_ERROR_FAILURE;
>  
> +  if (nsCOMPtr<nsIWritablePropertyBag2> promptBag = do_QueryInterface(prompt)) {
> +      promptBag->SetPropertyAsBool(NS_LITERAL_STRING("allowTabModal"), true);
> +    }

Nit: extra space still there :)

Comment 13

[Sushrut Girdhari (sg345)]

Attachment: fixed indentation

Comment 14

[Manish Goregaokar [:manishearth]]

Attachment: preview.png

Just verified, patch seems to be working! Nice job!

Comment 15

[Justin Dolske [:Dolske]]

I actually don't think this should have been marked as a good first bug (although it still might be).

Making prompts tab-modal is not as simple as setting the allowTabModal flag. You need to audit all the calling code to make sure it's ok with being reentrant, having the page closed while the prompt is showing, and otherwise ok with the browser's state changing. There's a lot of code that assumes prompts are literally modal, and that the user _must_ select one of the actions before the browser does anything else.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 8541512 [details] [diff] [review]
fixed indentation

Review of attachment 8541512 [details] [diff] [review]:
-----------------------------------------------------------------

I'm sorry, I know nothing about this code (and comment 15 makes me especially nervous).  Forwarding this review to somebody who has had some experience with this sort of stuff.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 8541512 [details] [diff] [review]
fixed indentation

Review of attachment 8541512 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/boot/src/nsSecurityWarningDialogs.cpp
@@ +104,5 @@
>    if (!prompt) return NS_ERROR_FAILURE;
>  
> +  if (nsCOMPtr<nsIWritablePropertyBag2> promptBag = do_QueryInterface(prompt)) {
> +      promptBag->SetPropertyAsBool(NS_LITERAL_STRING("allowTabModal"), true);
> +  }

please split out the comptr assignment out of the condition

Comment 18

[Biru [:poiru]]

Comment on attachment 8541512 [details] [diff] [review]
fixed indentation

Review of attachment 8541512 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/boot/src/nsSecurityWarningDialogs.cpp
@@ +103,5 @@
>    nsCOMPtr<nsIPrompt> prompt = do_GetInterface(ctx);
>    if (!prompt) return NS_ERROR_FAILURE;
>  
> +  if (nsCOMPtr<nsIWritablePropertyBag2> promptBag = do_QueryInterface(prompt)) {
> +      promptBag->SetPropertyAsBool(NS_LITERAL_STRING("allowTabModal"), true);

Nit: Please use two space indentation.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Honza, do you have any thoughts about comment 15? Will this code work correctly with tab-modal prompts?

Comment 20

[Honza Bambas (:mayhemer)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #19)
> Honza, do you have any thoughts about comment 15? Will this code work
> correctly with tab-modal prompts?

From the point of "this must block whole UI until decided" I say there is no need.  This is per tab only.

To answer the rest of concerns I cannot say much since I'm no expert to tab-modal dialogs and issues around them.  I know the calling code well but I don't know tab-modal mechanics if there is anything I need to know to make a good audit.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to Honza Bambas (:mayhemer) from comment #20)
> To answer the rest of concerns I cannot say much since I'm no expert to
> tab-modal dialogs and issues around them.  I know the calling code well but
> I don't know tab-modal mechanics if there is anything I need to know to make
> a good audit.

Dolske can probably offer more detail, but the tab-modal prompting code spins a nested event loop, which will spin during that prompt->ConfirmEx() call. It will unwind itself if the page in question is navigated away from, or the tab is closed, or the user responds, and maybe in some other cases, but it might also be theoretically possible for e.g. nsSecurityWarningDialogs::ConfirmDialog to re-enter. I guess the question is whether ConfirmPostToInsecureFromSecure callers (and their callers, etc.) will be OK with that nested event loop in general.

Comment 22

[Honza Bambas (:mayhemer)]

ni? me

Comment 23

[Honza Bambas (:mayhemer)]

So, performed few tests:
- an https: page with http: form action
- a timer started from onload to show a dialog
- a timer to change window.location
- an xhr that loads a slow content and in onreadystate shows a dialog
- a script referenced in body that loads slowly (10 seconds - a slow script) and at the end shows a dialog
- a slow script that navigates via altering window.location

The slow script in body is the only case when the dialog from the script overlaps the dialog for form submission (both tab-modal).

The slow script that navigates behaves with tab modal form submission dialog even better.  It does the navigation regardless the open tab modal dialog from system, but when the navigation is performed (not sure if it happens immediately or after when load is done/started to load) the tab modal dialog goes away.  When we use the browser modal one, it stays on the screen, so definitely there is a reentrancy *with* browser modal dialogs.  The reentrancy effects are mitigated with tab modal dialog for the form submission.

No crashes, nothing seems to be broken.

So, I vote for landing this.

Comment 24

[Manish Goregaokar [:manishearth]]

Sushrut, in that case could you clean up the nits mentioned in the patch so that we can land this? Thanks!

Comment 25

[Justin Dolske [:Dolske]]

(In reply to Honza Bambas (:mayhemer) from comment #23)

> No crashes, nothing seems to be broken.
> 
> So, I vote for landing this.

I'm not really comfortable with "it's didn't crash, so ship it". One needs to actually audit the relevant code paths to make sure they're fine with potential reentrancy.

What we really need to do to fix this issue and make it less fragile to to implement asynchronous prompts (ie, caller provides a callback, or get a Promise), and refactor code to use that.

Comment 26

[Honza Bambas (:mayhemer)]

The tests I've made suggest we already are reentrant with the current code.

Who should then do any audit?  And, there were also some thoughts (bugs?) to move this insecure-form-prompt code somewhere else.

Tanvi, can you elaborate please?

Comment 27

[Honza Bambas (:mayhemer)]

Please see the bug 832837, it's changing the form submission code.

We may be dependent on it.

Comment 28

[Tanvi Vyas[:tanvi]]

Looks like this will depend on bug 832837 which is moving the code for displaying this dialog and also reducing the text.

I'm fine with making this a tab-modal instead of a window-modal in general - this warning should block the tab but doesn't need to block the whole browser.  However, I'm not okay with the attached screenshot.  The continue button shouldn't be colored / more prominent than the cancel button.

I'd also like to keep the icon (I would actually like to change it to the mixed content icon - yellow triangle with an exclamation mark - for consistency, but that is for another bug).

Original dialog: https://people.mozilla.org/~tvyas/HTTPS-post-HTTP.jpg

Keeler says the code he has written in bug 832837 is reentrant.  He is probably the best person to ask about that.

Comment 29

[Manish Goregaokar [:manishearth]]

Probably not GFB quality until we figure out what we really want to do here.

Comment 30

[Sushrut Girdhari (sg345)]

Hi,

Please let me know what can be done to fix this bug. Should I go ahead with the nits suggested above (using two space indentation). Another nit suggested to split out nsCOMPtr assignment out of the condition. Does it mean I need to do something like this:


nsCOMPtr<nsIWritablePropertyBag2> promptBag = do_QueryInterface(prompt);
if(promptBag) { //code }

Thanks!

Comment 31

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

Bug 832837 changed where this code is. You'll need to modify HTMLFormElement::DoSecureToInsecureSubmitCheck now: https://hg.mozilla.org/mozilla-central/annotate/056c556b41b2/dom/html/HTMLFormElement.cpp#l865

Comment 32

[Sushrut Girdhari (sg345)]

@David Keeler: Could you elaborate a bit more. Is the bug still re-entrant?

Comment 33

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

There shouldn't be any reentrancy problems now, modulo the prompt service itself being safe. A good way to confirm would be to write tests for the cases you're concerned about.

Comment 34

[Johann Hofmann [:johannh]]

I think the right course of action is to remove this prompt once bug 1122237 is done. That doesn't have to block making it tab-modal, but I think this prompt has really outlived its usefulness now, if it was ever useful. The warning has consistently had over 90% click-through-rate[0]. We have in-content warnings for mixed login forms, but even those are _incredibly_ rare [1].

[0] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-11-15&keys=__none__!__none__!__none__&max_channel_version=release%252F57&measure=SECURITY_UI&min_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-11-12&table=0&trim=1&use_submission_date=0
[1] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-11-15&keys=__none__!__none__!__none__&max_channel_version=release%252F57&measure=PWMGR_LOGIN_PAGE_SAFETY&min_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-11-12&table=0&trim=1&use_submission_date=0

Comment 35

[Daniel Veditz [:dveditz]]

I thought this dialog went away in bug 799009? But then, that was fixed in Firefox 19 in 2012, and the code touched in the patch no longer exists. What have people been talking about in 2015++ ?

Comment 36

[Daniel Veditz [:dveditz]]

Never mind: the code must have moved (and been renamed) and bug 799009 got rid of similar ones bug not this one.

Comment 37

[Tanvi Vyas[:tanvi]]

(In reply to Johann Hofmann [:johannh] from comment #34)
> I think the right course of action is to remove this prompt once bug 1122237
> is done. That doesn't have to block making it tab-modal, but I think this
> prompt has really outlived its usefulness now, if it was ever useful. The
> warning has consistently had over 90% click-through-rate[0]. We have
> in-content warnings for mixed login forms, but even those are _incredibly_
> rare [1].
> 
> [0]
> https://telemetry.mozilla.org/new-pipeline/dist.html#!
> cumulative=0&end_date=2017-11-15&keys=__none__!__none__!
> __none__&max_channel_version=release%252F57&measure=SECURITY_UI&min_channel_v
> ersion=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&st
> art_date=2017-11-12&table=0&trim=1&use_submission_date=0
> [1]
> https://telemetry.mozilla.org/new-pipeline/dist.html#!
> cumulative=0&end_date=2017-11-15&keys=__none__!__none__!
> __none__&max_channel_version=release%252F57&measure=PWMGR_LOGIN_PAGE_SAFETY&m
> in_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=su
> bmissions&start_date=2017-11-12&table=0&trim=1&use_submission_date=0

bug 1122237 is low on the priority list.  We have to detect that the form action is insecure.  Sometimes the form action is an url, sometimes its javascript:..., sometimes its a javascript function.  When it is not a url, we can't tell if it is secure.  By the time the user hits submit and we can tell if it is secure, the page is likely about to navigate.  You can see this in Chrome, where the page flashes a mixed content icon for a second before moving on.

Hence, bug 1122237 is largely ineffective unless we can somehow parse javascript at page load time to determine whether the submit would go to an http destination or an https one.

The only stop gap we have is this annoying dialog.  It shows up after the javascript in the form action is parsed and we know the destination is insecure.

Looking at telemetry doesn't necessarily help - if a user is entering a search term on an https page and redirected to http (ex: searching for "rental cars"), then there is less to worry about and the user can click through the warning.  If they are entering credentials, then it is a bigger issue.  And the lock with the strikethrough in the url bar and the in context insecure password warning won't show up for an https page with a form action that results in passwords being submitted to an http destination.

We could modernize the "sent over an unencrypted connection" warning, indicating that it is a bigger problem when entering personal information, passwords, and credit cards.  And also updating the icongraphy so it looks similar to the insecure password UI.

Comment 38

[Johann Hofmann [:johannh]]

(In reply to Tanvi Vyas[:tanvi] from comment #37)
> And the lock with the strikethrough in the url bar and the in
> context insecure password warning won't show up for an https page with a
> form action that results in passwords being submitted to an http destination.

It does. The in-context warning appears.

My point is that this is a potential eviltraps DOS attack. It is very easy to lock users browser using this dialog. I think that this could be a bigger problem than mixed forms, which are virtually non-existent anymore, according to our telemetry data. It would be very easy to get rid of this eviltrap. Other browsers do not protect against this use case, either (I'd say we're already quite strong with the in-context warning).

Comment 39

[Tanvi Vyas[:tanvi]]

(In reply to Johann Hofmann [:johannh] from comment #38)
> (In reply to Tanvi Vyas[:tanvi] from comment #37)
> > And the lock with the strikethrough in the url bar and the in
> > context insecure password warning won't show up for an https page with a
> > form action that results in passwords being submitted to an http destination.
> 
> It does. The in-context warning appears.
> 
They see the in context warning if its a password field.

> My point is that this is a potential eviltraps DOS attack. It is very easy
> to lock users browser using this dialog. I think that this could be a bigger
> problem than mixed forms, which are virtually non-existent anymore,
> according to our telemetry data. It would be very easy to get rid of this
> eviltrap. Other browsers do not protect against this use case, either (I'd
> say we're already quite strong with the in-context warning).

You mean by using javascript to repeatedly and automatillcay submit forms to HTTP destinations?  And hence show this prompt over and over again without a way out?

Comment 40

[Johann Hofmann [:johannh]]

It's been a while so I'm unassigning folks. This has been abused in the wild (bug 1611517) and we're working on a solution to it in bug 1615588.

Comment 41

[Johann Hofmann [:johannh]]

(Although we could opt for making this a content modal prompt as done in the patch if necessary, I think, depending on how much this is actually abused in the wild).

Comment 43

[Paul Zühlcke [:pbz]]

Working on a patch for this based on Bug 1615588. It will make the prompt tab modal.

Comment 44

[Paul Zühlcke [:pbz]]

Attachment: Bug 616849 - Made insecure form submission prompt tab modal. r=johannh

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/825c1286b4e4
Made insecure form submission prompt tab modal. r=johannh,baku

Comment 46

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/825c1286b4e4