nsA(C)String::SetLength overflow in EncodeInputStream

RESOLVED FIXED

Status

()

Core
XPCOM
--
critical
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: mayhemer, Assigned: bsmedberg)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox8- wontfix, firefox9- wontfix, firefox10+ fixed, firefox11+ fixed, firefox12+ fixed, firefox-esr1010+ fixed, status1.9.2 unaffected)

Details

(Whiteboard: [sg:critical][qa-])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
All potential consumers:
http://mxr.mozilla.org/mozilla-central/ident?i=Base64EncodeInputStream
Clearing sg:critical to trigger triage while we confirm a problem here
Keywords: testcase-wanted
Whiteboard: [sg:critical?]
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.
Assignee: nobody → benjamin
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [sg:critical]

Updated

6 years ago
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox8: --- → wontfix
status-firefox9: --- → affected
tracking-firefox10: --- → +
tracking-firefox11: --- → +
tracking-firefox8: --- → -
tracking-firefox9: --- → +
(Assignee)

Comment 4

6 years ago
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.
Attachment #576997 - Flags: review?(dbaron)
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.
Attachment #576997 - Flags: review?(dbaron) → review+
Attachment #576997 - Flags: approval-mozilla-beta?
Attachment #576997 - Flags: approval-mozilla-aurora?
bsmedberg, can we get this landed on m-c?

Comment 7

6 years ago
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).
Attachment #576997 - Flags: approval-mozilla-beta?
Attachment #576997 - Flags: approval-mozilla-beta-
Attachment #576997 - Flags: approval-mozilla-aurora?
Attachment #576997 - Flags: approval-mozilla-aurora-
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?
status-firefox9: affected → wontfix
tracking-firefox9: + → -
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa47e51cbd8a
http://hg.mozilla.org/mozilla-central/rev/aa47e51cbd8a
status-firefox12: --- → fixed
tracking-firefox12: --- → +
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.
Attachment #576997 - Flags: approval-mozilla-beta?
Attachment #576997 - Flags: approval-mozilla-beta-
Attachment #576997 - Flags: approval-mozilla-aurora?
Attachment #576997 - Flags: approval-mozilla-aurora-

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
Attachment #576997 - Flags: approval-mozilla-beta?
Attachment #576997 - Flags: approval-mozilla-beta+
Attachment #576997 - Flags: approval-mozilla-aurora?
Attachment #576997 - Flags: approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/bb54e4c3ad09
http://hg.mozilla.org/releases/mozilla-beta/rev/a7beadd61664
status-firefox10: affected → fixed
status-firefox11: affected → fixed
status1.9.2: --- → unaffected
(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.
Whiteboard: [sg:critical] → [sg:critical][qa?]
(Reporter)

Comment 15

5 years ago
(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.
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.

Updated

5 years ago
status-firefox-esr10: --- → fixed

Updated

5 years ago
tracking-firefox-esr10: --- → 10+
Any luck on a testcase here?
(Reporter)

Comment 18

5 years ago
No time so far, sorry.
Group: core-security
qa- until testcase-wanted is satisfied.
Whiteboard: [sg:critical][qa?] → [sg:critical][qa-]
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.