<a class="header-button" href="https://bugzilla-dev.allizom.org/home" title="Go to home page"> Bugzilla

Reporter

Comment 1

•

5 years ago

Attached file worker.js (deleted) — Details

Reporter

Comment 2

•

5 years ago

Attached file prefs.js (deleted) — Details

Reporter

Comment 3

•

5 years ago

The attached testcase bisects back to the original implementation of Blob.stream() which was introduced in bug 1557121.

Flags: needinfo?(amarchesini)

Daniel Veditz [:dveditz]

Updated

•

5 years ago

Group: core-security → dom-core-security

Keywords: csectype-uaf, sec-high

Comment 4

•

5 years ago

It seems (though I have no proof), that during the same call to:
#3 0x7fc3444ebd18 in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3584:3
first a SnowWhiteKiller deletes some object without removing it from the mJSHolders Vectror of JSHolderInfo (containing raw pointers):
#1 0x7fc3444e3876 in SnowWhiteKiller::~SnowWhiteKiller() /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2416:7
then mCCJSRuntime->TraverseRoots(*mBuilder); is iterating that vector and crashes:
#2 0x7fc3444a70ee in mozilla::CycleCollectedJSRuntime::TraverseNativeRoots(nsCycleCollectionNoteRootCallback&) /builds/worker/workspace/build/src/xpcom/base/CycleCollectedJSRuntime.cpp:797:15

But I am not sure, if the expected behavior of the SnowWhiteKiller in nsCycleCollector.cpp should be to keep in sync the private vector mJSHolders in CycleCollectedRuntime.cpp, though. This might need some further investigation.

Andrea Marchesini [:baku]

Updated

•

5 years ago

Flags: needinfo?(peterv)

Comment 5

•

5 years ago

This bug is a dup of bug 1571037.

Status: NEW → RESOLVED

Closed: 5 years ago

Flags: needinfo?(peterv)

Flags: needinfo?(amarchesini)

Resolution: --- → DUPLICATE

Andrea Marchesini [:baku]

Reporter

Comment 6

•

5 years ago

This doesn't appear to be the same bug as the attached testcase still reproduces this issue using the latest nightly (6b93a83735ed).

Flags: needinfo?(amarchesini)

Comment 7

•

5 years ago

Tyson, do you think it's possible to record this too?

Flags: needinfo?(amarchesini) → needinfo?(twsmith)

Tyson Smith [:tsmith]

Updated

•

5 years ago

Flags: needinfo?(twsmith)

Tyson Smith [:tsmith]

Updated

•

5 years ago

Status: RESOLVED → REOPENED

Resolution: DUPLICATE → ---

Tyson Smith [:tsmith]

Comment 8

•

5 years ago

Here is a trace from a debug build:
BuildID=20190919094654
SourceStamp=6b93a83735ed3ab3b57b46c1b768814b1a1af5d6

Assertion failure: T::cycleCollection::CheckForRightISupports(s) (not the nsISupports pointer we expect), at /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionParticipant.h:357

