1241731 - Assertion failure: tag >= SCTAG_TRANSFER_MAP_PENDING_ENTRY, at js/src/vm/StructuredClone.cpp:429 with OOM

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 6764bc656c1d (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2 --ion-offthread-compile=off --ion-extra-checks):

switch (0) {
    default: oomTest(() => serialize(0, [schedulegc]))
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x087a0d40 in DiscardTransferables (buffer=<optimized out>, nbytes=<optimized out>, cb=0x0, cbClosure=0x0) at js/src/vm/StructuredClone.cpp:429
#0  0x087a0d40 in DiscardTransferables (buffer=<optimized out>, nbytes=<optimized out>, cb=0x0, cbClosure=0x0) at js/src/vm/StructuredClone.cpp:429
#1  0x087a79da in JSStructuredCloneWriter::~JSStructuredCloneWriter (this=0xffffb590, __in_chrg=<optimized out>) at js/src/vm/StructuredClone.cpp:749
#2  0x087d49a8 in WriteStructuredClone (cx=cx@entry=0xf7a7b020, v=v@entry=..., bufp=bufp@entry=0xffffb888, nbytesp=nbytesp@entry=0xffffb88c, cb=cb@entry=0x0, cbClosure=cbClosure@entry=0x0, transferable=...) at js/src/vm/StructuredClone.cpp:390
#3  0x087d4aeb in JS_WriteStructuredClone (cx=cx@entry=0xf7a7b020, value=value@entry=..., bufp=bufp@entry=0xffffb888, nbytesp=nbytesp@entry=0xffffb88c, optionalCallbacks=optionalCallbacks@entry=0x0, closure=closure@entry=0x0, transferable=transferable@entry=...) at js/src/vm/StructuredClone.cpp:2137
#4  0x087d4c50 in JSAutoStructuredCloneBuffer::write (this=this@entry=0xffffb888, cx=cx@entry=0xf7a7b020, value=..., transferable=..., optionalCallbacks=optionalCallbacks@entry=0x0, closure=closure@entry=0x0) at js/src/vm/StructuredClone.cpp:2325
#5  0x086f3f21 in Serialize (cx=0xf7a7b020, argc=2, vp=0xf57190b8) at js/src/builtin/TestingFunctions.cpp:2015
#6  0x086d231a in js::CallJSNative (cx=0xf7a7b020, native=0x86f3e90 <Serialize(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#7  0x086cebb1 in js::Invoke (cx=0xf7a7b020, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:478
#8  0x086be90a in Interpret (cx=cx@entry=0xf7a7b020, state=...) at js/src/vm/Interpreter.cpp:2802
#9  0x086ce92d in js::RunScript (cx=cx@entry=0xf7a7b020, state=...) at js/src/vm/Interpreter.cpp:425
#10 0x086cec66 in js::Invoke (cx=0xf7a7b020, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:496
#11 0x086d07d2 in js::Invoke (cx=cx@entry=0xf7a7b020, thisv=..., fval=..., argc=argc@entry=0, argv=argv@entry=0x0, rval=rval@entry=...) at js/src/vm/Interpreter.cpp:530
#12 0x08561c60 in JS_CallFunction (cx=cx@entry=0xf7a7b020, obj=..., fun=fun@entry=..., args=..., rval=rval@entry=...) at js/src/jsapi.cpp:2857
#13 0x086e6e6a in OOMTest (cx=0xf7a7b020, argc=1, vp=0xf5719060) at js/src/builtin/TestingFunctions.cpp:1218
#14 0x086d231a in js::CallJSNative (cx=0xf7a7b020, native=0x86e6b70 <OOMTest(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]
#26 main (argc=6, argv=0xffffcbd4, envp=0xffffcbf0) at js/src/shell/js.cpp:6985
eax	0x0	0
ebx	0x9842434	159654964
ecx	0xf7e3b88c	-136071028
edx	0x0	0
esi	0x1	1
edi	0xf7a02730	-140499152
ebp	0xffffb4a8	4294948008
esp	0xffffb450	4294947920
eip	0x87a0d40 <DiscardTransferables(uint64_t*, size_t, JSStructuredCloneCallbacks const*, void*)+320>
=> 0x87a0d40 <DiscardTransferables(uint64_t*, size_t, JSStructuredCloneCallbacks const*, void*)+320>:	movl   $0x1ad,0x0
   0x87a0d4a <DiscardTransferables(uint64_t*, size_t, JSStructuredCloneCallbacks const*, void*)+330>:	call   0x80fe8d0 <abort()>

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20151013053056" and the hash "8d9c20c241be7d7b3cfa90a3368a77db42172781".
The "bad" changeset has the timestamp "20151013054956" and the hash "d80f9d6921f8209ef01aa730be9a97ab727704d1".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8d9c20c241be7d7b3cfa90a3368a77db42172781&tochange=d80f9d6921f8209ef01aa730be9a97ab727704d1

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: OOM_VERBOSE=1 stack

I have attached a OOM_VERBOSE=1 stack.

Any idea how best to move this forward, Jon?

Comment 3

[Jon Coppeard (:jonco)]

Looks like an out of bounds buffer access on OOM.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1241731-structured-clone-oom

If we hit OOM in JSStructuredCloneWriter::writeTransferMap() we may not finish writing data into the out buffer.  DiscardTransferables() then reads numTransferables from the buffer and doesn't check the buffer position against the actual length.

The patch adds checks so we don't read beyond the end of the buffer.

Comment 5

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8711629 [details] [diff] [review]
bug1241731-structured-clone-oom

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

This change is definitely good. The semantics of OOM here are a little tricky still. If you hit this OOM, you'll end up discarding some but not all of the transferables passed in, without knowing which is which. So if the caller sees a false return value, it doesn't know which things to free, and presumably just leaks them right now.

Do you agree with that analysis? If so, I'll file a bug on freeing all of transferableObjects and not just the ones that happened to make it into the clone buffer before the OOM. We'd need to do that for *any* false-return, though.

Alternatively, we could say this is good. These are JSObjects, after all, so it's not like anything is *really* getting leaked. Transferables should be tracking their internal ownership already, so the finalizer knows whether to discard it or not.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink, :s:] from comment #5)
> If you hit this OOM, you'll end up discarding some but not all
> of the transferables passed in, without knowing which is which.

If we hit OOM during writeTransferMap() and don't finish writing the buffer then what we are left with is a buffer containing placeholder information only, i.e. null pointers and with ownership SCTAG_TMO_UNFILLED.  These won't get freed by DiscardTransferables() anyway.

If we hit OOM in transferOwnership(), then we have this problem where some of the transferables may have had their contents stolen and others not.  I don't think this is a problem though - as you say, these are JSObjects and ownership of any buffers is tracked internally already.  So the caller should just assume their contents may have been transferred and drop their references to them and they will be cleaned up fine either way.

Do you agree with that?

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8711629 [details] [diff] [review]
bug1241731-structured-clone-oom

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

Very difficult.  It would involve triggering OOM at the right time and arranging to have the right data present after the end buffer.  This could then trigger a write somewhere in the heap which might be exploitable.

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

I guess adding buffer end checks is kind of suspicious.

Which older supported branches are affected by this flaw?

Everything back to FF27.

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

Bug 861925.

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

Should be trivial.

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

Unlikely to to cause regressions.

Comment 8

[Steve Fink [:sfink] [:s:]]

(In reply to Jon Coppeard (:jonco) from comment #6)
> Do you agree with that?

Yes. I agree this is best left as is now. Thanks!

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8711629 [details] [diff] [review]
bug1241731-structured-clone-oom

sec-approval+ for checkin on Feb 9.

Please nominate for branches as we'll want this on ESR38, Aurora, and Beta as well.

Comment 10

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 2b73b0a4d52b).

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c98efaf3ae1c was this pushed to early re: comment 9 ?

Comment 12

[Jon Coppeard (:jonco)]

(In reply to Carsten Book [:Tomcat] from comment #11)
Sorry, I didn't see that this was approved for checkin in Feb and pushed straight away.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This was pushed early, see comment 11 and comment 12.

Comment 14

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Since this already landed on m-c, should we uplift now or wait till Feb. 9? 
The next beta build will be Monday Feb. 1st.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you request uplift approval please?

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Might as well checkin now. I doubt we'll zero day ourselves further than we already have.

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #16)
> Can you request uplift approval please?

The patch writer is usually the best person to ask for such approvals.

Comment 19

[Jon Coppeard (:jonco)]

Comment on attachment 8711629 [details] [diff] [review]
bug1241731-structured-clone-oom

Approval Request Comment
[Feature/regressing bug #]:  Bug 861925.
[User impact if declined]: Possible security vulnerability.
[Describe test coverage new/current, TreeHerder]: On central since 28th Jan.
[Risks and why]: Low.
[String/UUID change made/needed]: None.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8711629 [details] [diff] [review]
bug1241731-structured-clone-oom

Fix for sec regression, please uplift to aurora and beta.

Comment 21

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/564252067a68
https://hg.mozilla.org/releases/mozilla-beta/rev/33058a9ad6be

Comment 22

[Carsten Book [:Tomcat]]

(In reply to Jon Coppeard (:jonco) from comment #21)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/564252067a68
> https://hg.mozilla.org/releases/mozilla-beta/rev/33058a9ad6be

setting flags

Comment 23

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx45
JSBugMon: This bug has been automatically verified fixed on Fx46

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jon, as Al suggested in comment 9, we will need an uplift to esr38 as well. Could you please nominate a patch for uplift? Thanks!

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This never got ready for esr38.7. Tracked for esr38.8

Comment 26

[Jon Coppeard (:jonco)]

Attachment: bug1241731-esr38

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high.
User impact if declined: Possible security vulnerability.
Fix Landed on Version: 47
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8727422 [details] [diff] [review]
bug1241731-esr38

sec-high, taking it.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Should be in 38.8.0

Comment 29

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/bf34b97757b3