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

RESOLVED FIXED in Firefox 12

Status

()

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

People

(Reporter: mayhemer, Assigned: johns)

Tracking

({crash})

Trunk
mozilla13
crash
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)

(Reporter)

Description

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

Comment 1

6 years ago
looks like an sg:crit to me - potential memory overwrite - dveditz ?
Whiteboard: [sg:crit]

Comment 2

6 years ago
sg:critical is the right tag, not sg:crit
Whiteboard: [sg:crit] → [sg:critical]
blocking1.9.2: --- → needed
status1.9.2: --- → wanted
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox9: --- → affected
tracking-firefox10: --- → -
tracking-firefox11: --- → +
tracking-firefox12: --- → +
Keywords: crash, testcase-wanted
status-firefox9: affected → wontfix
tracking-firefox9: --- → -
John, can you hack up a fix here? I.e. check the math so we don't overflow etc.
Assignee: nobody → jschoenick
status-firefox9: wontfix → affected

Updated

6 years ago
status-firefox9: affected → wontfix
(Assignee)

Comment 4

6 years ago
Created attachment 588532 [details] [diff] [review]
Fix buffer overflow with nsScriptableInputStream for aCount >= SIZE_MAX

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 5

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

Comment 6

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

Comment 7

6 years ago
Could we just add - 1 to the second NS_MIN arg?

This indirectly blocks bug 215450.
(Assignee)

Comment 8

6 years ago
Created attachment 595495 [details] [diff] [review]
Cap reads to PR_UINT32_MAX - 1

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)

Updated

6 years ago
Attachment #595495 - Flags: review?(benjamin) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 9

6 years ago
Created attachment 595545 [details] [diff] [review]
Cap reads to PR_UINT32_MAX - 1, r=bsmedberg

Proper commit message, needs checkin
Attachment #595495 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/20ad3570d599
Keywords: checkin-needed

Updated

6 years ago
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?
(Assignee)

Comment 12

6 years ago
Created attachment 596154 [details] [diff] [review]
Cap reads to PR_UINT32_MAX - 1, attempt 2

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)

Updated

6 years ago
Attachment #596154 - Flags: review?(benjamin) → review+
(Assignee)

Comment 13

6 years ago
Created attachment 596731 [details] [diff] [review]
Cap reads to PR_UINT32_MAX - 1, r=bsmedberg

Proper commit message, checkin-needed
Attachment #596154 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0e35faac08c
https://hg.mozilla.org/mozilla-central/rev/e0e35faac08c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
status-firefox-esr10: --- → affected
status-firefox10: affected → wontfix
status-firefox11: affected → wontfix
status-firefox13: --- → fixed
tracking-firefox-esr10: --- → ?
tracking-firefox11: + → -
tracking-firefox13: --- → +
Whiteboard: [sg:critical] → [sg:moderate]
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.
tracking-firefox-esr10: ? → 12+
(Assignee)

Comment 17

6 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/2e6e504f7407
status-firefox12: affected → fixed
status-firefox14: --- → fixed
tracking-firefox14: --- → +
(Assignee)

Updated

6 years ago
Attachment #596731 - Attachment description: [checkin-needed] Cap reads to PR_UINT32_MAX - 1, r=bsmedberg → Cap reads to PR_UINT32_MAX - 1, r=bsmedberg
(Assignee)

Comment 20

6 years ago
Created attachment 605446 [details] [diff] [review]
[1.9.2] Cap reads to PR_UINT32_MAX - 1, r=bsmedberg

1.9.2 version - NS_MIN -> PR_MIN
(Assignee)

Updated

6 years ago
Attachment #605446 - Flags: approval1.9.2.28?
(Assignee)

Comment 21

6 years ago
[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
(Assignee)

Comment 22

6 years ago
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+
https://hg.mozilla.org/releases/mozilla-esr10/rev/582b3b404829
status-firefox-esr10: affected → fixed
qa- until testcase-wanted is satisfied
Whiteboard: [sg:moderate] → [sg:moderate][qa-]
Group: core-security
(Assignee)

Updated

5 years ago
Attachment #605446 - Flags: approval1.9.2.28?
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.