Authentication dialog remains open for site that doesn't require authentication

VERIFIED FIXED in Firefox 34

Status

()

VERIFIED FIXED
7 years ago
2 years ago

People

(Reporter: lmandel, Assigned: wesj)

Tracking

({sec-low})

Trunk
Firefox 34
All
Android
sec-low
Points:
---

Firefox Tracking Flags

(firefox13 affected, firefox34 verified, firefox-esr31 wontfix, blocking-fennec1.0 soft)

Details

(Whiteboard: [mtd][sg:low][adv-main34-])

Attachments

(2 attachments, 2 obsolete attachments)

The authentication dialog can remain open when clicking a link to a site that doesn't require authentication if it was not dismissed before closing the browser.

Steps to reproduce:
1. Open Firefox and go to a URL that requires authentication, like http://intranet.mozilla.com.
2. Close the browser without dismissing the dialog but clicking the home button on your phone.
3. Click on a link that doesn't require authentication, like http://mozilla.org, in another application and open it in Firefox. The link loads but the authentication dialog is shown even though the site doesn't require authentication.

Actual results:
mozilla.org opens but is blocked by an authentication dialog.

Expected results:
mozilla.org should open and I should be able to read/interact with the page. The authentication dialog should be displayed when I tab back to the page that requires authentication.
OS: Mac OS X → Android
Hardware: x86 → ARM

Updated

7 years ago
Version: unspecified → Trunk

Updated

7 years ago
status-firefox13: --- → affected
Thins is kinda ugly, noming.
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → +
This holds good for any prompt shown -- say an dialog for <select>. This happens because, the PromptService is a "pseudo blocking call" (new name? ;)) which shows a dialog until it gets a result. This is basically not tied to "any tab". Gecko Events are still processed by polling (pseudo -- gets its name here).

The change would be to "tie a prompt dialog" to a tab (may be a list of prompts in the order of addition) and not make it blocking. The dialog will return the result, once the user selects an option.
Wes - Is there anywhere we can associate the tab to a prompt? I guess desktop does this in PromptService. We could do something similar.
Created attachment 610706 [details] [diff] [review]
Patch

As per discussions in IRC, this patch will load the page in background, if the prompt dialog is being shown for any website.
Attachment #610706 - Flags: review?(mark.finkle)
Comment on attachment 610706 [details] [diff] [review]
Patch

loadUrlInBackgroundIfNeeded and loadUrlInBackgroundTab kinda duplicate loadUrlInTab and it's adds some complexity to the intents. Also, we really want the intents to go through GeckoEvent.createLoadEvent and not GeckoEvent.createBroadcastEvent because the BrowserCLH handles routing the URL to nsBrowserAccess (nsIBrowserDOMWindow) in browser,js so it can do cool things for webapps and bookmarks.

Therefore, I'd be more interested in finding a way in browser.js to ask "is there a prompt showing?" and handle the background load there.

We could also pass "-background" as a commandline param to BrowserCLH via GeckoEvent.createLoadEvent but we'd need to add support to background tabs in nsBrowserAccess (nsIBrowserDOMWindow)
Attachment #610706 - Flags: review?(mark.finkle) → review-
Assignee: nobody → sriram
blocking-fennec1.0: + → soft
Curtis, can we figure out if this is exploitable or not?
Group: core-security
It's not directly exploitable, but could make for some good spoofing in a multi-tab situation.
Keywords: sec-low
Whiteboard: [mtd] → [mtd][sg:low]
Throwing this at Wes based on Comment 3; beats having it on Sriram's plate :D
Assignee: sriram.mozilla → wjohnston
Hardware: ARM → All
(Assignee)

Comment 9

4 years ago
Created attachment 8472594 [details] [diff] [review]
Patch 1/2 - refactor

I needed to know if the tab associated with this dialog was selected before showing it, so I had to do a little refactoring. For instance processMessage actually called show() at its end before. Since show() created and showed the dialog, there was no opportunity between when the dialog was created and shown to stop.

This removes processMessage entirely because we don't really need it (i.e. show(json) should do that anyway). It replaces show() with a create() and show() methods to separate the process in two.
Attachment #610706 - Attachment is obsolete: true
Attachment #8472594 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 10

4 years ago
Created attachment 8472597 [details] [diff] [review]
Patch 2/2 - Make prompts tab aware

This does the work to actually make prompts tab-aware. Note, we're still not quite tab-modal, i.e. you can't switch tabs while a prompt is showing. But if you exit Fennec and come back in to a new tab, we'll hold onto the dialog until you switch back to the tab it was on.
Attachment #8472597 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 11

4 years ago
Created attachment 8472598 [details] [diff] [review]
Patch 1/2 - refactor

Old patch had some code from patch 2 in it.
Attachment #8472594 - Attachment is obsolete: true
Attachment #8472594 - Flags: review?(lucasr.at.mozilla)
Attachment #8472598 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8472597 [details] [diff] [review]
Patch 2/2 - Make prompts tab aware

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

Looks good with the suggested change.

::: mobile/android/base/prompts/Prompt.java
@@ +56,5 @@
>  
>      private static boolean mInitialized;
>      private static int mInputPaddingSize;
>  
> +    private Tab mTab;

We should probably avoid keeping local references to Tab instances as they are owned by the Tabs class. We might end up with dangling Tab instances. It seems you only really need to keep a tab ID here. So I suggest changing mTab to be an int and set it to -1 when the tabId is not defined in the message.

@@ +109,5 @@
>              choiceMode = ListView.CHOICE_MODE_MULTIPLE;
>          }
>  
> +        if (message.has("tabId")) {
> +            final int id = message.optInt("tabId");

It's probably a good idea to be more explicit here and set a fallback value i.e. message.optInt("tabId", -1);

::: mobile/android/modules/Prompt.jsm
@@ -36,5 @@
>  
>    if ("hint" in aOptions && aOptions.hint != null)
>      this.msg.hint = aOptions.hint;
> -
> -  let idService = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);

Unrelated?
Attachment #8472597 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8472598 [details] [diff] [review]
Patch 1/2 - refactor

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

Nice.
Attachment #8472598 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/ea86f3073411
https://hg.mozilla.org/mozilla-central/rev/e08243c7ea06
Status: NEW → ASSIGNED
status-firefox34: --- → fixed
Target Milestone: --- → Firefox 34

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-firefox34: fixed → verified
status-firefox-esr31: --- → wontfix
Whiteboard: [mtd][sg:low] → [mtd][sg:low][adv-main34-]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.