Last Comment Bug 307259 - Firefox 1.0.6 buffer overflow with hostname of all soft hyphens [@ nsStringBuffer::Realloc] [@ nsCSubstring::Capacity] [@ nsGenericElement::~nsGenericElement]
: Firefox 1.0.6 buffer overflow with hostname of all soft hyphens [@ nsStringBu...
Status: RESOLVED FIXED
[sg:fix]
: crash, fixed-aviary1.0.7, fixed1.7.12, testcase, verified1.8
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.8beta5
Assigned To: David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch)
:
Mentors:
data:text/html;charset=utf-8,%3Ca%20h...
: 307722 307725 307800 (view as bug list)
Depends on: 307899
Blocks: sbb+ Zalewski
  Show dependency treegraph
 
Reported: 2005-09-06 13:41 PDT by Tom Ferris
Modified: 2011-06-13 10:01 PDT (History)
46 users (show)
dbaron: blocking1.7.12+
dbaron: blocking‑aviary1.0.7+
brendan: blocking1.8b5+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
apple crash report (Gecko 1.8 branch) (23.23 KB, text/plain)
2005-09-06 14:06 PDT, Jesse Ruderman
no flags Details
partially simplified testcase (crashes firefox on gecko 1.8 branch) (1.08 KB, text/html)
2005-09-06 14:44 PDT, Jesse Ruderman
no flags Details
ugly patch to fix trunk crash (3.33 KB, patch)
2005-09-06 14:59 PDT, David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch)
no flags Details | Diff | Splinter Review
Simplified testcase (crashes Firefox on Gecko 1.8 branch and trunk) (61 bytes, text/html)
2005-09-06 15:02 PDT, Jesse Ruderman
no flags Details
corrected hack fix (3.39 KB, patch)
2005-09-09 09:24 PDT, David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch)
no flags Details | Diff | Splinter Review
aviary branch version of hack fix (3.95 KB, patch)
2005-09-09 09:24 PDT, David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch)
no flags Details | Diff | Splinter Review
hack fix (3.36 KB, patch)
2005-09-09 09:36 PDT, David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch)
darin.moz: review+
dveditz: superreview+
mscott: approval1.8b5+
Details | Diff | Splinter Review
aviary branch version of hack fix (3.92 KB, patch)
2005-09-09 09:37 PDT, David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch)
darin.moz: review+
dveditz: superreview+
Details | Diff | Splinter Review
XPI to disable IDN and bump the vender Sub to 1.0.6.1 (573 bytes, application/octet-stream)
2005-09-09 12:28 PDT, Scott MacGregor
no flags Details
untested seamonkey XPI bumps the version to 1.7.11.1 (564 bytes, application/octet-stream)
2005-09-09 12:52 PDT, Scott MacGregor
no flags Details
new xpi for seamonkey and firefox, sets vendorComment instead of changing the version (562 bytes, application/octet-stream)
2005-09-09 14:03 PDT, Scott MacGregor
no flags Details
update xpi to set productComment (564 bytes, application/octet-stream)
2005-09-09 14:38 PDT, Scott MacGregor
no flags Details
updated xpi with file perms 0644 (632 bytes, application/x-xpinstall)
2005-09-10 13:38 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details
belt-and-braces patch (24.02 KB, patch)
2005-09-11 15:51 PDT, David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch)
darin.moz: review+
dveditz: superreview+
asa: approval‑aviary1.0.7+
asa: approval1.7.12+
asa: approval1.8b5+
Details | Diff | Splinter Review
Original testcase from Ferris (24.62 KB, text/html)
2005-09-26 08:03 PDT, Daniel Veditz [:dveditz]
no flags Details

Description Tom Ferris 2005-09-06 13:41:34 PDT
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:
Comment 1 Jesse Ruderman 2005-09-06 14:06:35 PDT
Created attachment 195049 [details]
apple crash report (Gecko 1.8 branch)

