692817 - nsA(C)String::SetLength overflow in EncodeInputStream

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Honza Bambas (:mayhemer)]

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)]

All potential consumers:
http://mxr.mozilla.org/mozilla-central/ident?i=Base64EncodeInputStream

Comment 2

[Daniel Veditz [:dveditz]]

Clearing sg:critical to trigger triage while we confirm a problem here

Comment 3

[Daniel Veditz [:dveditz]]

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]

Attachment: 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-4, no longer working on Mozilla)]

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)]

bsmedberg, can we get this landed on m-c?

Comment 7

[Alex Keybl [:akeybl]]

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)]

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]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa47e51cbd8a

Comment 10

[Johnny Stenback (:jst)]

http://hg.mozilla.org/mozilla-central/rev/aa47e51cbd8a

Comment 11

[Johnny Stenback (:jst)]

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]]

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 (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/bb54e4c3ad09
http://hg.mozilla.org/releases/mozilla-beta/rev/a7beadd61664

Comment 14

[u279076]

(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)]

(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

[u279076]

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 - ex-MoCo]]

Any luck on a testcase here?

Comment 18

[Honza Bambas (:mayhemer)]

No time so far, sorry.

Comment 19

[u279076]

qa- until testcase-wanted is satisfied.