Closed Bug 389613 Opened 17 years ago Closed 17 years ago

back-port -osint logic from bug 384384 to 1.8.0 branch

Categories

(Thunderbird :: Security, defect)

Product:
Thunderbird
Component:
Security
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dveditz, Assigned: mscott)

References

Details

(Keywords: verified1.8.0.13)

Attachments

(2 files, 1 obsolete file)

updated patch
26.28 KB, patch
robert.strong.bugs
: review+
dveditz
: approval1.8.0.13+
Details | Diff | Splinter Review
brute force fix
1.35 KB, patch
Details | Diff | Splinter Review 
bug 384384 protected against potentially malicious command-line arguments when used as a protocol handler by adding logic to check for a -osint parameter and ignoring all but the expected options if present.

This needs to land on the 1.8.0 branch for Thunderbird 1.5.0.x

Given the size of the bug and number of attachments this seemed less likely to get lost in the shuffle as a separate back-port bug.
Flags: blocking1.8.0.13+
cc'ing rob strong to so I can keep him in the loop on what I'm doing.

Also, it looks like the test case from here:

https://bugzilla.mozilla.org/show_bug.cgi?id=384384#c14

is no longer available.
Hey Scott, there is an extended testcase from that page in bug 384384 attachment #269305 [details] - also, dveditz at one time had made some testcases for Thunderbird. I also believe the download available at http://larholm.com/2007/07/25/mozilla-protocol-abuse/ provides a testcase for this
Hmm, so far when I load a local HTML file that looks like the following in IE or Firefox 2.0.0.5:

<html><body>

<iframe src='mailto:test|"%20--install-global-extension%2520C:cmd.xpi"'></iframe>

</body></html>

I get a compose window in 1.5.0.12 with the To Field containing the extra arguments:

test|" --install-global-extension%20C:cmd.xpi"

I get similar results for the other tests listed here:

http://www.xs-sniper.com/sniperscope/IE-Pwns-Firefox.html

assuming I change the protocol from firefoxurl to mailto.
Behavior may be different depending on whether IE7 is installed or not on the system
Hmm, I am using IE 7 on Vista
The end result args on Vista IE7 are different than XP IE7 and are different than XP IE6. I think the easiest way to reproduce is with Firefox 2.0.0.5 and the following from http://larholm.com/2007/07/25/mozilla-protocol-abuse/ - be sure to close Thunderbird 1.5.0.x before trying to attempt. You can also replace the UNC path with a local path to an xpi. I haven't verified any of this myself so it may not be 100% correct.

mailto://me@nowhere.com" -install-global-extension \\127.0.0.1\shared\remote.xpi


Also, Thunderbird 1.5.0.x doesn't have all of the reg keys or cleanup the reg keys from a newer install where Thunderbird has been made default via the new association in the registry so that is another variable. For this XP may be easier to duplicate on
Attached patch the port (obsolete) — — Splinter Review 
This ports Rob's patch but I haven't been able to test it yet as I'm not able to trigger a test case yet. I'll keep poking at it.

