bug 384384 was the URI handler command injection vulnerability that made us introduce the -osint commandline option. We didn't fix that for SeaMonkey trunk back then because we knew the branch patch wouldn't apply to trunk and we had at least some parts missing (the Windows shell service, for example) for the new solution - additionally we were far from an alpha or release. As we are nearing alpha and hopefully get the Windows shell service in bug 380347, we should care to implement a full fix on trunk.

CCing Neil and Frank, I guess for the commandline handler(s?) we need a fix like attachment 271754 [details] [diff] [review] (Thunderbird), same for installer.

Microsoft fixed something though in the OS in the meantime, see http://www.microsoft.com/technet/security/bulletin/MS07-061.mspx.

Frank, is this fixed with your shellservice backend work now?

Looks like it is currently not, I think we have to change http://mxr.mozilla.org/comm-central/source/suite/browser/nsBrowserContentHandler.js. I used the test case from Bug 384384 for testing and the console open and JS Console open tests succeeded. I used IE6 on WinXP with full security updates for testing though and the FAQ on the Microsoft website said: "The vulnerability exists in a Windows file, Shell32.dll, included in supported editions of Windows XP and Windows Server 2003. Microsoft has not identified any way to exploit this vulnerability on systems using Internet Explorer 6, which is the browser that is included with these operating systems", so I wonder why this actually worked...?

This is the testcase from the FF bug with s/firefoxurl/seamonkeyurl.

So whatever Microsoft fixed there, this is an attempt at a patch. It's just the code from FF and TB copied together. I do not really want to take this bug because I do not know enough about this code... With the test page I now get an error, but I still see a console window popping up for <1s when clicking on the "Load console here" link. The same seems to happen with the Firefox test case here though.

(In reply to comment #2) > Microsoft fixed something though in the OS in the meantime, see > http://www.microsoft.com/technet/security/bulletin/MS07-061.mspx. Ok, it helps when you don't confuse fixes :-|. That bulletin was about CVE-2007-3896, this bug here is about CVE-2007-3670 (also see http://www.mozilla.org/security/announce/2007/mfsa2007-23.html).

Comment on attachment 337568 [details] [diff] [review] Attempt at a patch Neil, can you take a look at this?

Comment on attachment 337568 [details] [diff] [review] Attempt at a patch > const nsIChannel = Components.interfaces.nsIChannel; > const nsICommandLine = Components.interfaces.nsICommandLine; > const nsICommandLineHandler = Components.interfaces.nsICommandLineHandler; >+const nsICommandLineValidator = Components.interfaces.nsICommandLineValidator; > const nsIComponentRegistrar = Components.interfaces.nsIComponentRegistrar; > const nsICategoryManager = Components.interfaces.nsICategoryManager; > const nsIContentHandler = Components.interfaces.nsIContentHandler; -w diff right? ;-) >+const NS_ERROR_ABORT = Components.results.NS_ERROR_ABORT; > const NS_BINDING_ABORTED = 0x804b0002; > const NS_ERROR_WONT_HANDLE_CONTENT = 0x805d0001; The other consts are only here because they're not in Components.results. >+ var newsFlagIdx = cmdLine.findFlag("news", false); >+ var composeFlagIdx = cmdLine.findFlag("compose", false); How does Thunderbird validate news/compose? Can't we share its validator? > catMan.addCategoryEntry("command-line-handler", "x-default", > BROWSER_CONTRACTID, true, true); >+ catMan.addCategoryEntry("command-line-validator", >+ "b-default", >+ BROWSER_CONTRACTID, true, true); Nit: incorrect wrap/indentation.

Comment on attachment 337568 [details] [diff] [review] Attempt at a patch I see we can't share Thunderbird's code, so forget that. >+ validate: function bch_validate(cmdLine) { Nit: File style is to call this function validate. >+ var osintFlagIdx = cmdLine.findFlag("osint", false); I wonder why thunderbird allows -compose / -news with STATE_REMOTE_EXPLICIT but firefox doesn't allow -url with STATE_REMOTE_EXPLICIT. I'd play safe and check for both early on - only return early if the state is not STATE_REMOTE_EXPLICIT and there is no osint flag. >+ var urlFlagIdx = cmdLine.findFlag("url", false); >+ var newsFlagIdx = cmdLine.findFlag("news", false); >+ var composeFlagIdx = cmdLine.findFlag("compose", false); I'd use a loop of some sort (["url", "news", "compose"].forEach perhaps) >+ var urlParam = cmdLine.getArgument(urlFlagIdx + 1); >+ if (cmdLine.length != urlFlagIdx + 2 || /seamonkeyurl:/.test(urlParam)) { Please inline this variable so that we check the length first. r=me with both sets of nits fixed.

Comment on attachment 337568 [details] [diff] [review] Attempt at a patch a=me for Alpha1, given that it actually fixes the bug description.

Given the security implications, publicity and publicly posted exploits of bug 384384, I think this should block.

Neil, can you take a look at this? I think I all issues you mentioned and implemented the forEach loop...

Comment on attachment 339322 [details] [diff] [review] New patch >+ var osintFlagIdx = cmdLine.findFlag("osint", false); >+ >+ // If the osint flag is not present and we are not called by DDE then we're safe >+ if (osintFlagIdx == -1 && >+ cmdLine.state != nsICommandLine.STATE_REMOTE_EXPLICIT) Nit: cmdLine should line up with osintFlagIdx Nit: inline osintFlagIdx, you only use it once Nit: test cmdLine.state first now it's quicker >+ // Handle Browser URLs Nit: this comment doesn't make sense here >+ var testExpr = new RegExp("seamonkey" + value); Nit: need to test for a trailing colon too

Ok, all nits fixed. Thanks for the reviews. => FIXED

This was changeset cfff34c32e61 BTW.