1456189 - AddressSanitizer: bad-free deserializing JSStructuredCloneData

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Alex Gaynor [:Alex_Gaynor]]

Attachment: crash-b206d606710f01eb5cfbe5b53ad5efefe8984039

This was produced with my IPC fuzzer from bug 1451859. This consistently reproduces with the attached input, when run as:

(cd obj-x86_64-pc-linux-gnu/dist/bin; MOZ_RUN_GTEST=1 LIBFUZZER=1 FUZZER=ContentParentIPC ASAN_SYMBOLIER_PATH=/home/osboxes/llvm-symbolizer ./firefox ./crash-path -rss_limit_mb=0)

==5256==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x0ffff4d6f828 in thread T0
    #0 0x4e3520 in __interceptor_cfree.localalias.0 (/home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox+0x4e3520)
    #1 0x7fcebd94e1db in InfallibleAllocPolicy::free_(void*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/mozalloc.h:269:9
    #2 0x7fcebd94e1db in mozilla::BufferList<InfallibleAllocPolicy>::Clear() /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/BufferList.h:153
    #3 0x7fcebd94e1db in mozilla::BufferList<InfallibleAllocPolicy>::~BufferList() /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/BufferList.h:124
    #4 0x7fcebd94e1db in mozilla::BufferList<InfallibleAllocPolicy>::Extract(mozilla::BufferList<InfallibleAllocPolicy>::IterImpl&, unsigned long, bool*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/BufferList.h:582
    #5 0x7fcebd941bd8 in Pickle::ExtractBuffers(PickleIterator*, unsigned long, mozilla::BufferList<InfallibleAllocPolicy>*, unsigned int) const /home/osboxes/mozilla-central/ipc/chromium/src/base/pickle.cc:433:50
    #6 0x7fcebdba6aad in IPC::ParamTraits<JSStructuredCloneData>::Read(IPC::Message const*, PickleIterator*, JSStructuredCloneData*) /home/osboxes/mozilla-central/ipc/glue/IPCMessageUtils.h:863:26
    #7 0x7fcebdb2fe40 in bool IPC::ReadParam<JSStructuredCloneData>(IPC::Message const*, PickleIterator*, JSStructuredCloneData*) /home/osboxes/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:143:10
    #8 0x7fcebdb2fe40 in IPC::ParamTraits<mozilla::SerializedStructuredCloneBuffer>::Read(IPC::Message const*, PickleIterator*, mozilla::SerializedStructuredCloneBuffer*) /home/osboxes/mozilla-central/ipc/glue/IPCMessageUtils.h:892
    #9 0x7fcebdb2fe40 in bool mozilla::ipc::IPDLParamTraits<mozilla::SerializedStructuredCloneBuffer>::Read<mozilla::SerializedStructuredCloneBuffer>(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::SerializedStructuredCloneBuffer*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/IPDLParamTraits.h:43
    #10 0x7fcebdb2fe40 in bool mozilla::ipc::ReadIPDLParam<mozilla::SerializedStructuredCloneBuffer>(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::SerializedStructuredCloneBuffer*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/IPDLParamTraits.h:74
    #11 0x7fcebdb2fe40 in mozilla::ipc::IPDLParamTraits<mozilla::dom::ClonedMessageData>::Read(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::dom::ClonedMessageData*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/DOMTypes.cpp:168
    #12 0x7fcebddc77e2 in bool mozilla::ipc::ReadIPDLParam<mozilla::dom::ClonedMessageData>(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::dom::ClonedMessageData*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/IPDLParamTraits.h:74:10
    #13 0x7fcebddc77e2 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/PContentParent.cpp:5260
    #14 0x7fcecd2558bb in void mozilla::ipc::FuzzProtocol<mozilla::dom::ContentParent>(mozilla::dom::ContentParent*, unsigned char const*, unsigned long, std::unordered_set<unsigned int, std::hash<unsigned int>, std::equal_to<unsigned int>, std::allocator<unsigned int> >&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/ProtocolFuzzer.h:48:18
    #15 0x7fcecd2551b8 in RunContentParentIPCFuzzing(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/dom/ipc/fuzztest/content_parent_ipc_libfuzz.cpp:59:3
    #16 0x5e654b in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
    #17 0x5be1a4 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:280:6
    #18 0x5ca4b0 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:703:9
    #19 0x7fcecb84b16f in mozilla::FuzzerRunner::Run(int*, char***) /home/osboxes/mozilla-central/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10
    #20 0x7fcecb75d77a in XREMain::XRE_mainStartup(bool*) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4028:35
    #21 0x7fcecb7721af in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4964:12
    #22 0x7fcecb773e6a in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:5071:21
    #23 0x51e145 in do_main(int, char**, char**) /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:231:22
    #24 0x51e145 in main /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:304
    #25 0x7fcee15692e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)
    #26 0x425559 in _start (/home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox+0x425559)