Note: I couldn't find any browser installer files (i.e. browser.jst) that wrote protocol keys to the registry on the 1.8.0.x branch so I didn't port the NSIS equivalent changes in shared.nsh
(In reply to comment #4)
> src='mailto:test|"%20--install-global-extension%2520C:cmd.xpi"'></iframe>
> 
> I get a compose window in 1.5.0.12 with the To Field containing the extra
> arguments:
> 
> test|" --install-global-extension%20C:cmd.xpi"

You can't have encoded spaces, to fool the command-line parser into treating these as separate arguments you need a dquote followed by a real, un-encoded space.

> I get similar results for the other tests listed here:
> http://www.xs-sniper.com/sniperscope/IE-Pwns-Firefox.html
> assuming I change the protocol from firefoxurl to mailto.

First make sure Thunderbird is not running, otherwise windows uses DDE and you won't see any problems. You won't be able to test mailto: in particular from IE (at least not IE7, not sure about IE6). IE does encode characters for "web" protocols like mailto: so you won't get the required dquote-space.

Adding support for -osint is to protect against any other other potential callers who don't escape, Like Firefox 2.0.0.5 and lower, probably AIM and any IRC client, etc.

(In reply to comment #9)
> Note: I couldn't find any browser installer files (i.e. browser.jst) that wrote
> protocol keys to the registry on the 1.8.0.x branch so I didn't port the NSIS
> equivalent changes in shared.nsh

It's done generically at
http://lxr.mozilla.org/mozilla1.8.0/source/xpfe/components/winhooks/nsWindowsHooksUtil.cpp#180

called from
http://lxr.mozilla.org/mozilla1.8.0/source/xpfe/components/winhooks/nsWindowsHooks.cpp#88

there too. I don't know which one is active, they should both be patched.
Here's my mail test case based on the cmdxpi.html test on http://larholm.com/2007/07/25/mozilla-protocol-abuse/:

<html><body>

<iframe src='mailto://me@nowhere.com" -install-global-extension c:\cmd.xpi'></iframe>

</body></html>

On Windows XP with Thunderbird not running...

Firefox 2.0.0.4 & Thunderbird 2.0.0.0 --> I am able to reproduce the problem. No compose window, but the next time I start up Thunderbird the xpi has been installed.
Firefox 2.0.0.4 & Thunderbird 1.5.0.12 --> I keep getting a mail compose window that has the text: //me"@nowhere.com -install-global-extension c:cmd.xpi in the to field.
Firefox 2.0.0.4 & Thunderbird 2.0.0.5 --> Nothing happens. No compose window in Thunderbird, and the xpi is not installed.

Each version of Thunderbird was uninstalled before installing the next version.

I suspect my use of a local path for the xpi could be part of my problem with reproducing this in 1.5.0.12. 
Try -ProfileManager or another startup command line flag instead of -install-global-extension

using 1.5.0.12, -ProfileManager also results in a compose window that looks like:

//me"@nowhere.com -ProfileManager

I believe this is because 1.5.0.x doesn't always quote arguments in the registry keys it writes. For instance we used:

mailto\shell\open\command <path to exe>\thunderbird.exe -compose %1

in 1.5.0.12

In fact, changing my test case to:

<iframe src='mailto://me@nowhere.com -ProfileManager'></iframe> (note I removed the " after nowhere.com)

and now I see the Thunderbird profile manager come up when I load the test case in Firefox.

(In reply to comment #13)
> there too. I don't know which one is active, they should both be patched.
> 

FYI, nsWindowsHooksUtils.cpp is not compiled by Firefox on the 1.8.0 branch. 
I talked with someone on IRC (mcsmurf) a couple of days ago about porting this for SeaMonkey to 1.8.0. Just changing that one file won't provide any value without also updating their command line handlers and startup so I'd just let them take care of that backport
So far this patch is working quite well for me on Win XP with Thunderbird. I'm running the test cases from Firefox 2.0.0.4. 

I've also verified by manually inspecting the registry that the keys for mailto, nntp, news and snews and feed are all properly getting osint added correctly. The one think I haven't tested yet are the changes to the installer (mail.jst).
Comment on attachment 273923 [details] [diff] [review]
the port

I think this patch is ready for review. 

Although I still haven't tested the mail installer script changes.

I included the Firefox changes as well.
Attachment #273923 - Attachment description: untested WIP → the port
Attachment #273923 - Flags: review?(robert.bugzilla)
Note: We're going to need a check in the regexp for Firefox.URL in the command line validator.
Comment on attachment 273923 [details] [diff] [review]
the port

>Index: browser/components/nsBrowserContentHandler.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/nsBrowserContentHandler.js,v
>retrieving revision 1.12.2.5.2.4
>diff -u -w -r1.12.2.5.2.4 nsBrowserContentHandler.js
>--- browser/components/nsBrowserContentHandler.js	16 Jan 2007 22:39:43 -0000	1.12.2.5.2.4
>+++ browser/components/nsBrowserContentHandler.js	26 Jul 2007 05:59:45 -0000
>@@ -477,6 +479,21 @@
>     request.cancel(NS_BINDING_ABORTED);
>   },
> 
>+  /* nsICommandLineValidator */
>+  validate : function bch_validate(cmdLine) {
>+    // Other handlers may use osint so only handle the osint flag if the url
>+    // flag is also present and the command line is valid.
>+    var osintFlagIdx = cmdLine.findFlag("osint", false);
>+    var urlFlagIdx = cmdLine.findFlag("url", false);
>+    if (urlFlagIdx > -1 && (osintFlagIdx > -1 ||
>+        cmdLine.state == nsICommandLine.STATE_REMOTE_EXPLICIT)) {
>+      var urlParam = cmdLine.getArgument(urlFlagIdx + 1);
>+      if (cmdLine.length != urlFlagIdx + 2 || /firefoxurl:/.test(urlParam))
I believe the following regexp should cover Firefox.URL... could you verify if this is correct?
if (cmdLine.length != urlFlagIdx + 2 || /firefox\.?xurl:/.test(urlParam))

I'll review the remainder shortly
bah... that should be
if (cmdLine.length != urlFlagIdx + 2 || /firefox\.?url:/.test(urlParam))

Comment on attachment 273923 [details] [diff] [review]
the port

>Index: mailnews/mapi/mapihook/src/nsMapiRegistryUtils.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/mapi/mapihook/src/nsMapiRegistryUtils.cpp,v
>retrieving revision 1.29
>diff -u -w -r1.29 nsMapiRegistryUtils.cpp
>--- mailnews/mapi/mapihook/src/nsMapiRegistryUtils.cpp	14 Jul 2005 20:03:04 -0000	1.29
>+++ mailnews/mapi/mapihook/src/nsMapiRegistryUtils.cpp	26 Jul 2007 05:59:48 -0000
>@@ -760,7 +760,7 @@
>         SetRegistryKey(HKEY_LOCAL_MACHINE, keyName.get(), "DLLPath", (char *)dllPath.get());
>         
>         // (3) now that we have added Software\\Clients\\Mail\\<app name> add subkeys for each protocol mail supports
>-        setupDefaultProtocolKey(nsCString(keyName + NS_LITERAL_CSTRING("\\Protocols\\")).get(), "mailto", "URL:MailTo Protocol", "-compose");
>+        setupDefaultProtocolKey(nsCString(keyName + NS_LITERAL_CSTRING("\\Protocols\\")).get(), "mailto", "URL:MailTo Protocol", "-osint -compose");
> 
> 
>         setupFileExtension("Software\\Classes\\", ".eml");
>@@ -863,9 +863,9 @@
>             // (3) now that we have added Software\\Clients\\News\\<app name>
>             //     add subkeys for each protocol news supports
>             nsCString protocolKeyName(keyName + NS_LITERAL_CSTRING("\\Protocols\\"));
>-            setupDefaultProtocolKey(protocolKeyName.get(), "news", "URL:News Protocol", "-mail");
>-            setupDefaultProtocolKey(protocolKeyName.get(), "nntp", "URL:NNTP Protocol", "-mail");
>-            setupDefaultProtocolKey(protocolKeyName.get(), "snews", "URL:Snews Protocol", "-mail");
>+            setupDefaultProtocolKey(protocolKeyName.get(), "news", "URL:News Protocol", "-osint -mail");
>+            setupDefaultProtocolKey(protocolKeyName.get(), "nntp", "URL:NNTP Protocol", "-osint -mail");
>+            setupDefaultProtocolKey(protocolKeyName.get(), "snews", "URL:Snews Protocol", "-osint -mail");
> 
>             // (4) Software\Clients\News\<app name>\shell\open\command value 
>             nsCAutoString appKeyName;
>@@ -943,7 +943,7 @@
> 
> nsresult nsMapiRegistryUtils::setDefaultFeedClient()
> {
>-    return setupDefaultProtocolKey("Software\\Classes\\", "feed", "URL:Feed Protocol", "-mail");
>+    return setupDefaultProtocolKey("Software\\Classes\\", "feed", "URL:Feed Protocol", "-osint -mail");
> }
> 
> nsresult nsMapiRegistryUtils::unsetDefaultFeedClient()
Is SeaMonkey OK with this change?
Comment on attachment 273923 [details] [diff] [review]
the port

minusing since the changes to nsMailDefaultHandler.js aren't in this patch though most everything else looks good. Could you address comment #22 (revised in comment #23) and comment #24 then resubmit with the changes to nsMailDefaultHandler.js?
Attachment #273923 - Flags: review?(robert.bugzilla) → review-
(In reply to comment #24)
> Is SeaMonkey OK with this change?

You need to ask Neil about that... We're currently dealing with those things in bug 389257
(In reply to comment #26)
>(In reply to comment #24)
>>Is SeaMonkey OK with this change?
>You need to ask Neil about that... We're currently dealing with those things in
>bug 389257
My changes for SeaMonkey 1.1.4 (based on 1.8.1 branch) are slightly different, you might prefer to use them instead:
Index: mailnews/mapi/mapihook/src/nsMapiRegistryUtils.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/mapi/mapihook/src/nsMapiRegistryUtils.cpp,v
retrieving revision 1.29.2.4
diff -u -r1.29.2.4 nsMapiRegistryUtils.cpp
--- mailnews/mapi/mapihook/src/nsMapiRegistryUtils.cpp	25 Jan 2007 23:43:29 -0000	1.29.2.4
+++ mailnews/mapi/mapihook/src/nsMapiRegistryUtils.cpp	26 Jul 2007 23:01:25 -0000
@@ -691,14 +691,9 @@
       // Protocols\<protocol scheme>\shell\open\command value
       nsCAutoString appPath (thisApplication());
 
-      appPath += " ";
-      if (aCmdLineText) 
-      {
-        appPath += aCmdLineText;
-        appPath += " ";
-      }
-      
-      appPath += "%1";
+      appPath += " -osint ";
+      appPath += aCmdLineText;
+      appPath += " \"%1\"";
       nsCAutoString shellOpenKey (keyName);
       shellOpenKey.AppendLiteral("\\shell\\open\\command");
 
As far as I can tell aCmdLineText is never null.
Hey Scott, since this won't have the fix you landed for removing Mozilla Thunderbird.Url.Mailto you'll need to account for that as well

(In reply to comment #28)
> Hey Scott, since this won't have the fix you landed for removing Mozilla
> Thunderbird.Url.Mailto you'll need to account for that as well
Actuakky, that is probably already handled by the regexp
sorry, I forgot to diff against nsMailDefaultHandler.js when I made the patch. 

I'll add the regexp code for the shims too.

Neil's patch adds a quote around the %1 where before we weren't quoting it (on the 1.8.0.x branch). Neil, does this change cause any issues with detecting default client status? We used to not do a good job of truncating the right stuff out of the reg key before doing the comparisons. if that doesn't break, then we can incorporate  Neil's patch.
(In reply to comment #30)
>Neil, does this change cause any issues with detecting default client status?
I don't think so, I think the default client only tests the application path.
Attached patch updated patch — — Splinter Review 
This patch includes feedback from Rob and Neil:
* Includes the nsMailDefaultHandler changes
* incorporates Neil's patch to nsMapiRegistryUtils.cpp
* adjusts the regular expression in nsBrowserContentHandler to account for Firefox.URL
Attachment #273923 - Attachment is obsolete: true
Comment on attachment 274393 [details] [diff] [review]
updated patch

I believe I've addressed all of your comments Rob. Thanks for the review!
Attachment #274393 - Flags: review?(robert.bugzilla)
Comment on attachment 274393 [details] [diff] [review]
updated patch

Patch look good and r=me as long as you verified this doesn't break checking if the client is default.
Attachment #274393 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 274393 [details] [diff] [review]
updated patch

We think this patch is ready for the branch.
Attachment #274393 - Flags: approval1.8.0.13?
Comment on attachment 274393 [details] [diff] [review]
updated patch

approved for 1.8.0.13, a=dveditz for release-drivers
Attachment #274393 - Flags: approval1.8.0.13? → approval1.8.0.13+
Neil, are you ok with me making the change to nsMapiRegistryUtils.cpp which effects seamonkey on the 1.8.0.x branch?
(In reply to comment #37)
>Neil, are you ok with me making the change to nsMapiRegistryUtils.cpp which
>effects seamonkey on the 1.8.0.x branch?
We're not planning on any further releases from that branch, but if we were we'd probably want that change checked in anyway.
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: fixed1.8.0.13
Resolution: --- → FIXED
Is it good enough to verify that I did the following:

1) Downloaded the plaxotbird.xpi toolbar XPI
2) Dropped plaxotbird.xpi into top-level C: on Windows XP SP2
3) Opened Tools | Options | Main, and in the start page textfield, entered the following:

mailto:test|"%20--install-global-extension%2520C:plaxotbird.xpi

4) Chose Go | Mail Start Page

What happens next is that Mail compose launches, with the following in its "To:" addressing widget:

test|" --install-global-extension%20C:plaxotbird.xpi"

I briefly consulted w/biesi, and he said the main protection was that the XPI wouldn't be attempted to install.

Please let me know whether this is appropriate coverage; thanks!
All right; I used Scott's HTML testcase from comment 16, loading it inside of IE 7 and it resulted in launching Thunderbird 1.5.0.13 rc1 (version 1.5.0.13 (20070809)) with the following in the compose window's To: widget:

me"@nowhere.com -ProfileManager

Scott said that this was good enough to verify that we didn't launch the -Profilemanager argument.

Replacing fixed1.8.0.13 keyword with verified1.8.0.13
Keywords: fixed1.8.0.13 → verified1.8.0.13
(In reply to comment #40)
> me"@nowhere.com -ProfileManager

Did you first verify that that "worked" with a vulnerable version? In my Firefox tests I generally had to leave a space after the double-quote for it to be picked up as an argument delimiter.
(In reply to comment #41)
> (In reply to comment #40)
> > me"@nowhere.com -ProfileManager
> 
> Did you first verify that that "worked" with a vulnerable version? In my
> Firefox tests I generally had to leave a space after the double-quote for it to
> be picked up as an argument delimiter.

I can't get the profile manager to launch using the old 1.5.0.12 build of Thunderbird, even when running mscott's last testcase in comment 16, and that worries me, since I've "verified" this as FIXED in 1.5.0.13...of course, I still can't reproduce the exploit (since no profile manager launches in the 1.5.0.13 rc1 builds of Thunderbird on Windows XP SP2), but that's not much of a consolation, given that others have been able to reproduce the exploit.

I'm running in a Virtual Machine image of XP SP2, but its security updates only go up to July 17th, and don't look related to this.

I changed <iframe src='mailto://me@nowhere.com -ProfileManager'></iframe> to <iframe src='mailto://me@nowhere.com" -ProfileManager'></iframe>, and still no dice...
Hey Steve, put this HTML in a test file and load it in IE. 

<html><body>

<iframe src='mailto://me@nowhere.com -ProfileManager'></iframe>

</body></html>

I've just re-verified that using 1.5.0.12, I get the profile manager. Using 1.5.0.13, no profile manager :)


(In reply to comment #43)
> Hey Steve, put this HTML in a test file and load it in IE.

Which version of IE, on which OS?

> <html><body>
> 
> <iframe src='mailto://me@nowhere.com -ProfileManager'></iframe>
> 
> </body></html>

That's what I've been trying in my clean VM, to no avail.  I didn't want to use the tainted lab machines, either.

> I've just re-verified that using 1.5.0.12, I get the profile manager. Using
> 1.5.0.13, no profile manager :)

Something's screwed up on the VM, I'm sure.  Hopefully we'll get this sorted out tomorrow.

And actually, since you've just retested, can't we take your word for it?  I'm not sure what the complete sign-off on this bug is...
Stephen identified a problem with this fix. On the 1.5.0.x branch, we compare the application path and nothing else when determining if the app is still the default. Thus, a user who is running 1.5.0.12 as the default and gets upgraded to 1.5.0.13, won't get the osint flag written out to the registry for the appropriate keys. You need to unset, then set Thunderbird as the default mail client for the keys to get written out.

This isn't a problem when testing:

1.5.0.12 and 1.5.0.13 using clean VMs for each test.
1.5.0.12 and a debug 1.5.0.13 with the fix.

Note: the behavior of detecting default client status based on the application path and not the command line arguments as well has changed on the 1.8.1 branch so this isn't an issue there. It's specific to 1.8.0.

I think this is the broken code for checking default status based on the path of the exe:

http://mxr.mozilla.org/mozilla1.8.0/source/mailnews/mapi/mapihook/src/nsMapiRegistryUtils.cpp#311
Attached patch brute force fix — — Splinter Review 
Here's a fix that seems to address the spot where the default client status isn't changing, causing us to fail to write out the osint flags.

It's a brute force approach for sure, but this is for the 1.8.0 branch only, and this approach has a lot less risk than modifying the different ways in which we detect default app status for mailto, news, nntp, snews and feed urls.

Every time 1.5.0.x starts up, for each protocol the current application instance is the default for, re-write out the registration keys. And re-write out the protocol key giving us default status with the OS.
I temporarily landed this brute force fix on the branch so we can test it with software update on the nightly channel. i'll back it back out again in the morning.
Here's what I did to test the brute force patch:

(either a clean VM or make sure the mailto, nntp, news, feed and snews keys don't have -osint already in the registry)

1) Downloaded a nightly 1.5.0.13pre build from before the original fix went in like this one: ftp://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2007-07-31-06-mozilla1.8.0/thunderbird-1.5.0.13pre.en-US.win32.installer.exe

2) Make sure this installation is the default mail, news and feed client.

3) Verify that the test case still fails and the profile manager comes up.

4) Now check for updates and get updated to the latest update.

5) Restart to install the update

6) Quit thunderbird

7) you can manually inspect HKLM\mailto, HKLM\news, etc. to see that the -osint is now written out into the registry.

8) Run the test case again and verify that nothing comes up.

I can *finally*, *truly* verify this bug as fixed, using Scott's instructions in comment 49, above.

C:\PROGRA~1\MOZILL~1\THUNDE~1.EXE -osint -compose %1 gets set under:

My Computer\HKEY_LOCAL_MACHINE\SOFTWARE\Clients\Mail\Mozilla Thunderbird\protocols\mailto\shell\open\command

Using the pre-fix build (2007-07-31-06 of mozilla1.8.0), I see the Profile Manager invoked when running the testcase in comment 43.  Post-fix, after upgrading to 2007-08-26-04, I can see that not even the compose window comes up, which I think is the expected outcome given Scott's step #8 in comment 49.

Please do let me know if I've foobar'd anything; I trust I haven't.

Leaving verified1.8.0.13 since I never cleared it earlier :-(
So we can better track the specific open issue with software update, I've spun up a new bug, Bug 393823. I've pasted the comments stephen and I have been making since Friday including the brute force patch over to that bug.
Blocks: 393823
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: