567497 - Files executed via download manager cause Win7 compatibility mode to permanently apply to firefox.exe and not downloaded file

Status: RESOLVED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[Ed Morley [:emorley]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3

If downloaded executable files are launched via the Firefox download manager and the downloaded file triggers the windows compatibility mode assistant - then the compatibility assistant wizard sees Firefox as the owner process and so any changes it makes (eg "always run as administrator") are applied to firefox.exe and not just the downloaded file. These changes are then applied on every subsequent Firefox launch, and cause a UAC elevation prompt each time and the worrying side effect of permanently running Firefox as an administrator.

Reproducible: Always

Steps to Reproduce:
1. Launch Firefox and download an installer that triggers Windows Compatibility mode (eg this utility installer: http://support.microsoft.com/kb/290301)
2. Double click the file in the download manager to launch it
3. Click "Run" on the "are you sure you wish to run this file from internet zone" type prompt
4. Dismiss the subsequent access denied scripting host error (the installer will now exit, having failed)
5. Close Firefox
6. Once the Firefox process has closed, Windows will present the "Program Compatibility Assistant" dialogue (see attached jpg). The process is identified as firefox.exe and not the downloaded file.
7. Choose "Restart the program as an administrator"
8. The compatibility mode assistant will precede to restart Firefox and not the downloaded file as an administrator. This will permanently change the Firefox settings to run as admin, unless the user manually reverses this by un-ticking the "run as admin" box in the firefox.exe file properties.
Actual Results:  
Firefox.exe received the "run as administrator" compatibility mode corrections, rather than the downloaded file. This causes a UAC prompt on every subsequent Firefox startup, which aside from being annoying, means that Firefox is running unnecessarily as an administrator, which has fairly severe security implications.

Expected Results:  
Only the downloaded file has the "run as admin" correction applied and not firefox.exe. ie: Files launched via the download manager are launched in such a way that firefox.exe is not seen as the owner process, so the compatibility mode assistant can apply the corrections to the correct file. 

Could reproduce on FF 3.6.3 and Minefield nightly 3.7a5pre BuildID=20100521035955.

NB: After manually reversing the "run as admin" file compatibility properties, the wizard appears to not run again even if the STR are repeated. I presume there must be compatibility mode assistant cache in the registry that prevents the same action being applied again. However I found that by running a different installed copy of Firefox (eg Minefield nightly) rather than my main 3.6 install, the compatibility mode wizard would be triggered again. To prevent the compatibility mode wizard caching the result, just chose cancel at step 7 above, and the rest of the STR can be recreated as many times as required.

Comment 1

[Ed Morley [:emorley]]

Attachment: Screenshot of comptibility mode assistant prompt

Comment 2

[Masatoshi Kimura [:emk]]

Confirming.
If I launch Firefox from command line, I see the PCA dialog against the downloaded file before step 5. You need to launch Firefox from Explorer to reproduce the bug.

Comment 3

[Masatoshi Kimura [:emk]]

This problem doesn't occur if I updated the manifest to Win7.

Comment 4

[Masatoshi Kimura [:emk]]

Sorry, this bug was about firefox.exe itself (not about the installer). Reopening.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: adding manifest to Win7

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Also changed manifests for binaries other than firefox.exe

Comment 7

[(not currently active) Ted Mielczarek]

I'm sure Thunderbird and SeaMonkey would like to similarly patch their manifests.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: comm-central counterpart

Comment 9

[Justin Wood (:Callek)]

(In reply to comment #7)
> I'm sure Thunderbird and SeaMonkey would like to similarly patch their
> manifests.

Thank you for the heads up!

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 446885 [details] [diff] [review]
Also changed manifests for binaries other than firefox.exe

Ted,
Should xulrunner/stub/xulrunner-stub.exe.manifest also have this change? (in case an XULRunner app wants to expose the Download Manager)

Comment 11

[Justin Wood (:Callek)]

Comment on attachment 446905 [details] [diff] [review]
comm-central counterpart

Emk, first off thank you for the patch!

[Stealing review]

>+++ b/calendar/sunbird/app/sunbird.exe.manifest

Not necessarily needed anymore [Sunbird is no longer maintained standalone], but since its already patched happy to take it.

>+++ b/suite/app/seamonkey.exe.manifest

Heh, looks like SM missed some .manifest work already; Thanks for catching us up!

Comment 12

[Mark Banner (:standard8)]

Note: If this needs backporting to comm-1.9.2, please make sure to the patch on a MailNews Core specific bug so that we can track its fix properly.

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: Add more files [Checkin: Comment 14]

Thanks for pointing out a missing file.
I've searched .exe.manifest from mxr.
http://mxr.mozilla.org/mozilla-central/find?text=&string=.exe.manifest
http://mxr.mozilla.org/comm-central/find?text=&string=.exe.manifest
I hope there are no missing files anymore...

Comment 14

[Justin Wood (:Callek)]

Pushed as: http://hg.mozilla.org/mozilla-central/rev/63b89b311461

Comment 15

[Masatoshi Kimura [:emk]]

Justin, did you check-in comm-central patch?

Comment 16

[Justin Wood (:Callek)]

(In reply to comment #15)
> Justin, did you check-in comm-central patch?

Err no, whops; sorry.

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: 1.9.2 branch patch

This fix should be landed on the branch along with bug 522065.
I think review is not needed because an only change is the path of updater.exe.manifest (toolkit/mozapps/update/updater/ -> toolkit/mozapps/update/src/updater/)

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: 1.9.1 branch patch

Removed missing files (plugin-container.exe.manifest and nsinstall.exe.manifest) only.

Comment 19

[Ed Morley [:emorley]]

Have just tested this on the latest Minefield and can confirm this is now resolved for me.

Thanks!

Comment 20

[LegNeato]

We will not be taking this for .6/.11, moving nom for .7/.12

Comment 21

[Justin Wood (:Callek)]

(In reply to comment #8)
> Created attachment 446905 [details] [diff] [review]
> comm-central counterpart

...I'm sorry... 

My neglect in pushing this live has caused this patch to bitrot, (I did not verify if someone did push and did not comment here though).

Within the next few days I'll *try* to find time to update the patch for you, but if you can manage doing that and re-add checkin-needed I can get this a bit faster for you.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 452427 [details] [diff] [review]
1.9.2 branch patch

Approved for 1.9.2.9, a=dveditz for release-drivers

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 452428 [details] [diff] [review]
1.9.1 branch patch

Approved for 1.9.1.12, a=dveditz for release-drivers

Comment 24

[LegNeato]

Comment on attachment 452427 [details] [diff] [review]
1.9.2 branch patch

Removing .9 &.12 approval as this didn't land before freeze. Feel free to nominate again, though the bar for approval will be higher.

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Regrettable that these patches slipped through the cracks, but we still should take this.  It's relatively easy to hit, and painful for users that don't realize what's going on.

Comment 26

[Justin Wood (:Callek)]

Callek <- TODO fix and checkin patch[es] to c-c.

Comment 27

[Justin Wood (:Callek)]

(In reply to comment #26)
> Callek <- TODO fix and checkin patch[es] to c-c.

Err actually I just am overly forgetful lately:

http://hg.mozilla.org/comm-central/rev/8a82a37cab87

and *I* pushed it beginning of July http://hg.mozilla.org/comm-central/pushloghtml?changeset=8a82a37cab87

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9fc9768e25c0
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5068a37ab3c8

Comment 29

[Philip Chee]

Pushed to comm-1.9.1 a=Callek.
http://hg.mozilla.org/releases/comm-1.9.1/rev/f1de05cd3b11