Address 0x0ffff4d6f828 is located in the high shadow area.
SUMMARY: AddressSanitizer: bad-free (/home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox+0x4e3520) in __interceptor_cfree.localalias.0


Staring at the code a bit, I don't immediately recognize the cause.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

I am occasionally seeing (what I believe is) this same bug reported as a double free, hopefully the stacks here should improve the debug-ability:

==8670==ERROR: AddressSanitizer: attempting double-free on 0x602000046b70 in thread T0:
    #0 0x4e3520 in __interceptor_cfree.localalias.0 (/home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox+0x4e3520)
    #1 0x7f7e64bf017b in InfallibleAllocPolicy::free_(void*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/mozalloc.h:269:9
    #2 0x7f7e64bf017b in mozilla::BufferList<InfallibleAllocPolicy>::Clear() /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/BufferList.h:153
    #3 0x7f7e64bf017b in mozilla::BufferList<InfallibleAllocPolicy>::~BufferList() /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/BufferList.h:124
    #4 0x7f7e64bf017b in mozilla::BufferList<InfallibleAllocPolicy>::Extract(mozilla::BufferList<InfallibleAllocPolicy>::IterImpl&, unsigned long, bool*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/BufferList.h:582
    #5 0x7f7e64be3beb in Pickle::ExtractBuffers(PickleIterator*, unsigned long, mozilla::BufferList<InfallibleAllocPolicy>*, unsigned int) const /home/osboxes/mozilla-central/ipc/chromium/src/base/pickle.cc:433:50
    #6 0x7f7e64e48a4d in IPC::ParamTraits<JSStructuredCloneData>::Read(IPC::Message const*, PickleIterator*, JSStructuredCloneData*) /home/osboxes/mozilla-central/ipc/glue/IPCMessageUtils.h:863:26
    #7 0x7f7e64dd1de0 in bool IPC::ReadParam<JSStructuredCloneData>(IPC::Message const*, PickleIterator*, JSStructuredCloneData*) /home/osboxes/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:143:10
    #8 0x7f7e64dd1de0 in IPC::ParamTraits<mozilla::SerializedStructuredCloneBuffer>::Read(IPC::Message const*, PickleIterator*, mozilla::SerializedStructuredCloneBuffer*) /home/osboxes/mozilla-central/ipc/glue/IPCMessageUtils.h:892
    #9 0x7f7e64dd1de0 in bool mozilla::ipc::IPDLParamTraits<mozilla::SerializedStructuredCloneBuffer>::Read<mozilla::SerializedStructuredCloneBuffer>(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::SerializedStructuredCloneBuffer*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/IPDLParamTraits.h:43
    #10 0x7f7e64dd1de0 in bool mozilla::ipc::ReadIPDLParam<mozilla::SerializedStructuredCloneBuffer>(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::SerializedStructuredCloneBuffer*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/IPDLParamTraits.h:74
    #11 0x7f7e64dd1de0 in mozilla::ipc::IPDLParamTraits<mozilla::dom::ClonedMessageData>::Read(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::dom::ClonedMessageData*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/DOMTypes.cpp:168
    #12 0x7f7e65069782 in bool mozilla::ipc::ReadIPDLParam<mozilla::dom::ClonedMessageData>(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::dom::ClonedMessageData*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/IPDLParamTraits.h:74:10
    #13 0x7f7e65069782 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/PContentParent.cpp:5260
    #14 0x7f7e744f785b in void mozilla::ipc::FuzzProtocol<mozilla::dom::ContentParent>(mozilla::dom::ContentParent*, unsigned char const*, unsigned long, std::unordered_set<unsigned int, std::hash<unsigned int>, std::equal_to<unsigned int>, std::allocator<unsigned int> >&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/ProtocolFuzzer.h:48:18
    #15 0x7f7e744f7158 in RunContentParentIPCFuzzing(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/dom/ipc/fuzztest/content_parent_ipc_libfuzz.cpp:59:3
    #16 0x5e654b in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
    #17 0x5ea5d6 in fuzzer::Fuzzer::MinimizeCrashLoop(std::vector<unsigned char, fuzzer::fuzzer_allocator<unsigned char> > const&) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:791:7
    #18 0x5c3657 in fuzzer::MinimizeCrashInputInternalStep(fuzzer::Fuzzer*, fuzzer::InputCorpus*) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:461:6
    #19 0x5cb86a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:663:12
    #20 0x7f7e72aed10f in mozilla::FuzzerRunner::Run(int*, char***) /home/osboxes/mozilla-central/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10
    #21 0x7f7e729ff71a in XREMain::XRE_mainStartup(bool*) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4028:35
    #22 0x7f7e72a1414f in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4964:12
    #23 0x7f7e72a15e0a in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:5071:21
    #24 0x51e145 in do_main(int, char**, char**) /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:231:22
    #25 0x51e145 in main /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:304
    #26 0x7f7e8880b2e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)
    #27 0x425559 in _start (/home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox+0x425559)

