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]
:
: Nathan Froyd [:froydnj]
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 User image 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 User image Ian Melven :imelven 2012-01-09 12:45:44 PST
looks like an sg:crit to me - potential memory overwrite - dveditz ?
Comment 2 User image Ian Melven :imelven 2012-01-09 12:47:45 PST
sg:critical is the right tag, not sg:crit
Comment 3 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Johnny Stenback (:jst, jst@mozilla.com) 2012-02-08 20:34:43 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/20ad3570d599
Comment 11 User image 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 User image 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 User image 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 User image Johnny Stenback (:jst, jst@mozilla.com) 2012-02-16 17:46:49 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0e35faac08c
Comment 15 User image Ed Morley [:emorley] 2012-02-17 05:10:54 PST
https://hg.mozilla.org/mozilla-central/rev/e0e35faac08c
Comment 16 User image 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 User image 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 User image 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 User image Johnny Stenback (:jst, jst@mozilla.com) 2012-03-12 17:04:07 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/2e6e504f7407
Comment 20 User image 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 User image 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 User image 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 User image 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 User image Andrew McCreight [:mccr8] 2012-03-22 13:23:43 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/582b3b404829
Comment 25 User image 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.