rax = 0x000055e9d27f5340   rdx = 0x0000000000000000
rcx = 0x00007f9bd006758e   rbx = 0x00007f9ba4fbf310
rsi = 0x00007f9bdcfd5770   rdi = 0x00007f9bdcfd4540
rbp = 0x00007f9bc03fdf70   rsp = 0x00007f9bc03fdf60
r8 = 0x00007f9bdcfd5770    r9 = 0x00007f9bc03ff700
r10 = 0x0000000000000000   r11 = 0x0000000000000000
r12 = 0x00007f9bc03fdfd8   r13 = 0x00007f9bc03fdfd7
r14 = 0x00007f9bd2badf60   r15 = 0x00007f9bd2badf60
rip = 0x00007f9bccc44cfc
OS|Linux|0.0.0 Linux 4.15.0-64-generic #73~16.04.1-Ubuntu SMP Fri Sep 13 09:56:18 UTC 2019 x86_64
CPU|amd64|family 6 model 158 stepping 9|1
GPU|||
Crash|SIGSEGV|0x0|33
33|0|libxul.so|DowncastCCParticipantImpl<mozilla::dom::(anonymous namespace)::BlobBodyStreamHolder, true>::Run|hg:hg.mozilla.org/mozilla-central:xpcom/base/nsCycleCollectionParticipant.h:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|356|0x32
33|1|libxul.so|BlobBodyStreamHolder::cycleCollection::Trace|hg:hg.mozilla.org/mozilla-central:dom/file/Blob.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|326|0x24
33|2|libxul.so|mozilla::CycleCollectedJSRuntime::TraverseNativeRoots(nsCycleCollectionNoteRootCallback&)|hg:hg.mozilla.org/mozilla-central:xpcom/base/CycleCollectedJSRuntime.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|799|0xb
33|3|libxul.so|mozilla::CycleCollectedJSRuntime::TraverseRoots(nsCycleCollectionNoteRootCallback&)|hg:hg.mozilla.org/mozilla-central:xpcom/base/CycleCollectedJSRuntime.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|1138|0x5
33|4|libxul.so|nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*)|hg:hg.mozilla.org/mozilla-central:xpcom/base/nsCycleCollector.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|3611|0x15
33|5|libxul.so|nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool)|hg:hg.mozilla.org/mozilla-central:xpcom/base/nsCycleCollector.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|3413|0xf
33|6|libxul.so|nsCycleCollector_collect(nsICycleCollectorListener*)|hg:hg.mozilla.org/mozilla-central:xpcom/base/nsCycleCollector.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|3949|0x1e
33|7|libxul.so|js::gc::GCRuntime::maybeCallGCCallback(JSGCStatus)|hg:hg.mozilla.org/mozilla-central:js/src/gc/GC.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|7060|0xb
33|8|libxul.so|js::gc::GCRuntime::gcCycle(bool, js::SliceBudget, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason)|hg:hg.mozilla.org/mozilla-central:js/src/gc/GC.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|7089|0xd
33|9|libxul.so|js::gc::GCRuntime::collect(bool, js::SliceBudget, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason)|hg:hg.mozilla.org/mozilla-central:js/src/gc/GC.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|7321|0x1d
33|10|libxul.so|js::gc::GCRuntime::gc(JSGCInvocationKind, JS::GCReason)|hg:hg.mozilla.org/mozilla-central:js/src/gc/GC.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|7403|0x24
33|11|libxul.so|WorkerThreadPrimaryRunnable::Run|hg:hg.mozilla.org/mozilla-central:dom/workers/RuntimeService.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|2342|0x5
33|12|libxul.so|nsThread::ProcessNextEvent(bool, bool*)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|1225|0x15
33|13|libxul.so|NS_ProcessNextEvent(nsIThread*, bool)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|486|0x11
33|14|libxul.so|mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|333|0xd
33|15|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|315|0x17
33|16|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|290|0x8
33|17|libxul.so|nsThread::ThreadFunc(void*)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|458|0x38
33|18|libnspr4.so|_pt_root|hg:hg.mozilla.org/mozilla-central:nsprpub/pr/src/pthreads/ptthread.c:6b93a83735ed3ab3b57b46c1b768814b1a1af5d6|198|0x7
33|19|libpthread-2.23.so|start_thread|/build/glibc-LK5gWL/glibc-2.23/nptl/pthread_create.c|333|0x11
33|20|libc-2.23.so|clone|||0x6d

Comment 9

•

5 years ago

Sorry for my probably very incomplete knowledge of our toolchain, but is there a way to see (always regarding Comment 4) if it is actually the same call to BeginCollection? It is definitely the same Thread and the same stack trace up to that call, but it might be always different moments in time. Do we have a possibility to see this (from timestamps, log information or whatever) ?

But in the end probably it does not even make a real difference: The memory has been freed by that same function in that same thread, so I would exclude strange side effects and look straight into the implementation of the SnowWhiteKiller and surroundings also in the implementing classes.