0x602000046b70 is located 0 bytes inside of 8-byte region [0x602000046b70,0x602000046b78)
freed by thread T0 here:
    #0 0x4e3520 in __interceptor_cfree.localalias.0 (/home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox+0x4e3520)
    #1 0x7f7e64bf017b in InfallibleAllocPolicy::free_(void*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/mozalloc.h:269:9
    #2 0x7f7e64bf017b in mozilla::BufferList<InfallibleAllocPolicy>::Clear() /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/BufferList.h:153
    #3 0x7f7e64bf017b in mozilla::BufferList<InfallibleAllocPolicy>::~BufferList() /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/BufferList.h:124
    #4 0x7f7e64bf017b in mozilla::BufferList<InfallibleAllocPolicy>::Extract(mozilla::BufferList<InfallibleAllocPolicy>::IterImpl&, unsigned long, bool*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/BufferList.h:582
    #5 0x7f7e64be3beb in Pickle::ExtractBuffers(PickleIterator*, unsigned long, mozilla::BufferList<InfallibleAllocPolicy>*, unsigned int) const /home/osboxes/mozilla-central/ipc/chromium/src/base/pickle.cc:433:50
    #6 0x7f7e64e48a4d in IPC::ParamTraits<JSStructuredCloneData>::Read(IPC::Message const*, PickleIterator*, JSStructuredCloneData*) /home/osboxes/mozilla-central/ipc/glue/IPCMessageUtils.h:863:26
    #7 0x7f7e64dd1de0 in bool IPC::ReadParam<JSStructuredCloneData>(IPC::Message const*, PickleIterator*, JSStructuredCloneData*) /home/osboxes/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:143:10
    #8 0x7f7e64dd1de0 in IPC::ParamTraits<mozilla::SerializedStructuredCloneBuffer>::Read(IPC::Message const*, PickleIterator*, mozilla::SerializedStructuredCloneBuffer*) /home/osboxes/mozilla-central/ipc/glue/IPCMessageUtils.h:892
    #9 0x7f7e64dd1de0 in bool mozilla::ipc::IPDLParamTraits<mozilla::SerializedStructuredCloneBuffer>::Read<mozilla::SerializedStructuredCloneBuffer>(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::SerializedStructuredCloneBuffer*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/IPDLParamTraits.h:43
    #10 0x7f7e64dd1de0 in bool mozilla::ipc::ReadIPDLParam<mozilla::SerializedStructuredCloneBuffer>(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::SerializedStructuredCloneBuffer*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/IPDLParamTraits.h:74
    #11 0x7f7e64dd1de0 in mozilla::ipc::IPDLParamTraits<mozilla::dom::ClonedMessageData>::Read(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::dom::ClonedMessageData*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/DOMTypes.cpp:168
    #12 0x7f7e65069782 in bool mozilla::ipc::ReadIPDLParam<mozilla::dom::ClonedMessageData>(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::dom::ClonedMessageData*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/IPDLParamTraits.h:74:10
    #13 0x7f7e65069782 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/PContentParent.cpp:5260
    #14 0x7f7e744f785b in void mozilla::ipc::FuzzProtocol<mozilla::dom::ContentParent>(mozilla::dom::ContentParent*, unsigned char const*, unsigned long, std::unordered_set<unsigned int, std::hash<unsigned int>, std::equal_to<unsigned int>, std::allocator<unsigned int> >&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/ProtocolFuzzer.h:48:18
    #15 0x7f7e744f7158 in RunContentParentIPCFuzzing(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/dom/ipc/fuzztest/content_parent_ipc_libfuzz.cpp:59:3
    #16 0x5e654b in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
    #17 0x5ea5d6 in fuzzer::Fuzzer::MinimizeCrashLoop(std::vector<unsigned char, fuzzer::fuzzer_allocator<unsigned char> > const&) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:791:7
    #18 0x5c3657 in fuzzer::MinimizeCrashInputInternalStep(fuzzer::Fuzzer*, fuzzer::InputCorpus*) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:461:6
    #19 0x5cb86a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:663:12
    #20 0x7f7e72aed10f in mozilla::FuzzerRunner::Run(int*, char***) /home/osboxes/mozilla-central/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10
    #21 0x7f7e729ff71a in XREMain::XRE_mainStartup(bool*) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4028:35
    #22 0x7f7e72a1414f in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4964:12
    #23 0x7f7e72a15e0a in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:5071:21
    #24 0x51e145 in do_main(int, char**, char**) /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:231:22
    #25 0x51e145 in main /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:304
    #26 0x7f7e8880b2e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)

previously allocated by thread T0 here:
    #0 0x4e36e8 in malloc (/home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox+0x4e36e8)
    #1 0x51f29d in moz_xmalloc /home/osboxes/mozilla-central/memory/mozalloc/mozalloc.cpp:70:17
    #2 0x7f7e64bfc39c in char* InfallibleAllocPolicy::pod_malloc<char>(unsigned long) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/mozalloc.h:249:32
    #3 0x7f7e64bfc39c in mozilla::BufferList<InfallibleAllocPolicy>::AllocateSegment(unsigned long, unsigned long) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/BufferList.h:375
    #4 0x7f7e64bef064 in mozilla::BufferList<InfallibleAllocPolicy>::BufferList(unsigned long, unsigned long, unsigned long, InfallibleAllocPolicy) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/BufferList.h:94:7
    #5 0x7f7e64bef064 in mozilla::BufferList<InfallibleAllocPolicy>::Extract(mozilla::BufferList<InfallibleAllocPolicy>::IterImpl&, unsigned long, bool*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/BufferList.h:530
    #6 0x7f7e64be3beb in Pickle::ExtractBuffers(PickleIterator*, unsigned long, mozilla::BufferList<InfallibleAllocPolicy>*, unsigned int) const /home/osboxes/mozilla-central/ipc/chromium/src/base/pickle.cc:433:50
    #7 0x7f7e64e48a4d in IPC::ParamTraits<JSStructuredCloneData>::Read(IPC::Message const*, PickleIterator*, JSStructuredCloneData*) /home/osboxes/mozilla-central/ipc/glue/IPCMessageUtils.h:863:26
    #8 0x7f7e64dd1de0 in bool IPC::ReadParam<JSStructuredCloneData>(IPC::Message const*, PickleIterator*, JSStructuredCloneData*) /home/osboxes/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:143:10
    #9 0x7f7e64dd1de0 in IPC::ParamTraits<mozilla::SerializedStructuredCloneBuffer>::Read(IPC::Message const*, PickleIterator*, mozilla::SerializedStructuredCloneBuffer*) /home/osboxes/mozilla-central/ipc/glue/IPCMessageUtils.h:892
    #10 0x7f7e64dd1de0 in bool mozilla::ipc::IPDLParamTraits<mozilla::SerializedStructuredCloneBuffer>::Read<mozilla::SerializedStructuredCloneBuffer>(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::SerializedStructuredCloneBuffer*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/IPDLParamTraits.h:43
    #11 0x7f7e64dd1de0 in bool mozilla::ipc::ReadIPDLParam<mozilla::SerializedStructuredCloneBuffer>(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::SerializedStructuredCloneBuffer*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/IPDLParamTraits.h:74
    #12 0x7f7e64dd1de0 in mozilla::ipc::IPDLParamTraits<mozilla::dom::ClonedMessageData>::Read(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::dom::ClonedMessageData*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/DOMTypes.cpp:168
    #13 0x7f7e65069782 in bool mozilla::ipc::ReadIPDLParam<mozilla::dom::ClonedMessageData>(IPC::Message const*, PickleIterator*, mozilla::ipc::IProtocol*, mozilla::dom::ClonedMessageData*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/IPDLParamTraits.h:74:10
    #14 0x7f7e65069782 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/PContentParent.cpp:5260
    #15 0x7f7e744f785b in void mozilla::ipc::FuzzProtocol<mozilla::dom::ContentParent>(mozilla::dom::ContentParent*, unsigned char const*, unsigned long, std::unordered_set<unsigned int, std::hash<unsigned int>, std::equal_to<unsigned int>, std::allocator<unsigned int> >&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/ProtocolFuzzer.h:48:18
    #16 0x7f7e744f7158 in RunContentParentIPCFuzzing(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/dom/ipc/fuzztest/content_parent_ipc_libfuzz.cpp:59:3
    #17 0x5e654b in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
    #18 0x5ea5d6 in fuzzer::Fuzzer::MinimizeCrashLoop(std::vector<unsigned char, fuzzer::fuzzer_allocator<unsigned char> > const&) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:791:7
    #19 0x5c3657 in fuzzer::MinimizeCrashInputInternalStep(fuzzer::Fuzzer*, fuzzer::InputCorpus*) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:461:6
    #20 0x5cb86a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:663:12
    #21 0x7f7e72aed10f in mozilla::FuzzerRunner::Run(int*, char***) /home/osboxes/mozilla-central/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10
    #22 0x7f7e729ff71a in XREMain::XRE_mainStartup(bool*) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4028:35
    #23 0x7f7e72a1414f in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4964:12
    #24 0x7f7e72a15e0a in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:5071:21
    #25 0x51e145 in do_main(int, char**, char**) /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:231:22
    #26 0x51e145 in main /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:304
    #27 0x7f7e8880b2e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)

