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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Honza Bambas (:mayhemer)]

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

[Ian Melven :imelven]

looks like an sg:crit to me - potential memory overwrite - dveditz ?

Comment 2

[Ian Melven :imelven]

sg:critical is the right tag, not sg:crit

Comment 3

[Johnny Stenback (:jst)]

John, can you hack up a fix here? I.e. check the math so we don't overflow etc.

Comment 4

[John Schoenick [:johns]]

Attachment: 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.

Comment 5

[Benjamin Smedberg]

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.

Comment 6

[John Schoenick [:johns]]

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

Comment 7

[Honza Bambas (:mayhemer)]

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

This indirectly blocks bug 215450.

Comment 8

[John Schoenick [:johns]]

Attachment: 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.

Comment 9

[John Schoenick [:johns]]

Attachment: Cap reads to PR_UINT32_MAX - 1, r=bsmedberg

Proper commit message, needs checkin

Comment 10

[Johnny Stenback (:jst)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20ad3570d599

Comment 11

[Daniel Holbert [:dholbert]]

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

Comment 12

[John Schoenick [:johns]]

Attachment: 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

Comment 13

[John Schoenick [:johns]]

Attachment: Cap reads to PR_UINT32_MAX - 1, r=bsmedberg

Proper commit message, checkin-needed

Comment 14

[Johnny Stenback (:jst)]

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

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e0e35faac08c

Comment 16

[Johnny Stenback (:jst)]

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 17

[John Schoenick [:johns]]

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

Comment 18

[Alex Keybl [:akeybl]]

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.

Comment 19

[Johnny Stenback (:jst)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e6e504f7407

Comment 20

[John Schoenick [:johns]]

Attachment: [1.9.2] Cap reads to PR_UINT32_MAX - 1, r=bsmedberg

1.9.2 version - NS_MIN -> PR_MIN

Comment 21

[John Schoenick [:johns]]

[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 22

[John Schoenick [:johns]]

Comment on attachment 596731 [details] [diff] [review]
Cap reads to PR_UINT32_MAX - 1, r=bsmedberg

Approval for esr10 - see c17-18

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 24

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/582b3b404829

Comment 25

[u279076]

qa- until testcase-wanted is satisfied