Comment 10

•

5 years ago

The debug information just tells us, that we have a BlobBodyStreamHolder object, that gets corrupted:

326 CRASH--> NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(BlobBodyStreamHolder,
                                               BodyStreamHolder)
  NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mStream)
NS_IMPL_CYCLE_COLLECTION_TRACE_END

The BlobBodyStreamHolder is held as a RefPtr by a BodyStream (mStreamHolder), which is in turn held by Blob, which itself is a Cycle Collector participant.

I assume, this nested situation leads to a situation, in which:

CycleCollector collects all potential objects in a vector
while looking on them, the Blob is evicted
This removes (obscure to the cycle collector) also the nested BlobBodyStreamHolder
which then the Cycle Collector wants to remove

As I am new to Cycle Collectors: Is it a foreseen usage, to nest those Cycle Collector participants and to delete them through the owner rather than leave deletion to the Collector itself?

Updated

•

5 years ago

Component: DOM: Workers → DOM: File

Comment 11

•

5 years ago

Attached file BlobBodyStreamHolderSnippet (deleted) — Details

Hmmm, I should read better the provided information: Assertion failure: T::cycleCollection::CheckForRightISupports(s) (not the nsISupports pointer we expect), at /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionParticipant.h:357 This would indicate, that there is simply something wrong with the way BlobBodyStreamHolder has been defined: https://bugzilla.mozilla.org/attachment.cgi?id=9094180

Comment 12

•

5 years ago

Probably BlobBodyStreamHolder should not just declare NS_DECL_ISUPPORTS_INHERITED but a cycle collector variant one? Which?

Peter Van der Beken [:peterv]

Updated

•

5 years ago

Flags: needinfo?(bugmail)

Comment 13

•

5 years ago

Actually, I don't see anything wrong with the CC macros. BlobBodyStreamHolder uses NS_DECL_ISUPPORTS_INHERITED and NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED, which should be fine. It might need to |mozilla::DropJSObjects(this);| in its destructor and unlink though.

Comment 14

•

5 years ago