SUMMARY: AddressSanitizer: double-free (/home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox+0x4e3520) in __interceptor_cfree.localalias.0

Comment 2

[Nathan Froyd [:froydnj]]

Spent a bit of time looking at BufferList::Extract, but not seeing the bug there.  Can one reproduce this without a full fuzzing build, or is a full fuzzing build a necessity?

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

You could probably turn my fuzzer into a gtest with this input hardcoded in a pretty straight forward manner.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Ok, I added |MOZ_RELEASE_ASSERT(result.mSegments.length() > 0);| right before https://searchfox.org/mozilla-central/source/mfbt/BufferList.h#544 and that triggers. I think this is the cause of the issue.

This would happen for an empty iterator argument I believe.

I'm not sure what the right way to fix this is, just stick the |result.mSegments.erase(...)| in an if statement, or something more involved?

Comment 5

[Nathan Froyd [:froydnj]]

Probably just result.Clear() is the right thing to do; I'm not sure why we're playing a complicated dance with .erase().  But I won't have time to check that this week.

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

I think the intent of |result.mSegments.erase()| is to remove everything besides the first element; |result.Clear()| would be something different, right? Or perhaps I've misunderstood the intent of this code.

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

I've confirmed that wrapping the |erase()| call in |if (result.mSegments.length() > 0) {}| stops the security issue. Does that sounds sound like a correct fix? If yes I can throw up a patch for it.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #7)
> I've confirmed that wrapping the |erase()| call in |if
> (result.mSegments.length() > 0) {}| stops the security issue. Does that
> sounds sound like a correct fix? If yes I can throw up a patch for it.

