394974 - (CVE-2007-4841) URIs with invalid % encodings launch wrong handler on WinXP+IE7

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Daniel Veditz [:dveditz]]

spun off from bug 389580 which was marked FIXED with the landing of an incomplete fix and that's masked the remaining work.

On Windows XP with IE7 installed (but not XP+IE6 or Vista with IE7) URIs handed off to ShellExecute() for launching with an external protocol handler can be handed off to an unexpected file handler based on the "extension" of the URI if the URI contains a '%' followed by an invalid hex sequence (or "%00", but that was wallpapered in 389580). Per bug 389580 comment 36 the affected schemes are
 http, https, ftp, file, gopher, telnet, mailto, news, snews, nttp, wais, mk

(Handling the schemes internally avoids the problem, but Firefox will call the OS for mail URIs and Thunderbird will hand off http and the like)

In bug 389580 comment 37 jmathies attached a first cut at validating URIs through the newly-introduced Windows API CreateURI, which will exist on all systems affected by this bug. This approach means we don't have to try to guess what Windows is doing and worry about whether our list of schemes is complete or not.

This bug is filed to continue this work in a new space. The old bug made release tracking impossible because of the confusing flags already set and the fact that "fixed" bugs are invisible to the FF3/Gecko1.9 planners.

Comment 1

[Daniel Veditz [:dveditz]]

Adding recent blog about the potential problem to catch dupage
http://xs-sniper.com/blog/2007/09/01/firefox-file-handling-woes/

