Crash [@ memmove] or [@ mozilla::PodCopy] or [@ js_NewStringCopyN] or Assertion failure: PointerRangeSize(src, static_cast<const T*>(dst)) >= nelem, at dist/include/mozilla/PodOperations.h

VERIFIED FIXED in Firefox 29

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: gkw, Assigned: jonco)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla31
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox28 wontfix, firefox29 fixed, firefox30+ fixed, firefox31+ fixed, firefox-esr24 unaffected, b2g-v1.2 unaffected, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

(Whiteboard: [jsbugmon:update][adv-main29+], crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
function x() {}
for (var j = 0; j < 99999; ++j) {
    (function() {
        x += x.watch("endsWith", SharedArrayBuffer);
        return 0 >> Function(x)
    })()
}

crashes js debug shell on m-c changeset 573ef29c7a9f with --ion-eager --ion-parallel-compile=off at memmove with mozilla::PodCopy and js_NewStringCopyN on the stack. It also crashes js opt shell at memmove with js_NewStringCopyN on the stack.

Sometimes this assertion pops up: Assertion failure: PointerRangeSize(src, static_cast<const T*>(dst)) >= nelem, at dist/include/mozilla/PodOperations.h

s-s because such varying signatures are scary.

My debug configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

My opt configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140226135810" and the hash "ac9709133b27".
The "bad" changeset has the timestamp "20140226135912" and the hash "7bb98e166b5b".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ac9709133b27&tochange=7bb98e166b5b

Sean, only bug 933001 is on the stack, is it a likely regressor?
Flags: needinfo?(sstangl)
No longer blocks: 933001
Flags: needinfo?(sstangl)
This is not a SharedArrayBuffer bug -- any fuzz result that uses "SharedArrayBuffer" as a constructor will bisect at least to the patch that introduces it. In this case, the following modified testcase also reproduces the crash (on x64, with --no-ion --no-baseline):

function x() {}
for (var j = 0; j < 99999; ++j) {
  (function() {
    x += x.watch("endsWith", ArrayBuffer);
    return 0 >> Function(x);
  })()
}

I would recommend bisecting with this new version.
Reporter

Comment 2

5 years ago
Thanks Sean!

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/78a029898775
user:        Jon Coppeard
date:        Fri Feb 07 11:20:38 2014 +0000
summary:     Bug 961091 - Perform GC if necessary on exit from engine and on interpreter allocation r=terrence

Jon, seems like bug 961091 might be a more likely regressor, would it?
Blocks: 961091
Flags: needinfo?(jcoppeard)
Assignee

Comment 3

5 years ago
Posted patch bug986864-autoDeferPurge (obsolete) — Splinter Review
The problem is with SourceDataCache::AutoSuppressPurge.  SourceDataCache maps from ScriptSource* to decompressed source chars.  AutoSuppressPurge stops this cache being cleared on GC, but if a ScriptSource* that has an entry in the table is destroyed by the collection then its entry in the cache will not be removed.

Then, if a different ScriptSource is created at the same address, the cache will return the data for the old entry.

ScriptSource::substring() assumes the length of the return string, and in this case it's not what it expects and it tries to copy far more data out of it than it actually contains, which results in the assertion failures seen here.

The solution is not to suppress the purge, but merely defer it while someone can be accessing the data in the cache.  I changed AutoSuppressPurge to AutoDeferPurge, which will perform the purge in its destructor if a GC happens while it's on the stack.
Assignee: nobody → jcoppeard
Attachment #8397143 - Flags: review?(sphink)
Flags: needinfo?(jcoppeard)
Comment on attachment 8397143 [details] [diff] [review]
bug986864-autoDeferPurge

Review of attachment 8397143 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed IRL, it would be simpler to not use any RAII protection at all, and instead allocate a string of the right length in advance, then copy the character data in when you can't GC anymore.
Attachment #8397143 - Flags: review?(sphink)
Assignee

Comment 5

5 years ago
Posted patch bug986864-scriptSource2 (obsolete) — Splinter Review
Unfortunately it turned out that we couldn't ensure there was no GC whenever these chars where in use.

Here's new patch.  This adds a a holder object that takes ownership of the chars for a specific ScriptSource if the cache is purged, and deletes them itself when it goes out of scope.
Attachment #8398484 - Flags: review?(sphink)
Comment on attachment 8398484 [details] [diff] [review]
bug986864-scriptSource2

Review of attachment 8398484 [details] [diff] [review]:
-----------------------------------------------------------------

Had to stare at it for a while, but it looks good to me now.

::: js/src/jit-test/tests/gc/bug-986864.js
@@ +1,5 @@
> +// |jit-test| slow
> +function x() {}
> +for (var j = 0; j < 9999; ++j) {
> +    (function() {
> +        x += x.watch("endsWith", SharedArrayBuffer);

It would be much better if we could trigger this with something other than SharedArrayBuffer. That will get automatically disabled on Aurora and everything after, breaking this test.

::: js/src/jsscript.cpp
@@ +1423,5 @@
> +    // The holder is going out of scope.  If it has taken ownership of cached
> +    // chars then delete them, otherwise unregister ourself with the cache.
> +    if (charsToDelete_) {
> +        JS_ASSERT(!cache_ && !source_);
> +        js_delete(const_cast<jschar *>(charsToDelete_));

I think charsToDelete_ was allocated with js_realloc, so this should really be js_free instead of js_delete. (Maybe that means it should be charsToFree_?)
Attachment #8398484 - Flags: review?(sphink) → review+
Reporter

Comment 7

5 years ago
(In reply to Steve Fink [:sfink] from comment #6)
> ::: js/src/jit-test/tests/gc/bug-986864.js
> @@ +1,5 @@
> > +// |jit-test| slow
> > +function x() {}
> > +for (var j = 0; j < 9999; ++j) {
> > +    (function() {
> > +        x += x.watch("endsWith", SharedArrayBuffer);
> 
> It would be much better if we could trigger this with something other than
> SharedArrayBuffer. That will get automatically disabled on Aurora and
> everything after, breaking this test.

Let's use the test Sean made in comment 1 - it doesn't use SharedArrayBuffer, instead only relying on ArrayBuffer.
Assignee

Comment 8

5 years ago
Updated patch with review comments addressed.
Attachment #8397143 - Attachment is obsolete: true
Attachment #8398484 - Attachment is obsolete: true
Attachment #8398617 - Flags: review+
Assignee

Comment 9

5 years ago
AutoSuppressPurge was introduce in bug 938615.
Reporter

Comment 10

5 years ago
(In reply to Jon Coppeard (:jonco) from comment #9)
> AutoSuppressPurge was introduce in bug 938615.

Will this be the correct regressing bug? If so, this issue goes back to version 28.
Flags: needinfo?(jcoppeard)
Assignee

Comment 11

5 years ago
Comment on attachment 8398617 [details] [diff] [review]
bug986864-scriptSource-v3

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

So the problem is that this cache can contain a stale entry for a dead ScriptSource* and then hand out the pointer to its (live) source string.  The callers here assume the length of the string, but they only read from it.  So they may read source data for the wrong thing, which may be controllable by an attacker, and they may also read past the end of it.

An attack might be possible to get code in a privileged compartment to execute javascript code controllable by an attacker, but it seems this would be hard to arrange.  It would involve getting GC to trigger at a very specific point, and then somehow getting the privileged compartment to allocate a ScriptSource at the same address as a dead ScriptSource set up by the attacker.

So I think the answer is that it would be possible but difficult.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I would say it's not obvious from the patch.

Which older supported branches are affected by this flaw?

26 onwards.

If not all supported branches, which bug introduced the flaw?

Bug 938615.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

It should be possible to apply the same patch.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely.  Letting it bake on central for a few days should be fine.
Attachment #8398617 - Flags: sec-approval?
Comment on attachment 8398617 [details] [diff] [review]
bug986864-scriptSource-v3

sec-approval+ for trunk.

If this is 26 onwards, why doesn't it affect b2g 1.2, which is derived from 26?
Attachment #8398617 - Flags: sec-approval? → sec-approval+
Assignee

Comment 13

5 years ago
Sorry, this was introduced in 28 (not 26).
Blocks: 938615
No longer blocks: 961091
Flags: needinfo?(jcoppeard)
https://hg.mozilla.org/mozilla-central/rev/72ba89a774db

Please request Aurora/Beta approval on this ASAP.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Flags: needinfo?(jcoppeard)
Assignee

Comment 16

5 years ago
Comment on attachment 8398617 [details] [diff] [review]
bug986864-scriptSource-v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 938615
User impact if declined: Potential security issue.
Testing completed (on m-c, etc.): On m-c since 2014-03-31.
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None
Attachment #8398617 - Flags: approval-mozilla-beta?
Attachment #8398617 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jcoppeard)
Attachment #8398617 - Flags: approval-mozilla-beta?
Attachment #8398617 - Flags: approval-mozilla-beta+
Attachment #8398617 - Flags: approval-mozilla-aurora?
Attachment #8398617 - Flags: approval-mozilla-aurora+
Group: javascript-core-security → core-security
Status: RESOLVED → VERIFIED
Crash Signature: [@ memmove] [@ mozilla::PodCopy] [@ js_NewStringCopyN] → [@ memmove] [@ mozilla::PodCopy] [@ js_NewStringCopyN]
JSBugMon: This bug has been automatically verified fixed.
Crash Signature: [@ memmove] [@ mozilla::PodCopy] [@ js_NewStringCopyN] → [@ memmove] [@ mozilla::PodCopy] [@ js_NewStringCopyN]
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main29+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.