(In reply to Peter Van der Beken [:peterv] from comment #14)

Actually, I don't see anything wrong with the CC macros. BlobBodyStreamHolder uses NS_DECL_ISUPPORTS_INHERITED and NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED, which should be fine. It might need to |mozilla::DropJSObjects(this);| in its destructor and unlink though.

Thanks for that hint! In fact, the unlink function and the destructor look incomplete.

Assignee

Updated

•

5 years ago

Assignee: nobody → shes050117

Status: REOPENED → ASSIGNED

Updated

•

5 years ago

Priority: -- → P1

[1] https://searchfox.org/mozilla-central/rev/45f30e1d19bde27bf07e47a0a5dd0962dd27ba18/dom/file/Blob.cpp#304

Assignee

Comment 15

•

5 years ago

(In reply to Peter Van der Beken [:peterv] from comment #14)

Actually, I don't see anything wrong with the CC macros. BlobBodyStreamHolder uses NS_DECL_ISUPPORTS_INHERITED and NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED, which should be fine. It might need to |mozilla::DropJSObjects(this);| in its destructor and unlink though.

Apply this comment can fix the issue. However, I also found in BlobBodyStreamHolder::NullifyStream() [1] would call that as well and it's called by BodyStream::ReleaseObjects(). I wonder if something (XHR) blocks the normal calls for cleaning up/releasing objects and thus we have UAF in this case. I need to dig into the code path more.

Comment 16

•

5 years ago

I would strongly recommend to implement in any case the expected unlink functions directly into the holder object and not to rely on BodyStream::ReleaseObjects(), because I suspect, that the order of deletion might not be in a way, that BodyStream::ReleaseObjects() is ever called before trying to delete the Holder through the Cycle Collector. It might be a good idea, though, to guide all flows to the same implementing function (from the holder's destructor and the BodyStream::ReleaseObjects()/BlobBodyStreamHolder::NullifyStream() path), which could be directly the unlink function implemented by the macro (and with the right content).

[1] https://searchfox.org/mozilla-central/rev/45f30e1d19bde27bf07e47a0a5dd0962dd27ba18/dom/base/BodyStream.cpp#115
[2] https://searchfox.org/mozilla-central/rev/f43ae7e1c43a4a940b658381157a6ea6c5a185c1/dom/file/Blob.cpp#362

Assignee

Comment 17

•

5 years ago

So, the issue caught by this test is:

It failed at the BodyStream::Create() [1]
BlobBodyStreamHolder tells CC it holds a JSObject in [2] (before [1])
Thus, BodyStream is deleted right after BodyStream::Create() (because it fails and doesn't call the addref)
And thus, BlobBodyStreamHolder is called after that (ref count is zero; on snow white)
The second access is probably when CC check the registered object and then UAF

Normally, BodyStream::Close() would be called (and thus BlodyStream::ReleaseObject(), NullifyStream(), DropJSObject would be called) before it's deleted. However, in the case here, the BlodyStream::ReleaseObject() was not called before it got destructed.

To fix this, we should clean up (calling Close()) when we fail on the creation of the BodyStream. I'll update a patch for that.

Flags: needinfo?(bugmail)

Assignee

Comment 18

•

5 years ago

(In reply to Jens Stutte [:jstutte] from comment #17)

I would strongly recommend to implement in any case the expected unlink functions directly into the holder object and not to rely on BodyStream::ReleaseObjects(), because I suspect, that the order of deletion might not be in a way, that BodyStream::ReleaseObjects() is ever called before trying to delete the Holder through the Cycle Collector. It might be a good idea, though, to guide all flows to the same implementing function (from the holder's destructor and the BodyStream::ReleaseObjects()/BlobBodyStreamHolder::NullifyStream() path), which could be directly the unlink function implemented by the macro (and with the right content).

In this case, the unlink function for the BlobBodyStreamHolder wasn't called. So, if it's what we suppose to do, we should open another bug for that.

Assignee

Comment 19

•

5 years ago

Attached file Bug 1577311 - Close the stream if there is a failure during the creation; r=baku (deleted) — Details

Assignee

Comment 20

•

5 years ago

I could reproduce the issue on my desktop. After applying the patch I attached, I cannot hit the issue by the provided testcase.

Assignee

Comment 21

•

5 years ago

(In reply to Tom Tung [:tt, :ttung] from comment #19)

In this case, the unlink function for the BlobBodyStreamHolder wasn't called. So, if it's what we suppose to do, we should open another bug for that.

Talked with Jens this morning, we suspect the reason for that is probably because the SnowWhiteKiller doesn't have any way to understand these stuff. Thus the holder is killed and its work inside unlink is not called.

Also, we should probably call DropJSObjects on the holder's destructor to avoid another edge case in the future. And we have checked DropJSObjects is safe to be called multiple times.

The only thing we are not so sure about is that usage for also calling DropJSObjects on unlink function if we will do that on the destructor.

(In reply to Peter Van der Beken [:peterv] from comment #14)

Actually, I don't see anything wrong with the CC macros. BlobBodyStreamHolder uses NS_DECL_ISUPPORTS_INHERITED and NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED, which should be fine. It might need to |mozilla::DropJSObjects(this);| in its destructor and unlink though.

Peter, could you shed the light on the usage for doing that in unlink? My question is more like why it's not enough for calling that on destructor. Would it happen like we need to DropJSObjects at the point of unlink? Thank you in advance!

Flags: needinfo?(peterv)

Phabricator Automation

Updated

•

5 years ago

Attachment #9095244 - Attachment description: Bug 1577311 - Close the stream if the creation fail; → Bug 1577311 - Close the stream if there is a failure during the creation;

Assignee

Comment 22

•

5 years ago

Attached file Bug 1577311 - Ensure the js object is dropped while the holder is unlinked and destructored; (deleted) — Details

Depends on D47118

Al Billings [:abillings - ex-MoCo]

Assignee

Comment 23

•

5 years ago

Comment on attachment 9095244 [details]
Bug 1577311 - Close the stream if there is a failure during the creation; r=baku

Security Approval Request

How easily could an exploit be constructed based on the patch?: The attacker might understand it's probably related to not close the stream on the creation, but they cannot understand the way for making that fail on the creation.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
Which older supported branches are affected by this flaw?: 69
If not all supported branches, which bug introduced the flaw?: Bug 1557121
Do you have backports for the affected branches?: No
If not, how different, hard to create, and risky will they be?: Not really sure about the answer to this. Again, the attacker might understand it's probably related to not close the stream on the creation, but they cannot understand the way for making that fail on the creation
How likely is this patch to cause regressions; how much testing does it need?: The code path itself is for the failure cases and it only close the stream on the failure cases. So, I guess it should be hard.

Attachment #9095244 - Flags: sec-approval?

Comment 24

•

5 years ago

sec-approval+ for trunk. We'll want this nominated for beta as well.

status-firefox69: --- → wontfix

status-firefox71: --- → affected

tracking-firefox70: --- → +

tracking-firefox71: --- → +

Al Billings [:abillings - ex-MoCo]

Updated

•

5 years ago

Attachment #9095244 - Flags: sec-approval? → sec-approval+

Assignee

Comment 25

•

5 years ago

Jason, could you help me to verify if P1 solves the issue? It works on my machine. Thanks!

Flags: needinfo?(jkratzer)

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Assignee

Comment 26

•

5 years ago

Land Patch 1 first since I cannot reproduce the issue once I apply that.

Keywords: leave-open

Comment 27

•

5 years ago

Initial landing: https://hg.mozilla.org/integration/autoland/rev/7d1f6ed3ccf56db2f558bd8aaf352543122e09a2

Backed out for build bustage in dom/file/Blob.cpp::

https://hg.mozilla.org/integration/autoland/rev/9ffde0fb448b092c0b931610ee26d26e0ddb30a7

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=269255700&resultStatus=testfailed%2Cbusted%2Cexception%2Csuperseded%2Cretry%2Cusercancel&revision=7d1f6ed3ccf56db2f558bd8aaf352543122e09a2
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=269251355&repo=autoland

[task 2019-10-01T17:08:45.937Z] 17:08:45 ERROR - /builds/worker/workspace/build/src/dom/file/Blob.cpp:362:41: error: variable of type 'mozilla::dom::(anonymous namespace)::BlobBodyStreamHolder' only valid on the stack
[task 2019-10-01T17:08:45.937Z] 17:08:45 INFO - RefPtr<BlobBodyStreamHolder> holder = new BlobBodyStreamHolder();
[task 2019-10-01T17:08:45.937Z] 17:08:45 INFO - ^
[task 2019-10-01T17:08:45.937Z] 17:08:45 INFO - /builds/worker/workspace/build/src/dom/file/Blob.cpp:362:41: note: value incorrectly allocated on the heap
[task 2019-10-01T17:08:45.937Z] 17:08:45 INFO - /builds/worker/workspace/build/src/dom/file/Blob.cpp:295:7: note: 'mozilla::dom::(anonymous namespace)::BlobBodyStreamHolder' is a stack type because it inherits from a stack type 'mozilla::dom::BodyStreamHolder'
[task 2019-10-01T17:08:45.938Z] 17:08:45 INFO - class BlobBodyStreamHolder final : public BodyStreamHolder {
[task 2019-10-01T17:08:45.939Z] 17:08:45 INFO - ^
[task 2019-10-01T17:08:45.939Z] 17:08:45 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BodyStream.h:55:19: note: 'mozilla::dom::BodyStreamHolder' is a stack type because member 'mStreamCreated' is a stack type 'DebugOnly<bool>'
[task 2019-10-01T17:08:45.939Z] 17:08:45 INFO - DebugOnly<bool> mStreamCreated = false;
[task 2019-10-01T17:08:45.939Z] 17:08:45 INFO - ^
[task 2019-10-01T17:08:45.939Z] 17:08:45 INFO - 1 error generated.

Flags: needinfo?(ttung)

Reporter

Comment 28

•

5 years ago

(In reply to Tom Tung [:tt, :ttung] from comment #26)

Jason, could you help me to verify if P1 solves the issue? It works on my machine. Thanks!

I can confirm that this issue no longer reproduces using the patch in comment 24.

Flags: needinfo?(jkratzer)

Phabricator Automation

Updated

•

5 years ago

Attachment #9095244 - Attachment description: Bug 1577311 - Close the stream if there is a failure during the creation; → Bug 1577311 - Close the stream if there is a failure during the creation; r=baku

Peter Van der Beken [:peterv]

Assignee

Comment 29

•

5 years ago

Fix the DebugOnly<> issue

Flags: needinfo?(ttung)

Comment 30

•

5 years ago

(In reply to Peter Van der Beken [:peterv] from comment #14)

Actually, I don't see anything wrong with the CC macros. BlobBodyStreamHolder uses NS_DECL_ISUPPORTS_INHERITED and NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED, which should be fine. It might need to |mozilla::DropJSObjects(this);| in its destructor and unlink though.

Peter, could you shed the light on the usage for doing that in unlink? My question is more like why it's not enough for calling that on destructor. Would it happen like we need to DropJSObjects at the point of unlink? Thank you in advance!

Not doing it in unlink might lead to leaks, the easiest to understand case is that there's a cycle between the BlobBodyStreamHolder and a JS object, with one edge from BlobBodyStreamHolder to the JS object and one edge from the JS object to the BlobBodyStreamHolder. If you don't DropJSObjects in unlink, then the JS object will not be collected by the JS GC (because the JS holder, the BlobBodyStreamHolder in this case, still has a reference to it). In turn, we will not release the BlobBodyStreamHolder because it's held alive by a JS object. So the destructor for BlobBodyStreamHolder might not even be run in this case.

Note that the cycles and edges involved might be much more complicated, but incomplete unlinking can definitely lead to leaks.

Flags: needinfo?(peterv)

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 31

•

5 years ago

https://hg.mozilla.org/integration/autoland/rev/9898a46a96539cf58de81a2be6bacd9ee4b761b1
https://hg.mozilla.org/mozilla-central/rev/9898a46a9653

Group: dom-core-security → core-security-release

Status: ASSIGNED → RESOLVED

Closed: 5 years ago → 5 years ago

status-firefox71: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla71

BugBot [:suhaib / :marco/ :calixte]

Updated

•

5 years ago

Keywords: leave-open

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Updated

•

5 years ago

Group: core-security-release → dom-core-security

Status: RESOLVED → REOPENED

status-firefox71: fixed → affected

Keywords: leave-open

Resolution: FIXED → ---

Target Milestone: mozilla71 → ---

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

status-firefox67: --- → unaffected

status-firefox68: --- → unaffected

status-firefox-esr60: --- → unaffected

status-firefox-esr68: --- → unaffected

Regressed by: 1557121

BMO Automation

Updated

•

5 years ago

Keywords: regression

Assignee

Comment 32

•

5 years ago

(In reply to Peter Van der Beken [:peterv] from comment #31)

Not doing it in unlink might lead to leaks, the easiest to understand case is that there's a cycle between the BlobBodyStreamHolder and a JS object, with one edge from BlobBodyStreamHolder to the JS object and one edge from the JS object to the BlobBodyStreamHolder. If you don't DropJSObjects in unlink, then the JS object will not be collected by the JS GC (because the JS holder, the BlobBodyStreamHolder in this case, still has a reference to it). In turn, we will not release the BlobBodyStreamHolder because it's held alive by a JS object. So the destructor for BlobBodyStreamHolder might not even be run in this case.

Note that the cycles and edges involved might be much more complicated, but incomplete unlinking can definitely lead to leaks.

Thanks for the explanation! That clear on thought and I will call DropJSObject in the unlink fucntion in D47218.

Phabricator Automation

Updated

•

5 years ago

Attachment #9095443 - Attachment description: Bug 1577311 - Ensure the js object is dropped while the holder is destructored; → Bug 1577311 - Ensure the js object is dropped while the holder is unlinked and destructored;

Assignee

Comment 33

•

5 years ago

Comment on attachment 9095443 [details]
Bug 1577311 - Ensure the js object is dropped while the holder is unlinked and destructored;

Security Approval Request

How easily could an exploit be constructed based on the patch?: This patch cleanup the link between the holder and the js object in unlink function and dtor of the holder. It's a safeguard to prevent leaking in the worst case in theory. So, it should be hard to attack based on this patch.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
Which older supported branches are affected by this flaw?: 69
If not all supported branches, which bug introduced the flaw?: Bug 1557121
Do you have backports for the affected branches?: No
If not, how different, hard to create, and risky will they be?: So this patch prevents the worst case in theory so it should be hard to attack.
How likely is this patch to cause regressions; how much testing does it need?: Shouldn't be since it only calls cleanup function in its dtor and unlink function

Attachment #9095443 - Flags: sec-approval?

Assignee

Comment 34

•

5 years ago

I will uplift these two patches to the beta once the second stay in m-c for more than two days without reporting any issue.

Tom Ritter [:tjr] (Back 9/5)

Assignee

Comment 35

•

5 years ago

(In reply to Jason Kratzer [:jkratzer] from comment #29)

(In reply to Tom Tung [:tt, :ttung] from comment #26)

Jason, could you help me to verify if P1 solves the issue? It works on my machine. Thanks!

I can confirm that this issue no longer reproduces using the patch in comment 24.

Thanks for the verify! (And sorry that I somehow missed this comment : (

Comment 36

•

5 years ago

Comment on attachment 9095443 [details]
Bug 1577311 - Ensure the js object is dropped while the holder is unlinked and destructored;

sec-approval+ for trunk and I'll copy Al and ask to please nominate for beta as well.

Attachment #9095443 - Flags: sec-approval? → sec-approval+

Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)

Comment 37

•

5 years ago

Can you request beta uplift please (and provide a rebased patch for beta if necessary?) Thanks!

Flags: needinfo?(ttung)

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 38

•

5 years ago

https://hg.mozilla.org/integration/autoland/rev/9e3d328e63acdba002c3f4b4305fec205857c0cb

Patch applies cleanly to beta.

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 39

•

5 years ago

https://hg.mozilla.org/mozilla-central/rev/9e3d328e63ac

Assignee

Comment 40

•

5 years ago

Comment on attachment 9095244 [details]
Bug 1577311 - Close the stream if there is a failure during the creation; r=baku

Beta/Release Uplift Approval Request

User impact if declined: UAF if run the script provided by the reporter
Is this code covered by automated tests?: No
Has the fix been verified in Nightly?: Yes
Needs manual test from QE?: No
If yes, steps to reproduce:
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): The patch calls the cleanup function while there is a failure in the creation of a Stream object. In general, this code path is not easy to be run (because, normally, it is not easy to have a failure during the creation). Note that this patch fixes the reported issue and confirmed by the reporter.

Another patch (Ensure the js ...) is a cleanup function to ensure there is no other similar case that is not covered by the previous patch. Note, it was pushed to m-c only for one day, but I think it should be safe to uplift.

String changes made/needed:

Flags: needinfo?(ttung)

Attachment #9095244 - Flags: approval-mozilla-beta?

Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)

Assignee

Updated

•

5 years ago

Attachment #9095443 - Flags: approval-mozilla-beta?

Comment 41

•

5 years ago

Comment on attachment 9095244 [details]
Bug 1577311 - Close the stream if there is a failure during the creation; r=baku

Fix for sec-high issue, OK for beta 14 uplift.

Attachment #9095244 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

Attachment #9095443 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 42

•

5 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/d18091b73cc4
https://hg.mozilla.org/releases/mozilla-beta/rev/a465465b5d98

Assignee

Updated

•

5 years ago

status-firefox70: affected → fixed

status-firefox71: affected → fixed

Keywords: leave-open

Tom Ritter [:tjr] (Back 9/5)

Assignee

Updated

•

5 years ago

Status: REOPENED → RESOLVED

Closed: 5 years ago → 5 years ago

Resolution: --- → FIXED

Updated

•

5 years ago

Whiteboard: [adv-main70+][adv-main70-rollup]

Tom Ritter [:tjr] (Back 9/5)

Updated

•

5 years ago

Whiteboard: [adv-main70+][adv-main70-rollup] → [adv-main70+][adv-main70+r]

Daniel Veditz [:dveditz]

Updated

•

5 years ago

Group: dom-core-security → core-security-release

Bogdan Maris, Desktop QA

Updated

•

5 years ago

Flags: qe-verify+

Whiteboard: [adv-main70+][adv-main70+r] → [adv-main70+][adv-main70+r][post-critsmash-triage]

Brindusa Tot, Desktop QA

Updated

•

5 years ago

QA Whiteboard: [qa-triaged]

Comment 43

•

5 years ago

Hey Tom,

Are there any steps to reproduce this issue and the expected result?
Based on what is here so far I loaded the testcase on a local webserver, opened it in Firefox and expected it to crash after 1-2 minutes but nothing happened. I'm also getting an error in cmd so not quite sure if I can verify this fix..

Flags: needinfo?(ttung)

Assignee

Comment 44

•

5 years ago

(In reply to Timea Cernea [:tbabos] from comment #44)

Hey Tom,

Are there any steps to reproduce this issue and the expected result?
Based on what is here so far I loaded the testcase on a local webserver, opened it in Firefox and expected it to crash after 1-2 minutes but nothing happened. I'm also getting an error in cmd so not quite sure if I can verify this fix..

Hi Timea,

IIRC, if the web page doesn't crashing, then the fix is verified. Before the fixed is landed, it crashed immedicately on my machine.

Flags: needinfo?(ttung)

Comment 45

•

5 years ago

That sounds good, but I can't reproduce the crash on affected Nightly 69 either.
If you could find the time to verify Nightly and Beta for uplift, that would be great. I can't vouch for the fix if I can't reproduce it on affected builds :(

Flags: needinfo?(ttung)

Assignee

Comment 46

•

5 years ago

(In reply to Timea Cernea [:tbabos] from comment #46)

That sounds good, but I can't reproduce the crash on affected Nightly 69 either.

Did you enable the fuzzy mode? (ac_add_options --enable-fuzzing)

The testcase is 100% reproducible to me while I was testing (both Mac Book Pro and Linux machine). And I used an ASAN build with enable fuzzy mode.

I put the command I ran below:
python -m ffpuppet -p prefs.js --xvfb -d -l log ~/Work/mozilla-central/objdir-ff-asan/dist/bin/firefox -u http://localhost:8000/testcase.html

If you are curious about ffpuppet you can go to this https://github.com/MozillaSecurity/ffpuppet

Please let me know if you need more information.

If you could find the time to verify Nightly and Beta for uplift, that would be great. I can't vouch for the fix if I can't reproduce it on affected builds :(

Could you elaborate more? I guess you are asking for the reversion number? I put that below:

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #43)

https://hg.mozilla.org/releases/mozilla-beta/rev/d18091b73cc4
https://hg.mozilla.org/releases/mozilla-beta/rev/a465465b5d98

Flags: needinfo?(ttung) → needinfo?(tbabos)

Updated

•

5 years ago

Flags: needinfo?(tbabos)