Last Comment Bug 716556 - Potential buffer overflow in nsScriptableInputStream::Read with 4GB data
: Potential buffer overflow in nsScriptableInputStream::Read with 4GB data
Status: RESOLVED FIXED
[sg:moderate][qa-]
: crash
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla13
Assigned To: John Schoenick [:johns]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-09 08:15 PST by Honza Bambas (:mayhemer)
Modified: 2015-10-16 11:52 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
-
wontfix
-
wontfix
+
fixed
+
fixed
+
fixed
12+
fixed
needed
wanted


Attachments
Fix buffer overflow with nsScriptableInputStream for aCount >= SIZE_MAX (1.87 KB, patch)
2012-01-13 14:42 PST, John Schoenick [:johns]
benjamin: review-
Details | Diff | Splinter Review
Cap reads to PR_UINT32_MAX - 1 (1.08 KB, patch)
2012-02-08 12:39 PST, John Schoenick [:johns]
benjamin: review+
Details | Diff | Splinter Review
Cap reads to PR_UINT32_MAX - 1, r=bsmedberg (1.10 KB, patch)
2012-02-08 14:46 PST, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
Cap reads to PR_UINT32_MAX - 1, attempt 2 (1.11 KB, patch)
2012-02-10 13:21 PST, John Schoenick [:johns]
benjamin: review+
Details | Diff | Splinter Review
Cap reads to PR_UINT32_MAX - 1, r=bsmedberg (1.12 KB, patch)
2012-02-13 11:27 PST, John Schoenick [:johns]
akeybl: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review
[1.9.2] Cap reads to PR_UINT32_MAX - 1, r=bsmedberg (1.11 KB, patch)
2012-03-13 10:33 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review

Description Honza Bambas (:mayhemer) 2012-01-09 08:15:57 PST
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 2012-01-09 12:45:44 PST
looks like an sg:crit to me - potential memory overwrite - dveditz ?
Comment 2 Ian Melven :imelven 2012-01-09 12:47:45 PST
sg:critical is the right tag, not sg:crit
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2012-01-12 13:19:07 PST
John, can you hack up a fix here? I.e. check the math so we don't overflow etc.
Comment 4 John Schoenick [:johns] 2012-01-13 14:42:34 PST
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.
Comment 5 Benjamin Smedberg [:bsmedberg] 2012-01-16 10:16:17 PST
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] 2012-01-17 13:46:41 PST
(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) 2012-02-07 13:52:51 PST
Could we just add - 1 to the second NS_MIN arg?

This indirectly blocks bug 215450.
Comment 8 John Schoenick [:johns] 2012-02-08 12:39:10 PST
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.
Comment 9 John Schoenick [:johns] 2012-02-08 14:46:12 PST
Created attachment 595545 [details] [diff] [review]
Cap reads to PR_UINT32_MAX - 1, r=bsmedberg

Proper commit message, needs checkin
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-08 20:34:43 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/20ad3570d599
Comment 11 Daniel Holbert [:dholbert] 2012-02-08 22:26:15 PST
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] 2012-02-10 13:21:09 PST
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
Comment 13 John Schoenick [:johns] 2012-02-13 11:27:29 PST
Created attachment 596731 [details] [diff] [review]
Cap reads to PR_UINT32_MAX - 1, r=bsmedberg

Proper commit message, checkin-needed
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-16 17:46:49 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0e35faac08c
Comment 15 Ed Morley [:emorley] 2012-02-17 05:10:54 PST
https://hg.mozilla.org/mozilla-central/rev/e0e35faac08c
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2012-03-01 13:34:24 PST
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] 2012-03-02 13:43:34 PST
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] 2012-03-09 11:52:45 PST
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, jst@mozilla.com) 2012-03-12 17:04:07 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/2e6e504f7407
Comment 20 John Schoenick [:johns] 2012-03-13 10:33:42 PDT
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
Comment 21 John Schoenick [:johns] 2012-03-13 10:34:43 PDT
[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] 2012-03-13 11:50:10 PDT
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 2012-03-20 10:29:15 PDT
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] 2012-03-22 13:23:43 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/582b3b404829
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-29 12:08:29 PDT
qa- until testcase-wanted is satisfied

Note You need to log in before you can comment on or make changes to this bug.