397678 - Windows nsOSHelperAppService uses wrong default apps in Vista

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[Dan Mosedale (:dmosedale, :dmose)]

In email, Rob Strong wrote: 

> I just noticed that on Vista the Launch application ui displays what is most
> likely the HKCR\mailto protocol handler when sending a link which is not how
> the default mailto handler is defined on Vista. It seems that we may be doing
> what we ask other apps not to do (see bug 369703 for an example and bug 395942
> for what is hopefully not an example of what may happen with us being the app
> that is going against the normal way of doing things).

Poking at the code, at the very least, nsOSHelperAppService::GetDefaultAppInfo
is going to need a different code path for Vista, and it's conceivable that GetApplicationDescription might want one too.

Comment 1

[Robert Strong (they/them - no direct email)]

note: my fear regarding the way we are doing things is that we are parsing the open command provided by the windows registry instead of using shellexecute (e.g. creating a partial implementation of the functionality of shellexecute in our apps) and as we have seen as of late launching other apps via the command line is often exploitable.

Comment 2

[Jim Mathies [:jimm]]

Attachment: vista app prefs patch v.1

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 287045 [details] [diff] [review]
vista app prefs patch v.1

I think the eyes that most need to review this bug are probably Rob (for his Windows expertise) and biesi, since he's both the module owner and a superreviewer.

Comment 4

[Jim Mathies [:jimm]]

Just a quick update on this. After doing some testing on the query current default call, I noticed some 3rd party handlers don't encapsulate their paths in quotes. Here are some examples:

feed - "C:\PROGRA~1\MICROS~2\Office12\OUTLOOK.EXE" /share "%1"
mailto - Outlook.URL.mailto
Outlook.URL.webcal - "C:\PROGRA~1\MICROS~2\Office12\OUTLOOK.EXE" /share "%1"
WindowsMail.Url.nntp - "C:\Program Files\Windows Mail\WinMail.exe" /newsurl:"%1"
acrobat - C:\Program Files\Adobe\Acrobat 7.0\Reader\AcroRd32.exe /u "%1"
tn3270 - url.dll,TelnetProtocolHandler %l
telnet - url.dll,TelnetProtocolHandler %l
zune - "C:\Program Files\Zune\Zune.exe" -link:"%1"

Note acrobat doesn't have quotes which is wrong, but still we should support this. In GetDefaultAppInfo, I'm using a first char quote check to differentiate between progids (like 'Outlook.URL.mailto') and paths. This will need an update before this gets checked in. It shouldn't hold up reviews though. I'll try to get an update later this week. 

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 287045 [details] [diff] [review]
vista app prefs patch v.1