Corresponds to TB9093897Y.
Comment 2 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-06 14:44:08 PDT
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.
Comment 3 Jesse Ruderman 2005-09-06 14:44:10 PDT
Created attachment 195056 [details]
partially simplified testcase (crashes firefox on gecko 1.8 branch)
Comment 4 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-06 14:59:14 PDT
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.
Comment 5 Jesse Ruderman 2005-09-06 15:02:55 PDT
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.
Comment 6 Daniel Veditz [:dveditz] 2005-09-06 15:05:30 PDT
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.
Comment 7 Jesse Ruderman 2005-09-06 15:07:06 PDT
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.
Comment 8 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-06 15:17:14 PDT
The net_ToLowerCase call is also a potential buffer overrun here.
Comment 9 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-06 15:30:56 PDT
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.
Comment 10 Daniel Veditz [:dveditz] 2005-09-07 08:04:18 PDT
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.
Comment 11 Jesse Ruderman 2005-09-09 00:14:23 PDT
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.
Comment 12 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-09 00:42:04 PDT
Also http://seclists.org/lists/fulldisclosure/2005/Sep/index.html#233
Comment 13 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-09 00:51:35 PDT
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
Comment 14 Mike Schroepfer 2005-09-09 01:11:07 PDT
With network.enableIDN=true this reproduces easily for me.  With
network.enableIDN=false I do not crash.
Comment 15 Josh Bressers 2005-09-09 08:03:28 PDT
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.
Comment 16 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-09 08:44:32 PDT
Wrong, since there's stuff appended to the buffer after the hostname.
Comment 17 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-09 08:45:19 PDT
*** Bug 307722 has been marked as a duplicate of this bug. ***
Comment 18 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-09 08:54:24 PDT
*** Bug 307725 has been marked as a duplicate of this bug. ***
Comment 19 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-09 09:24:07 PDT
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 20 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-09 09:24:42 PDT
Created attachment 195422 [details] [diff] [review]
aviary branch version of hack fix
Comment 21 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-09 09:28:01 PDT
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);
Comment 22 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-09 09:36:30 PDT
Created attachment 195424 [details] [diff] [review]
hack fix
Comment 23 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-09 09:37:11 PDT
Created attachment 195425 [details] [diff] [review]
aviary branch version of hack fix
Comment 24 Josh Bressers 2005-09-09 10:38:50 PDT
This issue has been assigned the CVE id CAN-2005-2871.
Comment 25 Daniel Veditz [:dveditz] 2005-09-09 11:18:50 PDT
Comment on attachment 195424 [details] [diff] [review]
hack fix

sr=dveditz
This version fixes the remaining memory corruption I was seeing.
Comment 26 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-09 11:25:33 PDT
OK, removing security-confidential flag since everything in this bug is public.
Comment 27 Mike Schroepfer 2005-09-09 11:40:06 PDT
Can we confirm whether enableIDN=false fixes the problem?
Comment 28 Darin Fisher 2005-09-09 12:01:14 PDT
Comment on attachment 195424 [details] [diff] [review]
hack fix

r=darin

nit: insert newline between closing bracket and "else"
Comment 29 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-09 12:04:50 PDT
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 30 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-09 12:08:03 PDT
Comment on attachment 195424 [details] [diff] [review]
hack fix

Checked in to trunk, 2005-09-09 12:06 -0700.
Comment 31 Vectorspace 2005-09-09 12:13:14 PDT
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?
Comment 32 Asa Dotzler [:asa] 2005-09-09 12:20:43 PDT
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.
Comment 33 Darin Fisher 2005-09-09 12:23:36 PDT
Yes, you should be able to set enableIDN to false in about:config to workaround
this bug.
Comment 34 Scott MacGregor 2005-09-09 12:28:58 PDT
Created attachment 195447 [details]
XPI to disable IDN and bump the vender Sub to 1.0.6.1

This XPI installs a new file in the application default prefs directory that
adds the following default preferences:

pref("app.version", "1.0.6.1");
pref("general.useragent.vendorSub", "1.0.6.1");
pref("network.enableIDN", false);
Comment 35 Mike Schroepfer 2005-09-09 12:31:11 PDT
We are confirming for sure the enableIDN=false fixes it and if so will be
posting the workaround shortly.
Comment 36 Mike Schroepfer 2005-09-09 12:43:36 PDT
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 1.0.6.1
   c) Does it prevent the crash as described here.

Thanks!

Mike

Comment 37 Scott MacGregor 2005-09-09 12:52:46 PDT
Created attachment 195452 [details]
untested seamonkey XPI bumps the version to 1.7.11.1
Comment 38 Vectorspace 2005-09-09 12:58:35 PDT
(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 1.0.6.1
>    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/1.0.6.1
> 

Comment 39 Jay Patel [:jay] 2005-09-09 13:02:30 PDT
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).
Comment 40 bugzilla 2005-09-09 13:17:21 PDT
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/1.0.6.1
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; ja-JP; rv:1.7.11) Gecko/20050727
Comment 41 Jay Patel [:jay] 2005-09-09 13:27:51 PDT
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.
Comment 42 Scott MacGregor 2005-09-09 14:03:05 PDT
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.
Comment 43 bugzilla 2005-09-09 14:21:45 PDT
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.");
Comment 44 bugzilla 2005-09-09 14:30:08 PDT
Sorry, "general.useragent.vendorComment" should be a few simple words.
Ignore my comment above.
Comment 45 Vectorspace 2005-09-09 14:30:28 PDT
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
Comment 46 Vectorspace 2005-09-09 14:38:06 PDT
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
Comment 47 Scott MacGregor 2005-09-09 14:38:59 PDT
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.
Comment 48 Antti Näyhä 2005-09-09 14:44:55 PDT
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.
Comment 49 Daniel Veditz [:dveditz] 2005-09-09 14:47:35 PDT
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.
Comment 50 Vectorspace 2005-09-09 14:54:19 PDT
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
Comment 51 Jay Patel [:jay] 2005-09-09 15:00:26 PDT
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.
Comment 52 Marcia Knous [:marcia - use ni] 2005-09-09 15:02:05 PDT
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 53 Marcia Knous [:marcia - use ni] 2005-09-09 15:21:38 PDT
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 54 Scott MacGregor 2005-09-09 16:31:06 PDT
Comment on attachment 195424 [details] [diff] [review]
hack fix

