Closed Bug 388887 Opened 17 years ago Closed 16 years ago

Privileges automatically enabled with fresh profile and "default browser" dialog

Categories

(Core :: Security, defect, P5)

x86
All
defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: dholbert, Unassigned)

References

Details

(Whiteboard: [sg:moderate] fixed by 206691)

Attachments

(4 files)

Posting test case in a sec. Requirements: - Testcase must be saved locally. (not viewed on the web) - Use a fresh profile - Specify testcase filename on command line, so it opens as first page. - Run a version of Trunk that's <b>not</b> your default browser. (The automatic "Make me your default browser?" dialog is needed for this testcase to work.) e.g. mkdir foo; /path/to/firefox -profile foo -no-remote /path/to/testcase.html Observed behavior: - An empty Ok/Cancel dialog box pops up behind Default Browser dialog. - Clicking Ok, Cancel, or even closing this dialog box, will all enable privileges. Expected behavior: - Allow/Deny Privileges dialog box should appear in place of the empty Ok/Cancel dialog. Even better (although not security-sensitive) would be for the Allow/Deny Privileges box to appear *in front* of the Default Browser box, because you can't click anything in Default Browser dialog until you resolve the Allow/Deny Privileges box.
Attached file testcase (verbose) (deleted) —
Mmmm.... fun and tangy event loop stuff. :( I would have thought that pushing and popping null js contexts properly during the event loop would make this better... guess not.
One caveat when reproducing bug on Windows -- to get the Default Browser box to appear, it looks like *no* version of Firefox can be your default browser. You can temporarily set IE 7 as your default browser via the "Programs" tab of IE7's "Internet Options" dialog. Also: When trying the testcase in Firefox 2, the Firefox window seems to lock up, without displaying any dialogs. It doesn't seem to enable the privileges in Firefox 2, though.
Attached file testcase (small) (deleted) —
Here's a smaller testcase.
Save both testcases by right-clicking link and choosing "save-as" or using "view-source", rather than using File|Save.
Summary: Privileges automatically enabled on fresh profile and "default browser" dialog → Privileges automatically enabled with fresh profile and "default browser" dialog
Flags: blocking1.9?
So this doesn't seem like it will happen very often so it isn't a huge problem. What I do think we should do though is make sure that we default to *not* granting access when we end up with this broken dialog. Could we make sure that happens more easily? Possibly that could be done in the call to the dialog, or by chaning the dialog itself.
Flags: blocking1.9? → blocking1.9+
I can't seem to reproduce this with a debug Linux trunk build, for what it's worth... :(
(In reply to comment #7) > I can't seem to reproduce this with a debug Linux trunk build, for what it's > worth... :( > Hmm... I just re-tested this with my up-to-date debug Linux trunk build (checked out this morning), as well as with the latest nightly, and I got these results: - My debug trunk build: I still get behavior as described in comment 0. - Latest Nightly build: The dialog boxes switch roles -- the "Privileges" one shows up in full, and the default-browser bugging one shows up as blank. (and no matter what button I click on the blank dialog, the nightly build of firefox is set as my default browser.) Here are my .mozconfig settings, for what it's worth: mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../obj mk_add_options MOZ_CO_PROJECT=browser ac_add_options --enable-application=browser ac_add_options --enable-debug --disable-optimize ac_add_options --enable-extensions=default,layout-debug ac_add_options --disable-libxul ac_add_options --disable-airbag ac_cv_visibility_pragma=no export CFLAGS="-fmessage-length=0" export CPPFLAGS="-fmessage-length=0"
Could be a matter of GTK version or something again... But really, this should not be happening. We now push the proper null principals everywhere. So what's going on?
(In reply to comment #9) > Could be a matter of GTK version or something again... Hmm... it's possible that the bug is masked in your particular version of GTK, but it's definitely not a GTK-*dependent* issue, because I can reproduce it in Windows. (Unless we use some GTK library on Windows, but I'd imagine that we don't) I just now tested the latest nightly trunk build on a WinXP box, and the bug reproduced with no trouble on both testcases. I got these results: - Verbose testcase: the Privileges dialog was blank (and auto-accepted) - Small testcase: the "default browser" dialog was blank (and auto-accepted) Based on this, I'd guess that there's a race condition between the two dialogs, and whichever dialog we request *latest* becomes blank and auto-accepted. (I'm guessing that the default-browser dialog always pops up at about the same time, as part of the launch process, whereas the privilege-dialog's timing depends on how long it takes us to render the page content.)
Could be, yeah... Are there any errors in the error console? Make sure you enable reporting of chrome errors.
I'm getting this assertion around the time the browser window appears, and I think it's probably related: ###!!! ASSERTION: CachedChromeStream doesn't receive data: 'Not Reached', file /scratch/work/builds/trunk.07-07-31.09-38/mozilla/content/xul/document/src/nsXULDocument.cpp, line 4329 mxr link: http://mxr.mozilla.org/seamonkey/source/content/xul/document/src/nsXULDocument.cpp#4329
What's the stack to that assert?
(In reply to comment #11) > Are there any errors in the error console? Make sure you enable reporting of > chrome errors. No errors in error console, with chrome error reporting turned on.
(In reply to comment #13) > What's the stack to that assert? It's weird... ---- #0 JS_AddRoot (cx=0xbfeb229c, rp=0xb7d3be76) at /scratch/work/builds/trunk.07-07-31.09-38/mozilla/js/src/jsapi.c:1745 #1 0xbfeb1ea8 in ?? () #2 0xbfeb229c in ?? () #3 0xb7d3be76 in JS_PrintTraceThingInfo ( buf=0xb73a9560 "\207(���\225:��\225:��\225:��\225:��\225:��\225:��\225:��\225:�", bufsize=3084375913, trc=0xbfeb1ea8, thing=0x10e9, kind=3084399906, details=3) at /scratch/work/builds/trunk.07-07-31.09-38/mozilla/js/src/jsapi.c:2017 Backtrace stopped: frame did not save the PC ----- Not sure why I get that weird backtrace for this assert... I get a nicer one from this earlier assertion: ###!!! ASSERTION: nsNSSComponent relies on profile manager to wait for synchronous shutdown of all network activity: 'mIsNetworkDown', file /scratch/work/builds/trunk.07-07-31.09-38/mozilla/security/manager/ssl/src/nsNSSComponent.cpp, line 2305
Actually, ignore that last backtrace -- it looks like it gives me a different backtrace every time I run it. (maybe it's reporting the backtrace from the wrong thread?)
Here's a backtrace for the assertion mentioned in comments 12,13,15,16. (I think I wasn't able to get one before because the assertion was in a library whose symbols hadn't successfully loaded in GDB, or something like that.) Ignore backtrace levels 0 and 1 -- those are from trapping into GDB. Level 2 is the actual NS_NOTREACHED.
Hmm. I wonder where we end up with a CachedChromeStreamListener as the listener for a DocumentOpenInfo.... that seems like the real bug here. I bet the fact that the same XUL file implements both dialogs is important here. And the fresh profile is probably key because you want to avoid fastload....
So in particular, when nsChromeProtocolHandler::NewChannel is called for whichever dialog comes up second we end up with the document not in the XUL prototype cache (beacuse it's still loading as a result of the first dialog coming up) so we start an actual load of the document (the second such load; one is already in progress). Then when the second load actually goes to create its XUL document via StartDocumentLoad(), we discover that the document is now in the cache and create a CachedChromeStreamListener instead of a parser to handle the load... but our load isn't coming from the cache, leading to this assertion. Basically, the code seems to assume that if the document is not in the cache at NewChannel() time it also won't be there at StartDocumentLoad() time. Which is not a great assumption, really. It would be OK if NewChannel() created the prototype and put it in the cache, but it doesn't seem to do that. The rest of the bug is more of a problem. The issue is that nsDialogParamBlock::nsDialogParamBlock does: 46 for(PRInt32 i = 0; i < kNumInts; i++) 47 mInt[i] = 0; and the value of buttonPressed is gotten as: 353 block->GetInt(eButtonPressed, buttonPressed); in nsPromptService::ConfirmEx. Since apparently no one fills in any of the ints on the dialog param block in this case, we get back 0, which means OK. Somewhere in this code, imo, we should probably preset that value to 1 on the dialog param block. Possibly in ConfirmEx, but maybe somewhere deeper. biesi, any ideas?
fwiw, I'm still seeing this bug using a fresh trunk build from today: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007110715 Minefield/3.0b2pre
Flags: wanted1.9.0.x+
Flags: tracking1.9+
Flags: blocking1.9-
Marking sg:high since it allows potential machine ownership with major mitigations.
Whiteboard: [sg:high]
Assignee: dveditz → nobody
Has anyone had any flashes of inspiration as to how to fix this one?
FWIW: In mozilla-central, it looks like the dialogs have switched position... The privileges dialog now appears in full and behaves normally, and the "default browser" dialog is the blank (and auto-accepted) one. Here's a screenshot of the new behavior. I tried this 5 times (with a fresh profile and copy of firefox each time, to make sure to spawn the right dialogs), and I got this new behavior repeatably. (Then I tried it with an old build just to make sure I wasn't doing something different, and I got the old/scary behavior.) So, assuming that the behavior has definitively changed, this bug still isn't fixed -- it's just changed slightly so as to be a little less scary. (but perhaps more annoying -- it changes my default browser without any clear notification or any way to cancel) The new build I tested (shows blank default-browser dialog): Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090122 Minefield/3.2a1pre The old build I tested (shows blank enable-privs dialog): Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007072504 Minefield/3.0a7pre
IIRC, I've seen similar blank-dialog-when-Firefox-starts issues with other dialog combinations -- e.g. default browser + master-password dialog, if you start Firefox with a page that uses one of your saved passwords. I think those issues are probably the same underlying bug as this one.
Comment 19 describes the way (or rather some alternative ways) to fix the "automatically enabled" part of this bug... It won't help the blank dialog thing, which basically needs some XUL help. There are various options there, including explicitly flagging on the channel where it's coming from, and having the XUL document check that.
So, to trigger the assertion, you need a) xul cache enabled and b) to open an uncached but cacheable document in two windows near-simultaneously?
Interestingly, my latest work for bug 206691 may well end up fixing this bug and also one cause of bug 465998 (another of the causes has since been fixed).
Only the nsXULDocument.cpp changes from attachment 358702 [details] [diff] [review] are required here.
Comment 26 is correct.
Now bug 206691 is fixed, can someone retest with the next 1.9.2a1pre Minefield?
Awesome -- this is WFM now, in the latest nightly! I tested yesterday's nightly, to confirm that it was still broken there, and it was. BROKEN (behaves as described in comment 23): Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090129 Minefield/3.2a1pre WORKING (neither dialog blank, neither dialog automatically confirms): Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090130 Minefield/3.2a1pre Resolving as WORKSFORME. Neil, feel free to change resolution to FIXED & add dependency on that other bug, if you're sure that's what fixed it.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → WORKSFORME
OK, so what part(s) of attachment 358846 [details] [diff] [review] do we want on the branches?
This should now have been fixed for 1.9.1 by changeset f0deff7035c6.
Depends on: 206691
Whiteboard: [sg:high] → [sg:moderate]
Whiteboard: [sg:moderate] → [sg:moderate] fixed by 206691
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.

Attachment

General

Created:
Updated:
Size: