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

Status: RESOLVED WORKSFORME

(Core :: Security, defect, P5)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase (verbose)

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase (small)

Here's a smaller testcase.

Comment 5

[Daniel Holbert [:dholbert]]

Save both testcases by right-clicking link and choosing "save-as" or using "view-source", rather than using File|Save.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I can't seem to reproduce this with a debug Linux trunk build, for what it's worth... :(

Comment 8

[Daniel Holbert [:dholbert]]

(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"

Comment 9

[Boris Zbarsky [:bzbarsky]]

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?

Comment 10

[Daniel Holbert [:dholbert]]

(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.)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Could be, yeah...

Are there any errors in the error console?  Make sure you enable reporting of chrome errors.

Comment 12

[Daniel Holbert [:dholbert]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

What's the stack to that assert?

Comment 14

[Daniel Holbert [:dholbert]]

(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.

Comment 15

[Daniel Holbert [:dholbert]]

(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

Comment 16

[Daniel Holbert [:dholbert]]

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?)

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: backtrace for assertion/notreached 'CachedChromeStream doesn't receive data'

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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....

Comment 19

[Boris Zbarsky [:bzbarsky]]

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?

Comment 20

[Daniel Holbert [:dholbert]]

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

Comment 21

[Lucas Adamski [:ladamski]]

Marking sg:high since it allows potential machine ownership with major mitigations.

Comment 22

[Brandon Sterne (:bsterne)]

Has anyone had any flashes of inspiration as to how to fix this one?

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: Screenshot: now the "default browser" dialog is the empty 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

Comment 24

[Daniel Holbert [:dholbert]]

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 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[[e-mail]]

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?

Comment 27

[[e-mail]]

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).

Comment 28

[[e-mail]]

Only the nsXULDocument.cpp changes from attachment 358702 [details] [diff] [review] are required here.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment 26 is correct.

Comment 30

[[e-mail]]

Now bug 206691 is fixed, can someone retest with the next 1.9.2a1pre Minefield?

Comment 31

[Daniel Holbert [:dholbert]]

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.

Comment 32

[[e-mail]]

OK, so what part(s) of attachment 358846 [details] [diff] [review] do we want on the branches?

Comment 33

[[e-mail]]

This should now have been fixed for 1.9.1 by changeset f0deff7035c6.