I think that stops this particular security issue.

IIUC, that erase() call is assuming that the first segment is from the result.WriteBytes call() here:

https://searchfox.org/mozilla-central/source/mfbt/BufferList.h#549-553

but subsequent ones are from `this`, and we're transferring ownership for those into `result`.  But I don't think that's necessarily true.  That is, you could have:

1. Append a new Segment to `result` inside the loop:

https://searchfox.org/mozilla-central/source/mfbt/BufferList.h#568-570

This Segment so appended will be the first Segment in `result->mSegments`.

2. Fail in the same place this bug is failing, because malicious input wanted us to Extract() more data than was actually there:

https://searchfox.org/mozilla-central/source/mfbt/BufferList.h#579-583

3. Not erase() the Segment from step 1.
4. `result`'s destructor runs, freeing the memory belonging to the Segment from step 1.
5. Oops, the memory was actually still owned by `this`, because `this->mSegments` still contains a pointer to that memory.
6. UAF.

There may be other similar bugs lurking.  I'm not completely confident that this scenario can actually happen, but I bet if we patched things with a .length() > 0 check and then kept fuzzing, we'd turn something like that up.

I think there are two options, one of which is straightforward, and one of which is more complex.  The first option is to keep track of whether `result` owns its first Segment: it will only own its first segment if `toCopy` was non-zero:

https://searchfox.org/mozilla-central/source/mfbt/BufferList.h#543