>Index: uriloader/exthandler/win/nsOSHelperAppService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.cpp,v
>retrieving revision 1.76
>diff -u -p -8 -r1.76 nsOSHelperAppService.cpp
>--- uriloader/exthandler/win/nsOSHelperAppService.cpp	19 Oct 2007 04:15:43 -0000	1.76
>+++ uriloader/exthandler/win/nsOSHelperAppService.cpp	2 Nov 2007 03:56:32 -0000
>@@ -62,21 +62,35 @@
> #define LOG(args) PR_LOG(mLog, PR_LOG_DEBUG, args)
> 
> // helper methods: forward declarations...
> static nsresult GetExtensionFrom4xRegistryInfo(const nsACString& aMimeType, 
>                                                nsString& aFileExtension);
> static nsresult GetExtensionFromWindowsMimeDatabase(const nsACString& aMimeType,
>                                                     nsString& aFileExtension);
> 
>-nsOSHelperAppService::nsOSHelperAppService() : nsExternalHelperAppService()
>-{}
>+static const CLSID CLSID_AppAssoc = {0x591209C7,0x767B,0x42B2,{0x9F,0xBA,0x44,0xEE,0x46,0x15,0xF2,0xC7}};
>+static const IID   IID_IAppAssoc  = {0x4e530b0a,0xe611,0x4c77,{0xa3,0xac,0x90,0x31,0xd0,0x22,0x28,0x1b}};
>+
>+nsOSHelperAppService::nsOSHelperAppService() : 
>+  nsExternalHelperAppService(),
>+  mAppAssoc(nsnull)
>+{
>+  CoInitialize(0);
nit: CoInitialize(NULL) is used consistently in the Mozilla codebase instead of CoInitialize(0).

>+  CoCreateInstance(CLSID_AppAssoc, NULL, CLSCTX_INPROC,
>+                   IID_IAppAssoc, (void**)&mAppAssoc);
>+}
> 
> nsOSHelperAppService::~nsOSHelperAppService()
>-{}
>+{
>+  if (mAppAssoc)
>+    mAppAssoc->Release();
>+  mAppAssoc = nsnull;
>+  CoUninitialize();
>+}
> 
> // The windows registry provides a mime database key which lists a set of mime types and corresponding "Extension" values. 
> // we can use this to look up our mime type to see if there is a preferred extension for the mime type.
> static nsresult GetExtensionFromWindowsMimeDatabase(const nsACString& aMimeType,
>                                                     nsString& aFileExtension)
> {
>   nsAutoString mimeDatabaseKey;
>   mimeDatabaseKey.AssignLiteral("MIME\\Database\\Content Type\\");
>@@ -138,40 +152,76 @@ static nsresult GetExtensionFrom4xRegist
> }
> 
> nsresult nsOSHelperAppService::OSProtocolHandlerExists(const char * aProtocolScheme, PRBool * aHandlerExists)
> {
>   // look up the protocol scheme in the windows registry....if we find a match then we have a handler for it...
>   *aHandlerExists = PR_FALSE;
>   if (aProtocolScheme && *aProtocolScheme)
>   {
>-     HKEY hKey;
>-     LONG err = ::RegOpenKeyEx(HKEY_CLASSES_ROOT, aProtocolScheme, 0,
>-                               KEY_QUERY_VALUE, &hKey);
>-     if (err == ERROR_SUCCESS)
>-     {
>-       err = ::RegQueryValueEx(hKey, "URL Protocol", NULL, NULL, NULL, NULL);
>-       *aHandlerExists = (err == ERROR_SUCCESS);
>-       // close the key
>-       ::RegCloseKey(hKey);
>-     }
>+    // Vista: use the new application association COM interfaces
>+    // for resolving helpers.
>+    if (mAppAssoc) {
>+      HRESULT hr;
>+      PRUnichar * pResult = nsnull;
>+      NS_ConvertASCIItoUTF16 scheme(aProtocolScheme);
>+      // We are responsible for freeing returned strings.
>+      hr = mAppAssoc->QueryCurrentDefault(scheme.get(),
>+                                          AT_URLPROTOCOL, AL_EFFECTIVE,
>+                                          &pResult);
nit here and elsewhere
HRESULT hr = mAppAssoc->QueryCurrentDefault(scheme.get(),
                                            AT_URLPROTOCOL, AL_EFFECTIVE,
                                            &pResult);

>@@ -296,54 +346,65 @@ static void RemoveParameters(nsString& a
>...
>-  nsAutoString handlerKeyName(aTypeName);
>-  handlerKeyName.AppendLiteral("\\shell\\open\\command");
>-  nsCOMPtr<nsIWindowsRegKey> regKey = 
>-    do_CreateInstance("@mozilla.org/windows-registry-key;1");
>-  if (!regKey) 
>-    return NS_OK;
>+  if (aAppInfo.Length() == 0)
>+    return NS_ERROR_FAILURE;
Use aAppInfo.IsEmpty()

>@@ -587,17 +671,19 @@ already_AddRefed<nsIMIMEInfo> nsOSHelper
>   return mi;
> }
> 
> already_AddRefed<nsIHandlerInfo>
> nsOSHelperAppService::GetProtocolInfoFromOS(const nsACString &aScheme,
>                                             PRBool *found)
> {
>   NS_ASSERTION(!aScheme.IsEmpty(), "No scheme was specified!");
>-
>+  
>+  *found = PR_FALSE;
Is this really necessary with OSProtocolHandlerExists also setting it to PR_FALSE?

Handling the non quoted path is going to be tricky and I'd like to see that before plussing. Also, how about using shellexecute if the selected handler is the default handler?

Comment 6

[Robert Strong (they/them - no direct email)]

(In reply to comment #5)
>... Also, how about using shellexecute if the selected handler is
> the default handler?
Specifically, letting shellexecute choose the handler if the selected handler is the default handler?

Comment 7

[Jim Mathies [:jimm]]

We do this actually. This code is just responsible for retrieving information about the default handler prior to the user choosing what action to take. The actual execution of the content will pass through nsMIMEInfoWin's LaunchWithFile which chooses between the default handler or a custom handler if choosen via the app picker through nsHelperAppDlg. 

LaunchWithFile directs things between nsMIMEInfoWin's LaunchDefaultWithFile, which does a straight shellexecute on the target in nsLocalFile::Launch(), or it hands the content directly to a helper app, which is where we did those security patches.

Comment 8

[Jim Mathies [:jimm]]

>Handling the non quoted path is going to be tricky and I'd like to see that
>before plussing.

I can't believe they are returning two different results without indicating which result they return leaving it to the caller to decipher the result. 

That aside, I think I'll just do a reg open on the result in hkroot. That should filter out the progids from the paths. I also need to check to make sure our existing rundll32 handler code on the execute side handles weird stuff like 
tn3270.

Comment 9

[Jim Mathies [:jimm]]

>...or it hands the content directly to a helper app, which is where we did
>those security patches.

Sorry, that was incorrect, protocol handling gets sent off to nsMIMEInfoWin's LoadUriInternal, which is where those security patches landed. LaunchWithFile is where I recently added rundll32 handling.

Comment 10

[Jim Mathies [:jimm]]

Attachment: vista app prefs patch v.2

Updated. I'll need to test this on older versions of windows once I get back.

Comment 11

[Jim Mathies [:jimm]]

Attachment: vista app prefs patch v.3

Ok, did a bunch of testing on XP and Vista to make sure things were working correctly. Also made sure problems with apps that don't use quotes around extended paths was addressed (bug 395961).

Comment 13

[Jim Mathies [:jimm]]

Attachment: vista app prefs patch v.3

changing reviewers / removed a line of debug code

Comment 14

[Jim Mathies [:jimm]]

Ok, since it;s starting to look like this and bug 400852 might not make it in for the final, I'm going to split bug 395961 out of this patch and do it seperatly. It's a UI related problem that really needs to be fixed. The rest of this functionality could technically wait till after the final release.

Comment 15

[Jim Mathies [:jimm]]

downgrading...

Comment 16

[Robert Strong (they/them - no direct email)]

Jim, I'm reviewing bug 395961 and it would be a good thing to get a new patch for this bug in case we can get it for 3.0. I know this bug isn't a blocker but I'd still like to try to get it in.

Comment 17

[Jim Mathies [:jimm]]

yep - shooting for that. getting enough reviews is the only thing holding this up. I'm going to try and pair it down a bit, clean it up, add comments to help.

Comment 18

[Jim Mathies [:jimm]]

Attachment: vista app prefs patch v.4

- added commenting and additional testing

this includes the patch from bug 395961 which is needed to get this working right.

Comment 19

[Mike Schroepfer]

biesi - ping on review?

Comment 20

[Mike Schroepfer]

Jim can we use an alternative reviewer?  This patch has been sitting for nearly 2 months...

Comment 21

[Jim Mathies [:jimm]]

Well, I'm not sure. dmose and rstrong both took a look. rstrong gave it a thumbs up but felt it needed reviews from someone else. rob arnold might be a good candidate if he feels comfortable with it. he's darn good at windows related stuff. other than that, I'm out of options and open to suggestions.

Overall though it's a signifigant change. We could get away without it (with some complaints from vista users) until an update or 3.1. It's not suited to an RC release IMHO, it needs to bake in a beta or what not for a while. bug 395961 is far more important and isn't a major change like this.   

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 302855 [details] [diff] [review]
vista app prefs patch v.4

If you feel comfortable with this Rob, I'm looking for reviewers.

Comment 23

[Robert Strong (they/them - no direct email)]

With biesi's r+ along with mine I am fine with this landing without additional reviews. I am concerned about this not landing for Firefox 3 since the incorrect default mailto client for the user will be the one we tell the user to use, etc.

Comment 24

[Jim Mathies [:jimm]]

I'd be more comfortable landing this after the RC1 freeze so it at least has some time in nightlies to bake.

Comment 25

[Jim Mathies [:jimm]]

(side note - this will need an updated patch once bug 395961 lands since it includes that patch.)

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

While I haven't reviewed this code (there's a fair number of windows changes here that I wouldn't be a good reviewer for), it seems to me that this would be a pretty ugly bug to ship with.  So unless we already know that there's going to be an RC2, I'd argue for landing this now on the theory that we should front-load the risk as much as we can.

Comment 27

[Jim Mathies [:jimm]]

Attachment: vista app prefs patch v.5

Updated patch w/added com interface to protect older sdk builds.

Comment 28

[Jim Mathies [:jimm]]

Attachment: vista app prefs patch v.5

whitespace cleanup

Comment 29

[Jim Mathies [:jimm]]

Attachment: vista app prefs patch v.6

bugzilla didn't seem to like that one. lets try again.

Comment 30

[Damon Sicore (:damons)]

Comment on attachment 314473 [details] [diff] [review]
vista app prefs patch v.6

This didn't actually require approval since it's a blocker, but: 

I'm approving this, we'll have a couple of days of usage here before we ship RC.  Also, Rob has been using this patch for a few days now without problem.

Comment 31

[Reed Loden [:reed]]

uriloader/exthandler/win/IApplicationAssociationRegistration.h needs a license header before I land this.

Comment 32

[Jim Mathies [:jimm]]

Spoke to bsmedberg about this a while back. Since the code is adapted from an sdk header, he suggested no license, with a header that states where it came from.

Comment 33

[Jim Mathies [:jimm]]

Reed suggested I ask gerv to look at this. (cc'd). Also cc'ing bsmedberg. We're landing it because it needs as much bake time as possible. If there are any issues with the header I can update them tomorrow.

Comment 34

[Reed Loden [:reed]]

Checking in uriloader/exthandler/Makefile.in;
/cvsroot/mozilla/uriloader/exthandler/Makefile.in,v  <--  Makefile.in
new revision: 1.73; previous revision: 1.72
done
RCS file: /cvsroot/mozilla/uriloader/exthandler/win/IApplicationAssociationRegistration.h,v
done
Checking in uriloader/exthandler/win/IApplicationAssociationRegistration.h;
/cvsroot/mozilla/uriloader/exthandler/win/IApplicationAssociationRegistration.h,v  <--  IApplicationAssociationRegistration.h
initial revision: 1.1
done
Checking in uriloader/exthandler/win/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.84; previous revision: 1.83
done
Checking in uriloader/exthandler/win/nsOSHelperAppService.h;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.h,v  <--  nsOSHelperAppService.h
new revision: 1.32; previous revision: 1.31
done

Comment 35

[Gervase Markham [:gerv]]

Jim: please find out what the licensing terms are on the code you modified and post them here. Code in an SDK, particularly a Microsoft SDK, does not have "no license".

Gerv

Comment 36

[Jim Mathies [:jimm]]

Attachment: sdk header snippet

This includes the header of the file (a .h) and the original code I modified.

Comment 37

[Jim Mathies [:jimm]]

Attachment: License.htm from the root of the SDK

Comment 38

[Jim Mathies [:jimm]]

note the header was not listed in REDIST.TXT.

Comment 39

[Reed Loden [:reed]]

I'm reopening this to make sure it stays on the blocker list, as this licensing issue needs to be fixed.

Comment 40

[Gervase Markham [:gerv]]

If it's not listed in REDIST.TXT, we don't have a right to distribute it at all. And even if it was, the terms are not free-software-compatible. :-|

As I understand it, this file is required to make things build with pre-Vista SDKs. Is that right? If so, our options seem to be:

1) Remove the function
2) Require the Vista SDK
3) Reverse-engineer (as opposed to copy) the header file

All of these options suck at this stage in the release cycle. :-( Ideas?

Gerv

Comment 41

[(not currently active) Ted Mielczarek]

bug 412374 already added a dependency on the Vista SDK, although we added a configure flag to disable that functionality in bug 425979, since a lot of people still don't have the Vista SDK (and the Vista SDK doesn't actually work right with the express editions of Visual Studio currently). I think a similar approach would be ok here.

Comment 42

[Jim Mathies [:jimm]]

I'm not a fan of removing the functionality via an option. If we don't use these interfaces we break our integration with 3rd party apps on vista. If we offer the ability to disable their use, we end up with developers posting invalid bugs because they are not aware that critical functionality is being disabled. It's also important to note that this is not the only location we leverage this specific interface, we also use it in application registration. 

http://lxr.mozilla.org/mozilla/source/browser/components/shell/src/nsWindowsShellService.cpp#212
http://lxr.mozilla.org/mozilla/source/mail/components/shell/nsMailWinIntegration.cpp#640

disabling these uses would further make Mozilla apps incompatible with the os. 

I think obfuscating the code seems like the most plausible answer assuming we haven't already accomplished that with the existing code. (in which case we could just slap a mozilla license on it.) whether or not we've accomplished that isn't a question I can answer though. 

  

Comment 43

[Jim Mathies [:jimm]]

>As I understand it, this file is required to make things build with pre-Vista
>SDKs. Is that right?

Correct. officially we require the vista sdk. we do this for "better coverage",  for folks who don't want or can't use the vista sdk. mingwin builds are a good example.

Comment 44

[Gervase Markham [:gerv]]

OK. If the official builds are going to be fine without this file then, this close to release, it seems to me like the right thing to do is put the function behind an #ifdef HAVE_VISTA_SDK and remove the file.

Taking the existing file and "obfuscating" it isn't enough; it'll still be a derivative work of restricted code. It would need to be clean-room reverse engineered by someone studying the libraries in question.

Gerv

Comment 45

[(not currently active) Ted Mielczarek]

(In reply to comment #42)
> I'm not a fan of removing the functionality via an option. If we don't use
> these interfaces we break our integration with 3rd party apps on vista. If we
> offer the ability to disable their use, we end up with developers posting
> invalid bugs because they are not aware that critical functionality is being
> disabled. It's also important to note that this is not the only location we
> leverage this specific interface, we also use it in application registration. 

Jim: I don't think we'd wind up with invalid bugs filed if people have to explicitly disable it via a compiler option. If your mozconfig contains "--disable-vista-helper-app-service" or something like that, you'd have to be pretty dense to file bugs on it not working. (Not that I'm saying it won't happen, but I don't think we'll be flooded with them.) As I'm sure you noticed from the parental controls bug, changing build requirements affects a lot of people, so it benefits us to be careful and provide simple workarounds to allow people to disable features to continue building. Most people that build their own builds are not looking to redistribute their builds to a wide audience, and as such aren't likely to mind disabling some Vista-specific features to get a working build.

Comment 46

[Jim Mathies [:jimm]]

A one question before I tackle this - should we move away from individual features, al la "disable-vista-helper-app-service", "disable-vista-parental-controls", "disable-vista-application-registration" and just generalize something like "disable-msvc-6_x-sdk" or "disable-vista-sdk"? It seems kind of pointless to break this up since the root cause is the same in all cases.

Comment 47

[(not currently active) Ted Mielczarek]

Yeah, I'd support a |--disable-vista-sdk-requirements| or something like that, as long as it explained what it was disabling.

Comment 48

[Gervase Markham [:gerv]]

Jim: have you opened another bug for the necessary changes? If so, can you post the bug number here?

Thanks,

Gerv

Comment 49

[Jim Mathies [:jimm]]

yep - Bug 428970 

Comment 50

[cmtalbert]

I've verified this by checking the current nightlies against the previous beta builds.  Picking defaults works well with this fix on Vista.

Verified using this build: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008041606 Minefield/3.0pre

--> Verified.