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

ASSIGNED
Assigned to

Status

()

defect
ASSIGNED
9 years ago
a year ago

People

(Reporter: ws.bugzilla, Assigned: developer, Mentored)

Tracking

(Blocks 1 bug)

unspecified
x86
Windows 7
Points:
13
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=c++], )

Attachments

(3 attachments, 5 obsolete attachments)

Reporter

Description

9 years ago
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.
Reporter

Updated

9 years ago
Blocks: 616843
Reporter

Comment 1

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

Updated

9 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

9 years ago
trying to make test case

Updated

9 years ago
Blocks: 59314

Updated

8 years ago
No longer blocks: 59314
Whiteboard: [defect] p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: [defect] p=0 → p=0
Whiteboard: p=0 → p=13
Points: --- → 13
Flags: qe-verify?
Whiteboard: p=13
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
Mentor: manishearth
Whiteboard: [good first bug][lang=c++]
(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
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.
Assignee: nobody → developer
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Attachment #8541506 - Attachment is obsolete: true
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
Attachment #8541509 - Flags: review?(nfroyd)
Attachment #8541509 - Flags: feedback+
Comment hidden (obsolete)
Comment hidden (obsolete)
Assignee

Comment 13

5 years ago
Attachment #8541510 - Attachment is obsolete: true
Attachment #8541510 - Flags: review?(nfroyd)
Attachment #8541512 - Flags: review?(nfroyd)
Attachment #8541512 - Flags: feedback+
Posted image preview.png
Just verified, patch seems to be working! Nice job!
Status: NEW → ASSIGNED
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 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.
Attachment #8541512 - Flags: review?(nfroyd) → review?(honzab.moz)
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
Attachment #8541512 - Flags: review?(honzab.moz)
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.
Honza, do you have any thoughts about comment 15? Will this code work correctly with tab-modal prompts?
Flags: needinfo?(honzab.moz)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.
Flags: needinfo?(honzab.moz)
(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.
ni? me
Flags: needinfo?(honzab.moz)
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.
Flags: needinfo?(honzab.moz)
Sushrut, in that case could you clean up the nits mentioned in the patch so that we can land this? Thanks!
(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.
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?
Flags: needinfo?(tvyas)
Please see the bug 832837, it's changing the form submission code.

We may be dependent on it.
See Also: → 832837
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.
Flags: needinfo?(tvyas)
Probably not GFB quality until we figure out what we really want to do here.
Mentor: manishearth
Whiteboard: [good first bug][lang=c++] → [lang=c++]
Assignee

Comment 30

4 years ago
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!
Mentor: manishearth
Whiteboard: [lang=c++] → [good first bug][lang=c++]
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
Depends on: 832837
Assignee

Comment 32

4 years ago
@David Keeler: Could you elaborate a bit more. Is the bug still re-entrant?
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.
See Also: → 1122237
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++ ?
Never mind: the code must have moved (and been renamed) and bug 799009 got rid of similar ones bug not this one.
(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.
(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).
(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?
You need to log in before you can comment on or make changes to this bug.