Closed Bug 508074 Opened 10 years ago Closed 10 years ago

nsBinHexDecoder should use an nsCString for mName

Categories

(Core :: Networking, defect, critical)

ARM
Maemo
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking1.9.1 --- .3+
status1.9.1 --- .3-fixed

People

(Reporter: timeless, Assigned: timeless)

References

()

Details

(Keywords: crash, fixed1.8.1.24, fixed1.9.0.14, Whiteboard: [sg:critical?])

Attachments

(3 files, 4 obsolete files)

http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/nsBinHexDecoder.cpp?mark=206,210,212#184

steps:
1. load http://download.adobe.com/pub/adobe/golive/mac/6.x/GL6.0.1_Mac_Client_Updater.hqx

expected results:
unclear

actual results (on arm):
double free in destructor

184 nsresult nsBinHexDecoder::ProcessNextState(nsIRequest * aRequest, nsISupports * aContext)

204       // c & 63 returns the length of mName. So if we need the length, that's how
205       // you can figure it out...for now we are storing it in the first byte of mName
206       *(mName) = (c & 63);     

this can set a value of up to 63


let mCount = 62

209     case BINHEX_STATE_FNAME:
210       mName[mCount] = c;

here we stomp on values. mCount == 62

212       if (mCount++ > *(mName)) // the first byte of mName is the length...

if 62 > 63 ... no,
mCount = 63

210       mName[mCount] = c;

here we stomp on values. mCount == 63

if 63 > 63 ... no
mCount = 64

210       mName[mCount] = c;

here we stomp on values. mCount == 64
oops, our array is only 0..63

if 64 > 63 ... yes
mCount = 65
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #392291 - Flags: review?(cbiesinger)
Attached patch mq header .... (obsolete) — Splinter Review
Attachment #392291 - Attachment is obsolete: true
Attachment #392292 - Flags: review?(cbiesinger)
Attachment #392291 - Flags: review?(cbiesinger)
Attached patch i need dinnerSplinter Review
Attachment #392292 - Attachment is obsolete: true
Attachment #392294 - Flags: review?(cbiesinger)
Attachment #392292 - Flags: review?(cbiesinger)
Summary: Bad free in nsBinHexDecoder::~nsBinHexDecoder → nsBinHexDecoder should use an nsCString for mName
Attached patch using an nsCString (obsolete) — Splinter Review
this patch requires the patch from bug 508229.
Attachment #392465 - Flags: review?(cbiesinger)
Comment on attachment 392465 [details] [diff] [review]
using an nsCString

+      mName.SetLength(c & 63);
       break;

perhaps check that this succeeded:
  if (mName.Length() != (c & 63)) {
    // report error
  }
Attachment #392465 - Flags: review?(cbiesinger) → review+
Attachment #392294 - Flags: review?(cbiesinger) → review-
Attached patch with basic handling (obsolete) — Splinter Review
so, this is the best i can do short of rewriting a large portion of that code, and biesi thinks it's the better choice for now.

i have a draft which goes toward that goal, but doesn't reach it. note that this code technically violates necko apis, but we're going to live with that (i believe the existing code does too, but in other ways).
Attachment #392465 - Attachment is obsolete: true
Attachment #392719 - Flags: review?(cbiesinger)
Attachment #392719 - Attachment is obsolete: true
Attachment #392720 - Flags: review?(cbiesinger)
Attachment #392719 - Flags: review?(cbiesinger)
Attachment #392720 - Flags: review?(cbiesinger) → review+
http://hg.mozilla.org/mozilla-central/rev/f433451f8c40
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
testcase-wanted -- reduced if possible, but if nothing else capture the URL so we have a test in case the remote resource goes away.

Please request approval for the branches you want to land this on.
blocking1.9.1: --- → ?
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.0.14?
Flags: blocking1.8.1.next?
Flags: blocking1.8.0.next?
Keywords: testcase-wanted
Whiteboard: [sg:critical?]
jmaher, I can save the page but not test that it actually reproduces the problem without a machine. Can you take a look?
Flags: blocking1.9.0.14? → blocking1.9.0.14+
blocking1.9.1: ? → .3+
Attachment #392720 - Flags: approval1.9.1.3?
Attachment #392720 - Flags: approval1.9.0.14?
Comment on attachment 392720 [details] [diff] [review]
with both files....

Approved for 1.9.1.3 and 1.9.0.14, a=dveditz for release-drivers
Attachment #392720 - Flags: approval1.9.1.3?
Attachment #392720 - Flags: approval1.9.1.3+
Attachment #392720 - Flags: approval1.9.0.14?
Attachment #392720 - Flags: approval1.9.0.14+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/194b881050ac

mozilla/netwerk/streamconv/converters/nsBinHexDecoder.cpp 	1.22
mozilla/netwerk/streamconv/converters/nsBinHexDecoder.h 	1.6
Keywords: 4xp
Joel or Aakash, can you verify that this is fixed in 1.9.1 now?
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Depends on: 543497
Checking in netwerk/streamconv/converters/nsBinHexDecoder.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsBinHexDecoder.cpp,v  <--  nsBinHexDecoder.cpp
new revision: 1.20.24.1; previous revision: 1.20
done
Checking in netwerk/streamconv/converters/nsBinHexDecoder.h;
/cvsroot/mozilla/netwerk/streamconv/converters/nsBinHexDecoder.h,v  <--  nsBinHexDecoder.h
new revision: 1.4.28.1; previous revision: 1.4
done
Keywords: fixed1.8.1.24
Group: core-security
You need to log in before you can comment on or make changes to this bug.