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

VERIFIED FIXED in Firefox 45

Status

()

defect
--
critical
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: decoder, Assigned: jonco)

Tracking

(Blocks 2 bugs, 6 keywords)

Dependency tree / graph

Firefox Tracking Flags

(firefox44 wontfix, firefox45+ verified, firefox46+ verified, firefox47+ verified, firefox-esr3846+ fixed)

Details

(Whiteboard: [jsbugmon:update][adv-main45+][adv-esr38.8+])

Attachments

(3 attachments)

Reporter

Description

3 years ago
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()>

Updated

3 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

3 years ago
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
Posted file OOM_VERBOSE=1 stack
I have attached a OOM_VERBOSE=1 stack.

Any idea how best to move this forward, Jon?
Flags: needinfo?(jcoppeard)
Assignee

Comment 3

3 years ago
Looks like an out of bounds buffer access on OOM.
Assignee: nobody → jcoppeard
Group: javascript-core-security
Flags: needinfo?(jcoppeard)
Assignee

Comment 4

3 years ago
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.
Attachment #8711629 - Flags: review?(sphink)
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.
Attachment #8711629 - Flags: review?(sphink) → review+
Assignee

Comment 6

3 years ago
(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?
Assignee

Comment 7

3 years ago
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.
Attachment #8711629 - Flags: sec-approval?
(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 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.
Attachment #8711629 - Flags: sec-approval? → sec-approval+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][checkin on 2/9]

Updated

3 years ago
Whiteboard: [jsbugmon:update][checkin on 2/9] → [checkin on 2/9] [jsbugmon:update,ignore]

Comment 10

3 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 2b73b0a4d52b).
Target Milestone: --- → mozilla47
Assignee

Comment 12

3 years ago
(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.
This was pushed early, see comment 11 and comment 12.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Whiteboard: [checkin on 2/9] [jsbugmon:update,ignore] → [jsbugmon:update]

Updated

3 years ago
Status: RESOLVED → VERIFIED

Comment 14

3 years ago
JSBugMon: This bug has been automatically verified fixed.
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.
Flags: needinfo?(abillings)
Can you request uplift approval please?
Flags: needinfo?(choller)
Might as well checkin now. I doubt we'll zero day ourselves further than we already have.
Flags: needinfo?(abillings)
(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.
Flags: needinfo?(choller) → needinfo?(jcoppeard)
Assignee

Comment 19

3 years ago
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.
Flags: needinfo?(jcoppeard)
Attachment #8711629 - Flags: approval-mozilla-beta?
Attachment #8711629 - Flags: approval-mozilla-aurora?
Comment on attachment 8711629 [details] [diff] [review]
bug1241731-structured-clone-oom

Fix for sec regression, please uplift to aurora and beta.
Attachment #8711629 - Flags: approval-mozilla-beta?
Attachment #8711629 - Flags: approval-mozilla-beta+
Attachment #8711629 - Flags: approval-mozilla-aurora?
Attachment #8711629 - Flags: approval-mozilla-aurora+
Group: javascript-core-security → core-security-release

Comment 23

3 years ago
JSBugMon: This bug has been automatically verified fixed on Fx45
JSBugMon: This bug has been automatically verified fixed on Fx46
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!
Flags: needinfo?(jcoppeard)
This never got ready for esr38.7. Tracked for esr38.8
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main45+]
Assignee

Comment 26

3 years ago
[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
Flags: needinfo?(jcoppeard)
Attachment #8727422 - Flags: approval-mozilla-esr38?
Comment on attachment 8727422 [details] [diff] [review]
bug1241731-esr38

sec-high, taking it.
Attachment #8727422 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Should be in 38.8.0
Whiteboard: [jsbugmon:update][adv-main45+] → [jsbugmon:update][adv-main45+][adv-esr38.8+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.