and then we'd have:

  const size_t firstOwnedSegment = toCopy == 0 : 0 ? 1;

  // Clear out any segments that we don't own.
  auto guard = MakeScopeExit([&] {
    *aSuccess = false;
    result.mSegments.erase(result.mSegments.begin() + firstOwnedSegment, result.mSegments.end());
  };

This is a little awkward?  Though I don't know if the alternative is a lot better...

The second option stems from the observation that we're attempting to not shoot ourselves in the foot: we really want to transfer ownership from `this` to `result` of a certain number of Segments.  But we have this weird intermediate state where Segments from `this` and `result` are actually pointing to the same memory, and we haven't finalized the ownership transfer.  The erase() call is trying to discard those Segments in `result` that we don't really own, so we don't double-free or UAF.  In hindsight, this seems like a bad way to organize the code.

Instead, let's rewrite all the logic to ensure that only one BufferList is holding pointers to allocated memory at any one time, so we don't have to carefully erase() things.  So the function would look a little more like:

  // Step 1.
  // Allocate a new segment for any data remaining in the current segment.
  // That is, https://searchfox.org/mozilla-central/source/mfbt/BufferList.h#549-555
  // XXX: this is inefficient if we're already at the beginning of a Segment, we should
  // just transfer ownership instead.

  // Step 2.
  // Figure out how many full Segments can be transferred from `this` to `result`.
  // That is, https://searchfox.org/mozilla-central/source/mfbt/BufferList.h#563-577
  // Except that we're not actually append()'ing, we're just determining the slice of
  // this->mSegments that we'll transfer later.
  //
  // If we fail at any point during this step, `result` owns all of its data, and can
  // be safely destroyed.

  // Step 3.
  // Determine whether we have any leftover data that we need to allocate for `result`.
  // That is, https://searchfox.org/mozilla-central/source/mfbt/BufferList.h#579-585
  // Note here that result.WriteBytes() would add a new segment in the "wrong" place,
  // because conceptually the slice of `this->mSegments` that we calculated about will
  // exist in `result` before the segment this block of code would create.
  //
  // We have two options:
  // 1) WriteBytes anyway, and manually shuffle things around (std::insert_iterator?).
  // 2) Add a WriteBytes-esque function that returns a Maybe<Segment> or something,
  //    where Nothing indicates failure.  On success, we can transfer ownership of
  //    the slice from step 2 and then append the Segment from the returned Maybe.
  //
  // If we fail here, `result` can be safely destroyed, because it owns all its data.

  // Step 4.
  // Fallibly allocate space in `result` for the slice from Step 2.
  //
  // If that fails, `result` can be safely destroyed, because it owns all its data.

  // Step 5.
  // Actually transfer ownership of Segments from `this->mSegments` to `result`, since
  // we know that we can no longer fail.  Up to this point, `this->mSegments` owns all
  // of its data, and `result` has not held any pointers from those segments.

This second option would be more complicated, but we'd have more confidence that we're not screwing ownership up, at least.

WDYT?  Do you feel like one of those two options would be better, and do you feel like implementing said option?

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

Yeah, I think option #2 makes a good deal of sense; I'll take a pass at implementing it. Getting rid of the |resultGuard| here will make me extremely happy.

Comment 10

[Phabricator Automation]

Attachment: Bug 1456189 - simplify BufferList::Extract to make the lifetimes clearer; r?froydnj

Comment 11

[Phabricator Automation]

Comment on attachment 8972661 [details]
Bug 1456189 - simplify BufferList::Extract to make the lifetimes clearer; r?froydnj

Nathan Froyd [:froydnj] has approved the revision.

https://phabricator.services.mozilla.com/D1108

Comment 12

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8972661 [details]
Bug 1456189 - simplify BufferList::Extract to make the lifetimes clearer; r?froydnj

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

It's an IPC security issue; the patch itself is a full refactor of the method so you'd have to expend some effort to figure out what the relevant bits are.

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

Tests fail with a clear-ish ASAN error.

Which older supported branches are affected by this flaw?

all

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

bug 1264642

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

Haven't prepared backports, expect them to be trivial though

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

Unlikely, there's unit tests for BufferList and they're passing

Comment 13

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

sec-approval+ to land on trunk.
We'll want patches for Beta and both ESR branches made and nominated as well.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d0aa0c581cf7c22bf7a224f7c2628951dc294bb

FYI, this grafts cleanly to Beta & ESR60 as-landed, but it'll need a rebased patch for ESR52 uplift.

Comment 15

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

Backed out for bustage in selftest.py:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b1884407a7f63b1e836acae0be593b67e8c6f7

Push with bustage and failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1d0aa0c581cf7c22bf7a224f7c2628951dc294bb&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=179067461&repo=mozilla-inbound

[task 2018-05-17T21:41:51.594Z] 21:41:51     INFO - E   exiting test
[task 2018-05-17T21:41:51.594Z] 21:41:51     INFO - E   PID 8257 | JavaScript error: resource://gre/modules/osfile/ospath_unix.jsm, line 90: TypeError: invalid path component
[task 2018-05-17T21:41:51.595Z] 21:41:51     INFO - E   "CONSOLE_MESSAGE: (error) [JavaScript Error: "TypeError: invalid path component" {file: "resource://gre/modules/osfile/ospath_unix.jsm" line: 90}]"
[task 2018-05-17T21:41:51.595Z] 21:41:51  WARNING - E   TEST-UNEXPECTED-FAIL | test_child_assertions.js | test_child_assert - [test_child_assert : 257] A promise chain failed to handle a rejection: invalid path component - stack: join@resource://gre/modules/osfile/ospath_unix.jsm:90:13
[task 2018-05-17T21:41:51.595Z] 21:41:51     INFO - E   @resource://gre/modules/CrashManager.jsm:1464:16
[task 2018-05-17T21:41:51.595Z] 21:41:51     INFO - E   get@resource://gre/modules/XPCOMUtils.jsm:183:21
[task 2018-05-17T21:41:51.595Z] 21:41:51     INFO - E   @resource://gre/modules/Services.jsm:50:5
[task 2018-05-17T21:41:51.595Z] 21:41:51     INFO - E   get@resource://gre/modules/XPCOMUtils.jsm:183:21
[task 2018-05-17T21:41:51.595Z] 21:41:51     INFO - E   addCrash@file:///builds/worker/workspace/build/src/obj-firefox/dist/bin/components/CrashService.js:171:7
[task 2018-05-17T21:41:51.595Z] 21:41:51     INFO - E   async*_do_main@/builds/worker/workspace/build/src/testing/xpcshell/head.js:215:3
[task 2018-05-17T21:41:51.595Z] 21:41:51     INFO - E   _execute_test@/builds/worker/workspace/build/src/testing/xpcshell/head.js:522:5
[task 2018-05-17T21:41:51.596Z] 21:41:51     INFO - E   @-e:1:1
[task 2018-05-17T21:41:51.596Z] 21:41:51     INFO - E   Rejection date: Thu May 17 2018 21:41:45 GMT+0000 (UTC) - false == true
[task 2018-05-17T21:41:51.596Z] 21:41:51     INFO - E   resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:257
[task 2018-05-17T21:41:51.596Z] 21:41:51     INFO - E   /builds/worker/workspace/build/src/testing/xpcshell/head.js:_execute_test:523
[task 2018-05-17T21:41:51.596Z] 21:41:51     INFO - E   -e:null:1
[task 2018-05-17T21:41:51.596Z] 21:41:51     INFO - E   exiting test

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8c922db947

Comment 17

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

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please request Beta and ESR60 approval on this patch when you get a chance (it grafts cleanly as-landed). It'll also need a rebased patch and approval request for ESR52.

Comment 19

[Alex Gaynor [:Alex_Gaynor]]

Will mark for uplift after I review bug 1462912.

Comment 20

[Alex Gaynor [:Alex_Gaynor]]

Ok patch for bug 1462912 is on autoland. What's the right procedure to make sure they both get uplifted together?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Just request approval in that bug as well and note that it depends on this bug in the appropriate place of the form.

Comment 22

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8972661 [details]
Bug 1456189 - simplify BufferList::Extract to make the lifetimes clearer; r?froydnj

Approval Request Comment
[Feature/Bug causing the regression]: sec-bug
[User impact if declined]: IPC security vulnerability
[Is this code covered by automated tests?]: it is now!
[Has the fix been verified in Nightly?]: yes, for a day or so
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: bug 1462912
[Is the change risky?]: I don't think so
[Why is the change risky/not risky?]: It now has good unit test coverage
[String changes made/needed]:

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

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8972661 [details]
Bug 1456189 - simplify BufferList::Extract to make the lifetimes clearer; r?froydnj

Approved for 61.0b8 and ESR 60.1.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fbd069ce4e4d
https://hg.mozilla.org/releases/mozilla-esr60/rev/4ad855356607

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Hey Alex, looks like we still need an ESR52 patch here.

Comment 26

[Alex Gaynor [:Alex_Gaynor]]

Attachment: [esr52] 1456189.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-high
Fix Landed on Version: esr60
Risk to taking this patch (and alternatives if risky): low, it's been baking on nightly/esr60 for a while
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 27

[Alex Gaynor [:Alex_Gaynor]]

Note that that patch also includes a backport of the follow up fix, hopefully that's not a procedural faux pas :-)

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8984228 [details] [diff] [review]
[esr52] 1456189.patch

Approved for ESR 52.9 to ride along with the other releases.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/32509dfde003