Last Comment Bug 692817 - nsA(C)String::SetLength overflow in EncodeInputStream
: nsA(C)String::SetLength overflow in EncodeInputStream
Status: RESOLVED FIXED
[sg:critical][qa-]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Benjamin Smedberg [:bsmedberg]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-07 09:32 PDT by Honza Bambas (:mayhemer)
Modified: 2015-10-16 11:38 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
-
wontfix
+
fixed
+
fixed
+
fixed
10+
fixed
unaffected


Attachments
Use 64-bit math and check for overflow, rev. 1 (946 bytes, patch)
2011-11-25 13:45 PST, Benjamin Smedberg [:bsmedberg]
dbaron: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Honza Bambas (:mayhemer) 2011-10-07 09:32:16 PDT
http://hg.mozilla.org/mozilla-central/annotate/bd1411e362fb/xpcom/io/Base64.cpp#l198

Result of |(aCount + 2) / 3 * 4 + aOffset| may overflow based on the argument aCount and aOffset.  The resulting buffer size result could be truncated and we'll write (potentially attacker controlled content) past the buffer.

Reporting as unconfirmed since I'm not sure there is somewhere an overflow check, but doesn't seems to be, and also not sure this is easily exploitable.

I can try to create a test case.
Comment 1 Honza Bambas (:mayhemer) 2011-10-07 09:37:17 PDT
All potential consumers:
http://mxr.mozilla.org/mozilla-central/ident?i=Base64EncodeInputStream
Comment 2 Daniel Veditz [:dveditz] 2011-10-13 13:15:09 PDT
Clearing sg:critical to trigger triage while we confirm a problem here
Comment 3 Daniel Veditz [:dveditz] 2011-11-16 16:13:07 PST
This has been sg:critical in the past (we had a case in a user of the nspr base64 implementation) and we should just fix it.
Comment 4 Benjamin Smedberg [:bsmedberg] 2011-11-25 13:45:48 PST
Created attachment 576997 [details] [diff] [review]
Use 64-bit math and check for overflow, rev. 1

I don't know how to write an automated test for this without allocating 2G of memory, but suggestions welcome.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-11-25 14:21:16 PST
Comment on attachment 576997 [details] [diff] [review]
Use 64-bit math and check for overflow, rev. 1

r=dbaron, though it seems like it's probably better to have PRUint64(aCount), i.e.:

PRUint64 countlong = (PRUint64(aCount) + 2) / 3 * 4;

in case aCount itself is within 2 of PR_UINT32_MAX.
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2011-12-08 14:02:43 PST
bsmedberg, can we get this landed on m-c?
Comment 7 Alex Keybl [:akeybl] 2011-12-08 14:41:06 PST
Comment on attachment 576997 [details] [diff] [review]
Use 64-bit math and check for overflow, rev. 1

[Triage Comment]
Minusing for aurora and beta, but please re-nominate for aurora once this lands on m-c (we'll stick with - for beta given where we are in the cycle now).
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2011-12-15 13:14:03 PST
I'd still like to get this in for 10, it's a trivial patch for an exploitable bug.

Benjamin, can you land this on m-c?
Comment 9 Benjamin Smedberg [:bsmedberg] 2011-12-30 10:05:44 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa47e51cbd8a
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2012-01-05 13:25:50 PST
http://hg.mozilla.org/mozilla-central/rev/aa47e51cbd8a
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2012-01-05 13:26:57 PST
Comment on attachment 576997 [details] [diff] [review]
Use 64-bit math and check for overflow, rev. 1

Trivial patch for an sg:critical bug, we should IMO fix this on aurora and beta.
Comment 12 Alex Keybl [:akeybl] 2012-01-05 14:56:44 PST
Comment on attachment 576997 [details] [diff] [review]
Use 64-bit math and check for overflow, rev. 1

[Triage Comment]
Approved for aurora and beta based upon the trivial nature of this sg:crit patch.
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-06 11:03:14 PST
http://hg.mozilla.org/releases/mozilla-aurora/rev/bb54e4c3ad09
http://hg.mozilla.org/releases/mozilla-beta/rev/a7beadd61664
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-01 12:47:02 PST
(In reply to Honza Bambas (:mayhemer) from comment #0)
> I can try to create a test case.

Were you able to come up with a testcase? QA needs it to verify the fix. Thanks.
Comment 15 Honza Bambas (:mayhemer) 2012-02-02 09:53:21 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #14)
> (In reply to Honza Bambas (:mayhemer) from comment #0)
> > I can try to create a test case.
> 
> Were you able to come up with a testcase? QA needs it to verify the fix.
> Thanks.

I wasn't.  I could try, but please be patient.
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-02 11:03:16 PST
Sure Honza, thanks. I'd like to get this verified before Firefox 11 is released if at all possible so if I can have it in the next month that would be fine.
Comment 17 Al Billings [:abillings] 2012-03-19 17:55:48 PDT
Any luck on a testcase here?
Comment 18 Honza Bambas (:mayhemer) 2012-03-19 17:58:22 PDT
No time so far, sorry.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-29 12:03:43 PDT
qa- until testcase-wanted is satisfied.

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