approving for 1.8b5
Comment 55 Daniel Veditz [:dveditz] 2005-09-09 16:31:57 PDT
Comment on attachment 195425 [details] [diff] [review]
aviary branch version of hack fix

sr=dveditz
a=dveditz for aviary and 1.7 branches
Comment 56 Peter van der Woude [:Peter6] 2005-09-09 16:32:06 PDT
*** Bug 307800 has been marked as a duplicate of this bug. ***
Comment 57 Darin Fisher 2005-09-09 16:33:52 PDT
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 1.4.0.1?), we should just take dbaron's patch and not futz with the
enableIDN pref.
Comment 58 Daniel Veditz [:dveditz] 2005-09-09 19:45:04 PDT
(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 1.4.0.1?), 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.
Comment 59 Juha-Matti Laurio 2005-09-09 20:00:36 PDT
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.
Comment 60 Juha-Matti Laurio 2005-09-09 20:04:10 PDT
Information about new http://www.mozilla.org/security/idn.html page spreaded
with Internet Storm Center, MozillaZine etc.
Comment 61 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-10 00:16:01 PDT
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.
Comment 62 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-10 12:53:12 PDT
[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
Comment 63 Robert Strong [:rstrong] (use needinfo to contact me) 2005-09-10 13:38:44 PDT
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.
Comment 64 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-11 15:51:00 PDT
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.
Comment 65 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-11 15:56:01 PDT
The SetQuery and SetRef changes in that patch aren't quite right, though...
Comment 66 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-11 16:13:53 PDT
Actually, I think they're OK, since they match what BuildNormalizedSpec does.
Comment 67 Daniel Veditz [:dveditz] 2005-09-13 00:34:05 PDT
Comment on attachment 195682 [details] [diff] [review]
belt-and-braces patch

sr=dveditz
Comment 68 Alexander Sack 2005-09-13 06:34:48 PDT
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.
Comment 69 Robert Strong [:rstrong] (use needinfo to contact me) 2005-09-13 06:55:22 PDT
It appears that the 307259.xpi on the mirrors is still the one with bad perms
for unix... :/
Comment 70 Darin Fisher 2005-09-13 09:51:57 PDT
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
Comment 71 Worcester12345 2005-09-13 10:15:22 PDT
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.
Comment 72 Dave Miller [:justdave] (justdave@bugzilla.org) 2005-09-13 10:25:53 PDT
(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.
Comment 73 Robert Strong [:rstrong] (use needinfo to contact me) 2005-09-13 10:46:18 PDT
(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://149.174.36.116/pub/mozilla.org/firefox/releases/1.0.6/patches/
Comment 74 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-13 11:32:12 PDT
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
Comment 75 Dave Miller [:justdave] (justdave@bugzilla.org) 2005-09-13 20:27:39 PDT
(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.
Comment 76 Robert Strong [:rstrong] (use needinfo to contact me) 2005-09-13 20:43:28 PDT
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.
Comment 77 Zach Lipton [:zach] 2005-09-13 20:47:30 PDT
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?
Comment 78 Scott MacGregor 2005-09-13 21:02:34 PDT
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. 
Comment 79 Zach Lipton [:zach] 2005-09-13 21:09:15 PDT
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? 
Comment 80 Robert Strong [:rstrong] (use needinfo to contact me) 2005-09-14 12:02:38 PDT
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.
Comment 81 Chase Phillips 2005-09-14 12:20:02 PDT
(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
Comment 84 Brendan Eich [:brendan] 2005-09-14 15:25:56 PDT
(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
Comment 85 Daniel Veditz [:dveditz] 2005-09-14 16:56:12 PDT
(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.

Comment 86 Brendan Eich [:brendan] 2005-09-14 17:00:14 PDT
(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
Comment 87 Bob Clary [:bc:] 2005-09-14 17:25:43 PDT
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.
Comment 88 Daniel Veditz [:dveditz] 2005-09-26 08:03:40 PDT
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.
Comment 89 Bob Clary [:bc:] 2005-11-10 09:23:59 PST
no crash firefox 1.5 rc2 winxp/linux
Comment 90 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2006-02-07 18:17:45 PST
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.
Comment 91 Bob Clary [:bc:] 2009-03-31 13:37:36 PDT
Are crashtests appropriate Is or wanted for this? Would they live in netwerk/base/src/crashtests ?

Note You need to log in before you can comment on or make changes to this bug.