23.23 KB, text/plain
61 bytes, text/html
3.36 KB, patch
Scott MacGregor: approval1.8b5+
|Details | Diff | Splinter Review|
3.92 KB, patch
|Details | Diff | Splinter Review|
564 bytes, application/octet-stream
632 bytes, application/x-xpinstall
24.02 KB, patch
|Details | Diff | Splinter Review|
24.62 KB, text/html
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050416 Epiphany/1.2.8 Build Identifier: I have found a format string vulnerability within Firefox 1.0.6 which allows for arbitrary code execution. Affect Versions: Firefox Linux 1.0.6 Firefox Win32 1.0.6 Deer Park Latest Build Below is a url to reproduce this issue: http://www.security-protocols.com/firefoxwin32-death.html If you need any further information, please let me know. Thanks, Tom Ferris Researcher www.security-protocols.com Key fingerprint = 0DFA 6275 BA05 0380 DD91 34AD C909 A338 D1AF 5D78 Reproducible: Always Steps to Reproduce:
Created attachment 195049 [details] apple crash report (Gecko 1.8 branch) Corresponds to TB9093897Y.
So the reason I crash here on Linux is not a format string vulnerability; it's a buffer overflow. In particular, the presence of a hostname of all dashes (and a rather long one at that) causes the NormalizeIDN call in nsStandardURL::BuildNormalizedSpec to return true but set encHost to the empty string. This means we append 0 to approxLen, but later on append the long string of dashes to the buffer instead.
Created attachment 195056 [details] partially simplified testcase (crashes firefox on gecko 1.8 branch)
Created attachment 195062 [details] [diff] [review] ugly patch to fix trunk crash This fixes the trunk crash for me on Linux. I'm not sure if it's the right fix, though. It may be that a hostname of all dashes should be considered invalid ACE or whatever it is that makes NormalizeIDN think it can decode it, in which case NormalizeIDN should return false in this case. And it might be safer to do that in either case. I'm still a bit puzzled by some other things I saw with the testcase, though -- in particular, that it seemed like an embedded null got into the string somehow.
Created attachment 195063 [details] Simplified testcase (crashes Firefox on Gecko 1.8 branch and trunk) This may or may not be the same crash dbaron was working on.
On windows I get completely different stacks. On 1.0.6 I crash in js_SetProtoOrParent: TB9095338 In Deer Park I crash in operator new, handling a mouse event? TB9094980. The only mouse event I can think of is the one clicking on the testcase link, but it seemed to crash quite some time after that. I'll try with the patch and see.
The testcase in comment 5 doesn't crash when I load it from Bugzilla, but it does crash if I save it to my desktop and load it from there.
The net_ToLowerCase call is also a potential buffer overrun here.
The reason I was confused about the lengths is that it was a long string of soft hyphens (U+00AD), encoded in UTF-8 as (0xC2 0xAD). So it wasn't quite the normalization bug I thought it was, but there's definitely still some sort of bug here.
With the patch I still crash on both trunk and branch with the original testcase, unfortunately in all kinds of different places on corrupted or freed objects making it very hard to tell what's gone wrong.
The host-all-hyphens crash is now public: http://www.security-protocols.com/modules.php?name=News&file=article&sid=2910 I'm not making this bug public immediately because of dveditz' comment 10.
FWIW, the IDN RFCs are: ftp://ftp.rfc-editor.org/in-notes/rfc3490.txt ftp://ftp.rfc-editor.org/in-notes/rfc3491.txt ftp://ftp.rfc-editor.org/in-notes/rfc3492.txt and stringprep is: ftp://ftp.rfc-editor.org/in-notes/rfc3454.txt
With network.enableIDN=true this reproduces easily for me. With network.enableIDN=false I do not crash.
Can anyone verify that this issue ONLY works if the hostname is all -'s? While this will still be a crash, it won't allow arbitrary code execution.
Wrong, since there's stuff appended to the buffer after the hostname.
*** Bug 307722 has been marked as a duplicate of this bug. ***
*** Bug 307725 has been marked as a duplicate of this bug. ***
Created attachment 195421 [details] [diff] [review] corrected hack fix attachment 195062 [details] [diff] [review] wasn't sufficient, as I pointed out in comment 8, and actually had other bugs as well. This ought to be, although it's still not the right long term fix. dveditz, could you retest with this one?
Comment on attachment 195421 [details] [diff] [review] corrected hack fix >+ AppendToBuf(buf, i, encHost.get(), mHost.mLen); >+ i += mHost.mLen; This can be simplified in both patches to: + i = AppendToBuf(buf, i, encHost.get(), mHost.mLen);
Created attachment 195424 [details] [diff] [review] hack fix
This issue has been assigned the CVE id CAN-2005-2871.
Comment on attachment 195424 [details] [diff] [review] hack fix sr=dveditz This version fixes the remaining memory corruption I was seeing.
OK, removing security-confidential flag since everything in this bug is public.
Can we confirm whether enableIDN=false fixes the problem?
Comment on attachment 195424 [details] [diff] [review] hack fix r=darin nit: insert newline between closing bracket and "else"
Looking at the code on the trunk, the "network.enableIDN" pref being false should always avoid the problematic codepath since gIDN will be null.
Comment on attachment 195424 [details] [diff] [review] hack fix Checked in to trunk, 2005-09-09 12:06 -0700.
Would setting network.enableIDN to false be a viable work-around until the bug is fully fixed? If so, given the high rating Secunia gives this, should we be advising people to make that change?
I've tested 1.0.6 with Windows XP, Windows 2000, and Windows 98 and disabling IDN avoids the crash in the URL testcase. We should get a XPI out to toggle this pref.
Yes, you should be able to set enableIDN to false in about:config to workaround this bug.
Created attachment 195447 [details] XPI to disable IDN and bump the vender Sub to 126.96.36.199 This XPI installs a new file in the application default prefs directory that adds the following default preferences: pref("app.version", "188.8.131.52"); pref("general.useragent.vendorSub", "184.108.40.206"); pref("network.enableIDN", false);
We are confirming for sure the enableIDN=false fixes it and if so will be posting the workaround shortly.
We are preparing the announcement to http://www.mozilla.org/security/. Can people here assist in verification of the XPI: a) Does it set enableIDN=false b) Does it bump the about dialog version to 220.127.116.11 c) Does it prevent the crash as described here. Thanks! Mike
(In reply to comment #36) > We are preparing the announcement to http://www.mozilla.org/security/. Can > people here assist in verification of the XPI: > a) Does it set enableIDN=false > b) Does it bump the about dialog version to 18.104.22.168 > c) Does it prevent the crash as described here. > > Thanks! > > Mike > a) yes b) yes c) yes Mozilla/5.0 (Windows; U; Windows NT 5.0; en-GB; rv:1.7.10) Gecko/20050717 Firefox/22.214.171.124 >
I can verify that Scott's xpi prevents the crash from the simplified testcase (when opening the crash.html locally). As Jessa mentioned, the testcase doesn't crash (with the default prefs) if you try to open it from this bug, but it when saved locally. I tested it with Firefox 1.0.6 on Windows XP and Linux (FC 4).
XPI patches verified on both Firefox and Suite. Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.10) Gecko/20050716 Firefox/126.96.36.199 Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; ja-JP; rv:1.7.11) Gecko/20050727
With Firefox 1.5b1 two of the prefs no longer exist: app.version general.useragent.vendorSub Therefore applying a the xpi patch DOES properly set pref("network.enableIDN", false);, but adds and sets the other 2 version related prefs (which are no longer used), so the version bump doesn't happen as expected. If we plan to patch Beta 1, we'll have to figure out what the new version prefs are and create a separate patch.
Created attachment 195467 [details] new xpi for seamonkey and firefox, sets vendorComment instead of changing the version Here's an updated XPI file which doesn't change the version string at all. Instead if just adds a No IDN vendor comment. This allows us to use the same XPI for the suite and for any 1.0.x version of Firefox and the 1.5 beta.
XPI patch v2 looks good. But its comment should be more descriptive. - pref("general.useragent.vendorComment", "No IDN"); + pref("general.useragent.vendorComment", "IDN support is disabled for security reasons. See Bug 307259 for details.");
Sorry, "general.useragent.vendorComment" should be a few simple words. Ignore my comment above.
New patch works for me on 1.0.6 Mozilla/5.0 (Windows; U; Windows NT 5.0; en-GB; rv:1.7.10) Gecko/20050717 Firefox/1.0.6 However on 1.5 Beta 1 it does not seem to show the vendor comment in Help > About, although the modified preference is correct in about:config Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4 Also, if you have the ActiveX plugin installed, that sets a vendor comment of (ax) which (for me) overrode the No IDN vendor commment in 1.0.6
P.S. on Deer Park it did still successfully disable IDN and so did apply the workaround (it survuded the testcase) - I just didn't get the vendor comment
Created attachment 195471 [details] update xpi to set productComment This updated XPI sets the product comment instead of the vendor comment. The vendor comment did not show up on mozilla suite and firefox 1.5b1 because those products don't have vendorSub strings. By moving to the productComment we think the No IDN text will show up on all 3 of those builds.
The latest XPI works for me on FF 1.0.6 / WinXP: enableIDN is properly set to false, the text "(No IDN)" appears after "Gecko/20050716" in the UA string, and the crash is gone.
So we're going to try using the general.useragent.productComment pref. Not ideal because some UA sniffers might not be expecting a comment string between the Gecko and Firefox tokens (they are more tolerant of end garbage), but will work on all Gecko-based browsers. Keep in mind that we will be issuing an actual patched build in the near future, and then this whole comment thing goes away.
Survives testcase and adds No IDN product comment in my installs of both FF 1.0.6 and FF 1.5 Beta 1 Mozilla/5.0 (Windows; U; Windows NT 5.0; en-GB; rv:1.7.10) Gecko/20050717 (No IDN) Firefox/1.0.6 (ax) Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 (No IDN) Firefox/1.4
I have verified the patch from comment #47 works for: FF106: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 (No IDN) Firefox/1.0.6 FF15b1: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 (No IDN) Firefox/1.4 M1711: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.11) Gecko/20050728 (No IDN) All three have their network.enableIDN=false and are crash free.
Looks good on Mac, both FF 1.0.6 and FF 1.5b1. The No IDN text appears in the about firefox and the enable IDN is set to false.
and looks good in the Mozilla suite as well, forgot to add that - Mozilla 1.7.11 Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.11) Gecko/20050727 (No IDN) (In reply to comment #52) > Looks good on Mac, both FF 1.0.6 and FF 1.5b1. The No IDN text appears in the > about firefox and the enable IDN is set to false.
Comment on attachment 195424 [details] [diff] [review] hack fix approving for 1.8b5
Comment on attachment 195425 [details] [diff] [review] aviary branch version of hack fix sr=dveditz a=dveditz for aviary and 1.7 branches
*** Bug 307800 has been marked as a duplicate of this bug. ***
Firefox 1.5 beta 1 should be patched via the new app update system. There's no reason to cut an XPI for it. For the update to 1.5 beta 1 (is it going to be 1.4.1 or 188.8.131.52?), we should just take dbaron's patch and not futz with the enableIDN pref.
(In reply to comment #57) > Firefox 1.5 beta 1 should be patched via the new app update system. There's no > reason to cut an XPI for it. For the update to 1.5 beta 1 (is it going to be > 1.4.1 or 184.108.40.206?), we should just take dbaron's patch and not futz with the > enableIDN pref. 1.5 should and most likely will be patched with the update system (what a great opportunity to test it!), but we're not going to build/test/push over the weekend. For users who see the press over the weekend it's nice to have an easy patch for them. We'll make sure the next version's installers removes the pref file. I know the patch system can remove files... will it fail if the file is not there to remove? that is, some number of users will have the file to be removed, but most will not. If removing a non-existing file causes problems then it's better not to mess with it.
This is SA16764 at http://secunia.com/advisories/16764/ and FrSIRT/ADV-2005-1690 at http://www.frsirt.com/english/advisories/2005/1690 (both rated as Critical). Is there a change to update Alias field to SA16764 etc.
Information about new http://www.mozilla.org/security/idn.html page spreaded with Internet Storm Center, MozillaZine etc.
attachment 195424 [details] [diff] [review] checked in to trunk, 2005-09-09 12:06 -0700. attachment 195425 [details] [diff] [review] checked in to AVIARY_1_0_1_20050124_BRANCH, 2005-09-09 16:33 -0700. attachment 195425 [details] [diff] [review] checked in to MOZILLA_1_7_BRANCH, 2005-09-09 16:34 -0700. attachment 195424 [details] [diff] [review] checked in to MOZILLA_1_8_BRANCH, 2005-09-10 00:12 -0700.
[10 12:33:56] <rob_strong> JAY: looks like the 307259.xpi was packaged with incorrect perms for Linux ... [10 12:39:45] <dbaron> rob_strong, do you mean that the permissions inside the XPI are wrong? [10 12:41:39] <rob_strong> dbaron: yes [10 12:42:16] <rob_strong> dbaron: same as happened last time iirc [10 12:42:54] <dbaron> what are they, and what should they be? [10 12:43:06] <rob_strong> they are 400 [10 12:43:29] <rob_strong> They should be +r for all [10 12:45:05] <rob_strong> Here is a handy py script for verifying http://biomikro.vscht.cz/maldiman/hassmanm/projects/testzip.py.txt [10 12:51:43] <dbaron> mind if I paste this in the bug? [10 12:52:26] <rob_strong> Not at all
Created attachment 195573 [details] updated xpi with file perms 0644 for what it is worth the files in this xpi are the same as on the mirrors but have been packaged with perms set to 0644.
Created attachment 195682 [details] [diff] [review] belt-and-braces patch Rather than figure out whether we really need this, I propose we take this as well. I also changed two function names that are dangerously unclear.
The SetQuery and SetRef changes in that patch aren't quite right, though...
Actually, I think they're OK, since they match what BuildNormalizedSpec does.
Comment on attachment 195682 [details] [diff] [review] belt-and-braces patch sr=dveditz
Does the belt-and-braces patch contribute to fix this specific problem or is this just a preventive measure like the name 'belt-and-braces' suggests? If so, I would rather suggest to not send this patch to the aviary branch. Would be great to keep the changeset minimal for the stable branch.
It appears that the 307259.xpi on the mirrors is still the one with bad perms for unix... :/
Comment on attachment 195682 [details] [diff] [review] belt-and-braces patch It almost seems to me that a small helper class that combines a string object with a boolean flag would be nice here. >Index: nsStandardURL.cpp >+ EncodeSegmentCount(str.BeginReading(text), URLSegment(0, str.Length()), mask, result, encoded); >+ if (encoded) > return result; > else > return str; > } I know this isn't your code, but if you would, please change that to this: return encoded ? result : str; Or, at least remove the unnecessary 'else'. thanks! r=darin
Will this bug alone cause a 1.0.7 release? Or will there be a waiting period in case there are more? I see it is marked blocking, but how urgent is it? Thanks for the quick fix.
(In reply to comment #69) > It appears that the 307259.xpi on the mirrors is still the one with bad perms > for unix... :/ ftp-chi.osuosl.org was the odd-one-out when I checked all the md5sums. Everyone else had the correct version. I contacted the admin, and it's probably been fixed already by the time I'm typing this.
(In reply to comment #72) > ftp-chi.osuosl.org was the odd-one-out when I checked all the md5sums. Everyone > else had the correct version. I contacted the admin, and it's probably been > fixed already by the time I'm typing this. Are you sure? I checked on a couple of the mirrors and they aren't fixed for me. I also checked using the python script ( http://biomikro.vscht.cz/maldiman/hassmanm/projects/testzip.py.txt ) which shows they are packaged with 400 perms and the dates are from the 9th which is before the permissions issue was brought up on 20050910. http://mozilla.ussg.indiana.edu/pub/mozilla.org/firefox/releases/1.0.6/patches/ http://220.127.116.11/pub/mozilla.org/firefox/releases/1.0.6/patches/
attachment 195682 [details] [diff] [review] checked in: * to trunk, 2005-09-13 10:38 -0700 * to MOZILLA_1_8_BRANCH, 2005-09-13 11:23 -0700 attachment 195682 [details] [diff] [review] excluding the renames of functions (which didn't exist on the old branches) checked in: * to AVIARY_1_0_1_20050124_BRANCH, 2005-09-13 11:23 -0700 * to MOZILLA_1_7_BRANCH, 2005-09-13 11:24 -0700
(In reply to comment #73) > (In reply to comment #72) > > ftp-chi.osuosl.org was the odd-one-out when I checked all the md5sums. > > Everyone else had the correct version. I contacted the admin, and it's > > probably been fixed already by the time I'm typing this. > Are you sure? I checked on a couple of the mirrors and they aren't fixed for > me. All 17 servers match md5sums now, and all have the same md5sum as stage. If it's wrong, then they're all wrong, and the correct xpi apparently hasn't been uploaded yet.
12 years ago
Correct... at least now the xpi with the incorrect file permissions for UNIX is properly distributed to all the mirrors. :P I verified using the py script and by installing the patch that it does indeed have no access set for everyone except root when installed as root so the patch is not applied to anyone but the root account (e.g. no access means it isn't read by the pref system since it can't access it). See bug 189905 for details as to the underlying details.
Well that's not good at all. dbaron: if you can take a quick look at this and make sure that Rob's update works as expected, I can push it to the ftp site. Do we need to go further with this, or just post the xpi with correct permissions and push on to 1.0.7?
did someone actually push the xpi with permission bits set for linux over the weekend or did the xpi just get posted to this bug? I'm more than happy to upload the new version. I thought someone had already done so.
According to stage, the xpi hasn't been modified since September 9th (Rob's patch was posted on the 10th for those following along at home). This seems like another one of those "someone will get it" situations... Scott: are you going to post the xpi, or should I go ahead and do it?
If there is anything I can do to help just ask... also, it might not be such a bad idea to run the python script on any future xpi based patches to verify the perms. This is the second time this has happened out of the two or three times xpi patches have been pushed in the last year.
(In reply to comment #77) > Well that's not good at all. dbaron: if you can take a quick look at this and > make sure that Rob's update works as expected, I can push it to the ftp site. Do > we need to go further with this, or just post the xpi with correct permissions > and push on to 1.0.7? I've pushed dbaron's updated XPI to the FTP site. Here are the MD5 checksums for the 307259.xpi file: Old 307259.xpi: d3d1c4d2dac9b90143574ca7e2fa8330 New 307259.xpi: c58d917c5dab7bbd18ec9807485cb7d4
(In reply to comment #83) > FYI: > > http://security-protocols.com/modules.php?name=News&file=article&sid=2920(In > reply to comment #82) > > FYI: > > > > http://security-protocols.com/modules.php?name=News&file=article&sid=2920 > > almost forget the PoC: > > http://www.security-protocols.com/deerpark-death.html What does that crash bug (in 1.5b1 only) have to do with this bug? /be
(In reply to comment #82) > FYI: > > http://security-protocols.com/modules.php?name=News&file=article&sid=2920 Filed bug 308579 on the new crash. It's a completely unrelated null dereference despite the superficial similarity in the testcase.
(In reply to comment #85) > (In reply to comment #82) > > FYI: > > > > http://security-protocols.com/modules.php?name=News&file=article&sid=2920 > > Filed bug 308579 on the new crash. It's a completely unrelated null dereference > despite the superficial similarity in the testcase. Tom Ferris: your public statements on this matter of fact are incorrect and misleading. What are you going to do about that? /be
ff 1.0.6/winxp: URL->crash, simplified->no crash, firefoxwin32-death->crash ff 1.0.7/winxp; ff 1.5/winxp (20050914) no crash.
Created attachment 197418 [details] Original testcase from Ferris The contents at the testcase link in comment 0 have been changed to Jesse's simplified testcase from comment 5. Just for the record this is the original testcase from Tom Ferris.
no crash firefox 1.5 rc2 winxp/linux
Comment on attachment 195425 [details] [diff] [review] aviary branch version of hack fix Clearing aviary1.0.8/1.7.13 approval flags on this attachment because attachment 195682 [details] [diff] [review] is already in.
Are crashtests appropriate Is or wanted for this? Would they live in netwerk/base/src/crashtests ?