Potential buffer overflow in nsScriptableInputStream::Read with 4GB data

RESOLVED FIXED in Firefox 12

Status

()

defect
--
critical
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: mayhemer, Assigned: johns)

Tracking

({crash})

Trunk
mozilla13
Points:
---

Firefox Tracking Flags

(firefox9- wontfix, firefox10- wontfix, firefox11- wontfix, firefox12+ fixed, firefox13+ fixed, firefox14+ fixed, firefox-esr1012+ fixed, blocking1.9.2 needed, status1.9.2 wanted)

Details

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

Attachments

(2 attachments, 4 obsolete attachments)

NS_IMETHODIMP
nsScriptableInputStream::Read(PRUint32 aCount, char **_retval) {
    nsresult rv = NS_OK;
    PRUint32 count = 0;
    char *buffer = nsnull;

    if (!mInputStream) return NS_ERROR_NOT_INITIALIZED;

    rv = mInputStream->Available(&count);
    if (NS_FAILED(rv)) return rv;

    count = NS_MIN(count, aCount);
    buffer = (char*)nsMemory::Alloc(count+1); // make room for '\0'
    if (!buffer) return NS_ERROR_OUT_OF_MEMORY;


- have mInputStream having exactly 4GB of data, its available will return PR_UINT32_MAX
- try to read all this 4GB of data with the scriptable stream
- count+1 will evaluate as 0

It is not well defined what result of malloc (if nsMemory::Alloc resolve to it) is with 0 as an arg.

So, we may potentially get an invalid pointer and write to it.
looks like an sg:crit to me - potential memory overwrite - dveditz ?
Whiteboard: [sg:crit]
sg:critical is the right tag, not sg:crit
Whiteboard: [sg:crit] → [sg:critical]
John, can you hack up a fix here? I.e. check the math so we don't overflow etc.
Assignee: nobody → jschoenick
This checks that aCount is not >= SIZE_MAX, which avoids capping reads to 4GiB on 64bit systems, and ensures this wont regress if these interfaces are changed off of PRUint32 in the future.
Attachment #588532 - Flags: review?(benjamin)
Comment on attachment 588532 [details] [diff] [review]
Fix buffer overflow with nsScriptableInputStream for aCount >= SIZE_MAX

aCount is a PRUint32. If you want to prevent it from overflowing, you need to use PR_UINT32_MAX, not SIZE_MAX... size_t could be a 64-bit size.
Attachment #588532 - Flags: review?(benjamin) → review-
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Comment on attachment 588532 [details] [diff] [review]
> Fix buffer overflow with nsScriptableInputStream for aCount >= SIZE_MAX
> 
> aCount is a PRUint32. If you want to prevent it from overflowing, you need
> to use PR_UINT32_MAX, not SIZE_MAX... size_t could be a 64-bit size.

With the patch, (size_t)aCount+1 is passed to Alloc, so we only need to check that aCount isn't going to overflow a size_t. This prevents having a wierd edge case with 4GiB reads on 64bit systems (where such a thing is feasible, if unlikely at present).
Could we just add - 1 to the second NS_MIN arg?

This indirectly blocks bug 215450.
Per conversation on IRC, preventing the 4GiB edge case isn't worth the added code complexity - just cap reads at PR_UINT32_MAX - 1.
Attachment #588532 - Attachment is obsolete: true
Attachment #595495 - Flags: review?(benjamin)
Attachment #595495 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
Proper commit message, needs checkin
Attachment #595495 - Attachment is obsolete: true
Attachment #595545 - Flags: checkin+
Philor backed this out for causing xpcshell bustage:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/9e23446b3559

The xpcshell failure looks like this:
{
TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\xpcshell\tests\content\test\unit\test_xml_serializer.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | xpcshell/head.js | [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIScriptableInputStream.read]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: c:/talos-slave/test/build/xpcshell/tests/content/test/unit/test_xml_serializer.js :: test8 :: line 289"  data: no]

}

Sample orange xpcshell logs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=9199851&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=9199677&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=9199868&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=9200717&tree=Mozilla-Inbound
Attachment #595545 - Flags: checkin+ → checkin?
Attachment #595545 - Attachment description: [checkin-needed] Cap reads to PR_UINT32_MAX - 1, r=bsmedberg → Cap reads to PR_UINT32_MAX - 1, r=bsmedberg
Attachment #595545 - Flags: checkin?
Per more conversations in IRC

The interface doesn't guarantee that bytes read will be min(available, requested), so just reading max-1 bytes when 4GiB is requested is acceptable. This lets scripts continue to do read(-1) as a shorthand for getting all available bytes, which is what broke the tests.

For good measure I pushed this version to try:
https://tbpl.mozilla.org/?tree=Try&rev=b60d2d4ea351
Attachment #595545 - Attachment is obsolete: true
Attachment #596154 - Flags: review?(benjamin)
Attachment #596154 - Flags: review?(benjamin) → review+
Proper commit message, checkin-needed
Attachment #596154 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e0e35faac08c
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
John, can you check if this applies to aurora (I'd be surprised if it didn't) and request approval for aurora if it does? Thanks.
Comment on attachment 596731 [details] [diff] [review]
Cap reads to PR_UINT32_MAX - 1, r=bsmedberg

[Approval Request Comment]
User impact if declined: [sg-moderate]
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Very low
Attachment #596731 - Flags: approval-mozilla-aurora?
Comment on attachment 596731 [details] [diff] [review]
Cap reads to PR_UINT32_MAX - 1, r=bsmedberg

[Triage Comment]
Low risk security fix - approved for Aurora 12.
Attachment #596731 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #596731 - Attachment description: [checkin-needed] Cap reads to PR_UINT32_MAX - 1, r=bsmedberg → Cap reads to PR_UINT32_MAX - 1, r=bsmedberg
1.9.2 version - NS_MIN -> PR_MIN
Attachment #605446 - Flags: approval1.9.2.28?
[Approval Request Comment]
For 1.9.2:
User impact if declined: [sg-moderate]
Testing completed (on m-c, etc.): on m-c, on aurora
Risk to taking this patch (and alternatives if risky): Very low
Comment on attachment 596731 [details] [diff] [review]
Cap reads to PR_UINT32_MAX - 1, r=bsmedberg

Approval for esr10 - see c17-18
Attachment #596731 - Flags: approval-mozilla-esr10?
Comment on attachment 596731 [details] [diff] [review]
Cap reads to PR_UINT32_MAX - 1, r=bsmedberg

[Triage Comment]
Please go ahead and land on ESR branch, see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for details.
Attachment #596731 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
qa- until testcase-wanted is satisfied
Whiteboard: [sg:moderate] → [sg:moderate][qa-]
Group: core-security
Attachment #605446 - Flags: approval1.9.2.28?
You need to log in before you can comment on or make changes to this bug.