(the blog glosses over how they get an attacker-supplied script file downloaded into a predictable location on the user's computer. If they can actually do it that would be a problem in its own right and should be handled in a separate bug. This bug is simply about side-stepping %-containing URI bustage on Windows)

Comment 2

[Jesse Ruderman]

biesi looked at this last week, and IIRC he concluded that the patch using CreateURI would cause some URLs that work in IE to start failing in Firefox.  biesi, can you comment with details?

Maybe we need to get in contact with Microsoft to figure out which APIs they think we should use.

Comment 3

[Jesse Ruderman]

This sounds to me like a Windows XP flaw: an application asks Windows to launch a protocol handler for a protocol, and because the string contains some % stuff, Windows decides to do something entirely different and launch a filename that appears inside the URL.  Am I missing something?  Are we using an ambiguous "launch this thing" API when we should be using a more specific "launch this URL" API?

Comment 4

[Jesse Ruderman]

Can Firefox detect the "invalid % encodings" that cause Windows to have this bizarre behavior?  If so, the worst case is that we escape those %s and break a few apps, or throw up a dialog saying "Due to a flaw in Windows, Firefox cannot launch this URL".  IMO, either of those would be better than remaining vulnerable for weeks and weeks.

Comment 5

[Jim Mathies [:jimm]]

Just an FYI, I'm touching up the original experimental patch I did and a new one related to the suggestions we received from outside sources. I'll post them here as soon as I get them built and tested.

Comment 6

[Daniel Veditz [:dveditz]]

Jesse: yes, this is a Windows flaw. If you pass a mailto: url to windows ShellExecute and it launches Windows Media Player because there a .wmv at the end or Windows Scripting Host because it's got .vbs that's windows doing something wacky. Several external analysts (Secunia, Hiese) have agreed with that PoV.

MS sidestepped my direct question "Does Microsoft acknowledge this as a bug?" but were very helpful in providing some technical details

In any case it's a potentially dangerous situation and even though we think it's a windows problem, and MS may or may not agree, it's likely that Windows isn't going to change any time soon. If we can we need to protect our users.

Comment 7

[Daniel Veditz [:dveditz]]

The suggestion we got from the Microsoft Security Response Center was

  The recommendation I have been given are as follows:

  Call SHParseDisplayName with the URL/string. If it succeeds then call
  ShellExecuteEx, passing it SEE_MASK_INVOKEIDLIST flag and the PIDL. 

  Calling CreateUri and testing for success is an alternative approach that
  would also work as well for this case.  However, SHParseDisplayName comes
  closer to mimicking the internal behavior of ShellExecute, whereas CreateUri
  is only part of a specific folder's parsing behavior.

My request: please add this code to the windows platform-specific files that actually call ShellExecute rather than #ifdefing the common routine if it can at all be helped (nsMIMEInfoWin::LoadUriInternal).

Comment 8

[Jim Mathies [:jimm]]

I'll have both approaches posted this afternoon for testing.

Comment 9

[Jim Mathies [:jimm]]

Attachment: createuri patch

Comment 10

[Jim Mathies [:jimm]]

Attachment: BonEcho SHParseDisplayName patch

This patch seems like the best approach. It doesn't appear to suffer from the problems the createuri patch does related to parameter passing.

Comment 11

[Jim Mathies [:jimm]]

Note, these patches are not compatible with Trunk. The handling there has changed quite a bit since BonEcho. These patches were applied to FIREFOX_2_0_0_6_RELEASE.

Comment 12

[Daniel Veditz [:dveditz]]

Jim: are you ready to get reviews on either of the branch patches? we'd like to get this landed.

Comment 13

[Jim Mathies [:jimm]]

Sure - shparsedisplayname patch is ready to go. That's the solution MS suggested we use. I should have a trunk version here this weekend if all goes well.  

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 280086 [details] [diff] [review]
BonEcho SHParseDisplayName patch

Rob: please review, or suggest someone else appropriate.

Benjamin: seeking sr as a module-owner approval

Comment 15

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

Comment on attachment 280086 [details] [diff] [review]
BonEcho SHParseDisplayName patch

I don't know this code well enough to review it... perhaps biesi?

Couple of notes:
SHParseDisplayName has the following note that makes me think that CreateURI might be a better choice:
You should call this function from a background thread. Failure to do so could cause the user interface (UI) to stop responding.

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/reference/functions/shparsedisplayname.asp

The statement about SHParseDisplayName coming closer to mimicking the internal behavior of shellexecute vs. CreateURI doesn't give me any reassurance considering that we are calling shellexecute. I suspect the statement is that SHParseDisplayName closer mimics shellexecute on Vista in which case then I am fine with the statement.

For ShellExecute NULL is specified for the owner window and for ShellExecuteEx you are specifying the desktop window.

>+      // Version of shell32.dll < 6.0, SP2 is not installed
I believe you are referring to *Windows XP* SP2?

Comment 16

[Jim Mathies [:jimm]]

>You should call this function from a background thread. Failure to do so could
>cause the user interface (UI) to stop responding.

You know I'm not convinced this is an issue in the way we are using it. This call is oriented around binding UI menu items to shell items which is then invoked by the user. (This is the setup end of things.) So for example you might get a slight "hang" when resolving something like a network folder using this call. But afaict you're not going to get hangs resolving local handers like mailtos.  


In general - I was under the impression that before we reviewed / checked anything in both these 'experimental' patches were going to undergo a lot of QA? The bug was marked qawanted a while back but I haven't seen any activity on it since then. Seems like we missed a step and jumped right to review / check-in?

Comment 17

[Jim Mathies [:jimm]]

Attachment: GranParadiso SHParseDisplayName patch

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 280086 [details] [diff] [review]
BonEcho SHParseDisplayName patch

I had to add ole32 to the OS_LIBS in mozilla/docshell/build/Makefile.in to get this to build (because of the CoTaskMemFree call).

>+      if (SUCCEEDED(pMySHParseDisplayName(NS_ConvertASCIItoUTF16(urlSpec).get(), NULL, &pidl, 0, &sfgao))) {
  ...
>+        if (!bRes || ((int)sinfo.hInstApp) < 32)
>+          rv = NS_ERROR_FAILURE;
>+      }

My only slight concern is that if SHPraseDisplayName() returns an error we will return the default NS_OK from this function. But then, the only caller of LoadUriInternal() ignores the return value entirely so I guess it doesn't matter.

Patch works fine.
r=dveditz

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 280086 [details] [diff] [review]
BonEcho SHParseDisplayName patch

approved for 1.8.1.8 and 1.8.0.14, a=dveditz for drivers

We want this on the 1.8.0 branch for the eventual Thunderbird 1.5.0.14

Do you need someone (me?) to check-in for you?

Comment 20

[Jim Mathies [:jimm]]

Yes, please, I don't have cvs access at this point. 

Comment 21

[Daniel Veditz [:dveditz]]

Checked attachment 280086 [details] [diff] [review] into the 1.8 and 1.8.0 branches

Comment 22

[Mats Palmgren (inactive)]

FYI, some Tinderboxes are busted due to this checkin (both branches).
The error is: error C2065: 'SFGAOF' : undeclared identifier
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla1.8/1191441120.1191442677.2381.gz

http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla1.8
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla1.8.0

Comment 23

[Daniel Veditz [:dveditz]]

The Thunderbird machine had the original VC 6 Pro with old headers (1998). The Firefox machine had gotten the 2003 PlatformSDK update and was fine. Added 
  typedef ULONG SFGAOF;
copied from <ShObjIdl.h> to clear things up.

Comment 24

[Stephen Donner [:stephend] Not actively reading bugmail]

Dan / Rob: are the steps to ensuring this fix basically the same as the testcase in https://bugzilla.mozilla.org/show_bug.cgi?id=389580#c38?

Comment 25

[Mike Schroepfer]

Wanted to make sure everyone saw this: http://blogs.technet.com/msrc/archive/2007/10/10/msrc-blog-additional-details-and-background-on-security-advisory-943521.aspx

Comment 26

[Jim Mathies [:jimm]]

Would we still roll the SHParseDisplayName patch into 3.0 if they released this and it addressed the problem? I suppose it adds an extra level of protection, but it's also leveraging code that in an "unknown territory".

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

Under Vista using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8, when I click on the protocols from https://bugzilla.mozilla.org/attachment.cgi?id=274255 registered using Rob's installer at https://bugzilla.mozilla.org/attachment.cgi?id=274232 from bug 389580, I see the following in the Windows shell's console (I used 'Copy link location for the "passed" link, and on telnet it's a bit different pattern'):

Mailto:

passed: mailto:%41%2D%31%5Ftest"ing?%41%31%00.txt
result: mailto:A-1_test%22ing?%41%31%00.txt

News: 

passed: news:%41%2D%31%5Ftest"ing?%41%31%00.txt
result: news:A-1_test%22ing?%41%31%00.txt

Telnet:

passed: telnet:%41%2D%31%5Ftest"ing?%41%31%00.txt
result: telnet:%41%2D%31%5Ftest%22ing?%41%31%00.txt

Not sure why telnet is different, i.e. why it doesn't begin with "A-1_test" as mailto: and news: do.

I've also tested rtsp, itms, mailto:, skype:, call: and telnet: using 'in-the-wild' links, (meaning, working links garnered from google searches/general browsing/test sites), and all seems to still work.  Those I tested on Vista, XP SP2 with IE 7, and XP SP2 with IE 6.

I could still use some help in testing (as always), since I can follow the directions but might not understand the intricacies/nuances of the fix and potential fallout.

Comment 28

[Stephen Donner [:stephend] Not actively reading bugmail]

(BTW, I think "Copy link location" does some sanitizing/presentation of its own; the links I see and the links I paste aren't always the same, just FYI...)

Comment 29

[Daniel Veditz [:dveditz]]

We need to land this on trunk still -- we don't know when MS will actually fix it, whether they're fixing what we think they're fixing, or how fast users will upgrade to the fixed version.

We can always back it out later if we're happy with MS's fix.

Comment 30

[Stephen Donner [:stephend] Not actively reading bugmail]

I verified this as best I could; replacing fixed1.8.1.8 keyword with verified1.8.1.8, based on my testing in comment 27.  Leaving qawanted, though, because there's probably always more testing we can do.

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 283011 [details] [diff] [review]
GranParadiso SHParseDisplayName patch

+      if (SUCCEEDED(pMySHParseDisplayName(NS_ConvertASCIItoUTF16(urlSpec).get(), NULL, &pidl, 0, &sfgao))) {

UTF-8, not ASCII

also, please keep your lines below 80 chars

do we really want the DDEWAIT flag? Couldn't that hang the thread for quite a while?

why pass NO_UI?

+        sinfo.lpVerb   = (LPCSTR)&cmdVerb;

do you need the cast?

Comment 32

[Jim Mathies [:jimm]]

>UTF-8, not ASCII

will do.

> do we really want the DDEWAIT flag? 

I added this due to the assumption that the thread may not be a UI calling thread. The calling thread needs to have a message pump associated with it. If that's not the case, or if the thread exits during the conversation, things can fall apart. Holding the thread temporarily insures the conversation completes.

> why pass NO_UI?

Well, my assumption was that if an error occured, we'd handle it via the return value through our own UI. If we throw something up, and Windows throws something up, that doesn't come off as being cleanly handled. If we don't handle the error return value, imho, we probably should be. I don't have a super-strong opinion on this myself though.

> sinfo.lpVerb   = (LPCSTR)&cmdVerb;
> do you need the cast?

I think it might prevent a warning. I'll try removing it and check for a clean compile.

Comment 33

[Jim Mathies [:jimm]]

Attachment: granparadiso patch

Comment 34

[Jim Mathies [:jimm]]

Attachment: granparadiso patch

Sorry, missed a cvs conflict in the includes. Update attached.

Comment 35

[Reed Loden [:reed]]

Checking in uriloader/exthandler/win/nsMIMEInfoWin.cpp;
/cvsroot/mozilla/uriloader/exthandler/win/nsMIMEInfoWin.cpp,v  <--  nsMIMEInfoWin.cpp
new revision: 1.9; previous revision: 1.8
done

Comment 36

[Bill Gianopoulos [:WG9s]]

This appears to have resulted in bustage on SeaMonkey Tinderbox (sea-win32) and Sunbird Tinderbox (sb-win32).

Comment 37

[Bill Gianopoulos [:WG9s]]

(In reply to comment #36)
> This appears to have resulted in bustage on SeaMonkey Tinderbox (sea-win32) and
> Sunbird Tinderbox (sb-win32).
> 
This looks like the issue mentioned in comment #18.

Comment 38

[Reed Loden [:reed]]

(In reply to comment #36)
> This appears to have resulted in bustage on SeaMonkey Tinderbox (sea-win32) and
> Sunbird Tinderbox (sb-win32).

Checked-in a bustage fix, as per comment #18.

Checking in Makefile.in;
/cvsroot/mozilla/docshell/build/Makefile.in,v  <--  Makefile.in
new revision: 1.49; previous revision: 1.48
done

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #32)
> > do we really want the DDEWAIT flag? 
> 
> I added this due to the assumption that the thread may not be a UI calling
> thread. The calling thread needs to have a message pump associated with it. If
> that's not the case, or if the thread exits during the conversation, things can
> fall apart. Holding the thread temporarily insures the conversation completes.

Well, we don't call this from non-UI threads...

Comment 40

[Jim Mathies [:jimm]]

From the original bug / blog post, it sounds like mailto is the main handler effected by this - 

http://xs-sniper.com/blog/2007/09/01/firefox-file-handling-woes/