AddressSanitizer: heap-use-after-free READ of size 513 IPC

RESOLVED FIXED in Firefox 59

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rs, Assigned: Nika)

Tracking

({csectype-uaf, sec-high, testcase-wanted})

Trunk
mozilla59
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 disabled, firefox58 disabled, firefox59 fixed)

Details

(Whiteboard: [Nightly only])

Attachments

(1 attachment, 1 obsolete attachment)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.14 Safari/537.36

Steps to reproduce:

I got this crash on one of my fuzzers. I'm trying to see if I can reproduce it with a specific test case. To make sure it is not duplicated I sent it just in case.

tested on Firefox Nightly 59.0a1 (2017-12-10) (64-bit)





Actual results:

=================================================================                                                                                                                    
==8897==ERROR: AddressSanitizer: heap-use-after-free on address 0x615002329700 at pc 0x0000004319bb bp 0x7ffec68d23d0 sp 0x7ffec68d1b78                                                                              
READ of size 513 at 0x615002329700 thread T0 (Web Content)                                                                                                                                                                                                                                                                             
    #0 0x4319ba in __interceptor_strlen /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:303:5                                    
    #1 0x7f70daa7ee37 in length /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCharTraits.h:468:12                                                                                                    
    #2 0x7f70daa7ee37 in nsTDependentString /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTDependentString.h:81                                                                                      
    #3 0x7f70daa7ee37 in IPC::ParamTraits<mozilla::HangStack>::Write(IPC::Message*, mozilla::HangStack const&) /builds/worker/workspace/build/src/toolkit/components/backgroundhangmonitor/HangStack.cpp:223         
    #4 0x7f70daa7bcb4 in WriteParam<mozilla::HangStack> /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_message_utils.h:111:3                                                                  
    #5 0x7f70daa7bcb4 in IPC::ParamTraits<mozilla::HangDetails>::Write(IPC::Message*, mozilla::HangDetails const&) /builds/worker/workspace/build/src/toolkit/components/backgroundhangmonitor/HangDetails.cpp:310   
    #6 0x7f70d0c4920a in WriteParam<mozilla::HangDetails> /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_message_utils.h:111:3                                     
    #7 0x7f70d0c4920a in Write<mozilla::HangDetails> /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders/mozilla/dom/PContentChild.h:2308       
    #8 0x7f70d0c4920a in mozilla::dom::PContentChild::SendBHRThreadHang(mozilla::HangDetails const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PContentChild.cpp:4491
    #9 0x7f70daa826d9 in operator() /builds/worker/workspace/build/src/toolkit/components/backgroundhangmonitor/HangDetails.cpp:248:23                                                                 
    #10 0x7f70daa826d9 in mozilla::detail::RunnableFunction<mozilla::nsHangDetails::Submit()::$_0>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:529                            
    #11 0x7f70cf4d4b54 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:396:25                                        
    #12 0x7f70cf4fb42e in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1033:14                                                                              
    #13 0x7f70cf516d90 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:508:10                                                                            
    #14 0x7f70d039e21a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21                                                             
    #15 0x7f70d02f51a9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10                                                                                                
    #16 0x7f70d02f51a9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319                                                                                                    
    #17 0x7f70d02f51a9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299                                                                                            
    #18 0x7f70d68a4dda in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27                                                                                                  
    #19 0x7f70dafd8b0b in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:865:22                                                                                               
    #20 0x7f70d02f51a9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10                                                                                                
    #21 0x7f70d02f51a9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319                                                                                                    
    #22 0x7f70d02f51a9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299                                                                                            
    #23 0x7f70dafd84fd in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:691:34                                                          
    #24 0x4ee965 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30                                                                             
    #25 0x4ee965 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280                                                                                                                    
    #26 0x7f70ee108039 in __libc_start_main (/lib64/libc.so.6+0x21039)                                                                                             
    #27 0x41dfe8 in _start (/home/fuzzer/browsers/firefox/firefox+0x41dfe8)                                                                                                                                              
                                                                                                                                                                                          
0x615002329900 is located 0 bytes to the right of 512-byte region [0x615002329700,0x615002329900)                                                                                                                    
freed by thread T0 (Web Content) here:                                                                                                                                                                               
    #0 0x4bea42 in __interceptor_free /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:68:3                                                                        
    #1 0x7f70d0553f59 in free_ /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/AllocPolicy.h:119:5                                                                                               
    #2 0x7f70d0553f59 in ~Vector /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Vector.h:930                                                                                                    
    #3 0x7f70d0553f59 in ~HangStack /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/HangStack.h:25                                                                                               
    #4 0x7f70d0553f59 in mozilla::HangDetails::~HangDetails() /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/HangDetails.h:29                                                                   
    #5 0x7f70daa8200b in ~nsHangDetails /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/HangDetails.h:87:29                                                                                      
    #6 0x7f70daa8200b in mozilla::nsHangDetails::~nsHangDetails() /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/HangDetails.h:87                                                               
    #7 0x7f70daa7a48c in mozilla::nsHangDetails::Release() /builds/worker/workspace/build/src/toolkit/components/backgroundhangmonitor/HangDetails.cpp:279:1                                                         
    #8 0x7f70daa8299e in Release /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:41:11                                                                                                  
    #9 0x7f70daa8299e in Release /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:398                                                                                                    
    #10 0x7f70daa8299e in ~RefPtr /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:79                                                                         
    #11 0x7f70daa8299e in ~ /builds/worker/workspace/build/src/toolkit/components/backgroundhangmonitor/HangDetails.cpp:237                                      
    #12 0x7f70daa8299e in ~RunnableFunction /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:517
    #13 0x7f70daa8299e in mozilla::detail::RunnableFunction<mozilla::nsHangDetails::Submit()::$_0>::~RunnableFunction() /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:517
    #14 0x7f70cf5133dc in mozilla::Runnable::Release() /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:43:1                                                                                       
    #15 0x7f70cf4d4d83 in assign_assuming_AddRef /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:355:7                                                        
    #16 0x7f70cf4d4d83 in operator= /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:631                                                                                                       
    #17 0x7f70cf4d4d83 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:401                                                                           
    #18 0x7f70cf4fb42e in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1033:14                                                                              
    #19 0x7f70cf516d90 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:508:10                                                                            
    #20 0x7f70d039e21a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21                                                             
    #21 0x7f70d02f51a9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10                                                                                                
    #22 0x7f70d02f51a9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319                                                                                                    
    #23 0x7f70d02f51a9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299                                                                                            
    #24 0x7f70d68a4dda in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27                                                                                                  
    #25 0x7f70dafd8b0b in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:865:22                                                                                               
    #26 0x7f70d02f51a9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10                                                                                                
    #27 0x7f70d02f51a9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319                                                                                                    
    #28 0x7f70d02f51a9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299                                                                                            
    #29 0x7f70dafd84fd in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:691:34                                                     
    #30 0x4ee965 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30                           
    #31 0x4ee965 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280                                                                                                                         
    #32 0x7f70ee108039 in __libc_start_main (/lib64/libc.so.6+0x21039)                                                                                                                    
                                                                                                                                                                                                                     
previously allocated by thread T20 (BgHangManager) here:                                                                                                                                                             
    #0 0x4bed83 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3                                                                                    
    #1 0x7f70d1fa3612 in maybe_pod_malloc<char> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/AllocPolicy.h:81:28                                                                              
    #2 0x7f70d1fa3612 in pod_malloc<char> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/AllocPolicy.h:102                                                                                      
    #3 0x7f70d1fa3612 in convertToHeapStorage /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Vector.h:959                                                                                       
    #4 0x7f70d1fa3612 in mozilla::Vector<char, 0ul, mozilla::MallocAllocPolicy>::growStorageBy(unsigned long) /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Vector.h:1050                      
    #5 0x7f70daa803a5 in reserve /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Vector.h:1112:9                                                                                                 
    #6 0x7f70daa803a5 in EnsureBufferCapacity /builds/worker/workspace/build/src/toolkit/components/backgroundhangmonitor/HangStack.h:270                                                                            
    #7 0x7f70daa803a5 in mozilla::ThreadStackHelper::PrepareStackBuffer(mozilla::HangStack&) /builds/worker/workspace/build/src/toolkit/components/backgroundhangmonitor/ThreadStackHelper.cpp:98                    
    #8 0x7f70daa747f6 in GetStack /builds/worker/workspace/build/src/toolkit/components/backgroundhangmonitor/ThreadStackHelper.cpp:124:8                                                                            
    #9 0x7f70daa747f6 in mozilla::BackgroundHangManager::RunMonitorThread() /builds/worker/workspace/build/src/toolkit/components/backgroundhangmonitor/BackgroundHangMonitor.cpp:366                                
    #10 0x7f70daa73d23 in mozilla::BackgroundHangManager::MonitorThread(void*) /builds/worker/workspace/build/src/toolkit/components/backgroundhangmonitor/BackgroundHangMonitor.cpp:76:49
    #11 0x7f70eb7d7d7e in _pt_root /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:216:5                                                   
    #12 0x7f70ef1d1608 in start_thread (/lib64/libpthread.so.0+0x7608)                                           
Thread T20 (BgHangManager) created by T0 (Web Content) here:                                                                                                                                                         
    #0 0x4a80dd in __interceptor_pthread_create /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:204:3                             
    #1 0x7f70eb7d4acf in _PR_CreateThread /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:457:14                                                                                               
    #2 0x7f70eb7d46be in PR_CreateThread /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:548:12                                                                                                
    #3 0x7f70daa7722e in BackgroundHangManager /builds/worker/workspace/build/src/toolkit/components/backgroundhangmonitor/BackgroundHangMonitor.cpp:261:24                                                          
    #4 0x7f70daa7722e in mozilla::BackgroundHangMonitor::Startup() /builds/worker/workspace/build/src/toolkit/components/backgroundhangmonitor/BackgroundHangMonitor.cpp:619                                         
    #5 0x7f70cf555243 in NS_InitXPCOM2 /builds/worker/workspace/build/src/xpcom/build/XPCOMInit.cpp:724:3                                                                                                            
    #6 0x7f70dafd79a3 in XRE_InitEmbedding2(nsIFile*, nsIFile*, nsIDirectoryServiceProvider*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:187:8                                              
    #7 0x7f70d03a929e in mozilla::ipc::ScopedXREEmbed::Start() /builds/worker/workspace/build/src/ipc/glue/ScopedXREEmbed.cpp                                                                                        
    #8 0x7f70d607632a in mozilla::dom::ContentProcess::Init(int, char**) /builds/worker/workspace/build/src/dom/ipc/ContentProcess.cpp:245:13                                                                        
    #9 0x7f70dafd84eb in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:663:21                                                           
    #10 0x4ee965 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30                                                                             
    #11 0x4ee965 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280                                                                                                                         
    #12 0x7f70ee108039 in __libc_start_main (/lib64/libc.so.6+0x21039)                                                                                                                                               
                                                                                                                                                                                                                     
SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:303:5 in __interceptor_strlen
Shadow bytes around the buggy address:                                                                                                                             
  0x0c2a8045d290: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa                                                                                                                                                    
  0x0c2a8045d2a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa                                                                                                                         
  0x0c2a8045d2b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa                                                                                                                                                    
  0x0c2a8045d2c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa                                                                                                                                                    
  0x0c2a8045d2d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa                                                                                                                                                    
=>0x0c2a8045d2e0:[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd                                                                                                                                                    
  0x0c2a8045d2f0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd                                                                                                                                                    
  0x0c2a8045d300: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd                                                                                                                                                    
  0x0c2a8045d310: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd                                                                                                                                                    
  0x0c2a8045d320: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa                                                                                                                                                    
  0x0c2a8045d330: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa                                                                                                                                                    
Shadow byte legend (one shadow byte represents 8 application bytes):        
  Addressable:           00                                                                                                                                                                                          
  Partially addressable: 01 02 03 04 05 06 07                                                                                                                                                                        
  Heap left redzone:       fa                                                                                                                                                             
  Freed heap region:       fd                                                                                                                                    
  Stack left redzone:      f1
  Freed heap region:       fd                                                                                                                                                                              [175/1818]
  Stack left redzone:      f1                                                                                                                                                      
  Stack mid redzone:       f2                                                                                                                                                                          
  Stack right redzone:     f3                                                                                                                                                                                        
  Stack after return:      f5                                                                                                                                                        
  Stack use after scope:   f8                                                                                                                                                                                        
  Global redzone:          f9                                                                                                                                                                                        
  Global init order:       f6                                                                                                                                                                                        
  Poisoned by user:        f7                                                                                                                                                                                        
  Container overflow:      fc                                                                                                                                                                                        
  Array cookie:            ac                                                                                                                                                                                        
  Intra object redzone:    bb                                                                                                                                                                                        
  ASan internal:           fe                                                                                                                                                                                        
  Left alloca redzone:     ca                                                                                                                                                                                        
  Right alloca redzone:    cb                                                                                                                                                                                        
==8897==ABORTING
Flags: sec-bounty?
Group: firefox-core-security → dom-core-security
Component: Untriaged → Security: Process Sandboxing
Product: Firefox → Core
Nika, any ideas? It looks like we're somehow trying to send a HangDetails after it was destroyed.
Component: Security: Process Sandboxing → XPCOM
Flags: needinfo?(nika)
HangStack objects contain char* pointers into a buffer which is also on the object. This doesn't appear to be due to sending a HangDetails after it was destroyed, but rather that one of these HangStack objects somehow got a reference into another HangStack's backing buffer.

I looked through the code and don't know how that could have happened. If we somehow get a more reproducible test case then it will be much easier to figure out what happened.
Flags: needinfo?(nika)
So far I have not been able to reproduce it with the samples buffer I have at that moment (I use to save the last 50 testcases before a crash). That doesn't means it somehow fixed. It happened twice, but I'm still looking in case I could get a specific testcase. Anyway, a second pair of eyes is appreciated.


> If we
> somehow get a more reproducible test case then it will be much easier to
> figure out what happened.
See Also: → 1426184
Have you had the chance to review #bug 1426184 stacktrace?

(In reply to Nika Layzell [:mystor] from comment #2)
(In reply to Francisco A. from comment #4)
> Have you had the chance to review #bug 1426184 stacktrace?
> 
> (In reply to Nika Layzell [:mystor] from comment #2)

Can you please take a look? :)
Flags: needinfo?(nika)
I'm rewriting HangStack to remove all of the unsafe logic which could be causing the crash, and instead use obviously safe code in the hope that that fixes this issue.
This patch is ~+200 -800, so I figure it's a pretty good one.

I tried to read through the HangStack class to find the problem but couldn't
because it is big and unweildy, so I just re-implemented it in an obviously safe
way. This also let me get around the need to manually write the IPC::ParamTraits
impls, as I could instead just write the data types in ipdl, which also reduced
the complexity of the component significantly.

MozReview-Commit-ID: qlBUnvYams
Attachment #8941643 - Flags: review?(nfroyd)
Assignee: nobody → nika
Flags: needinfo?(nika)
(In reply to Nika Layzell [:mystor] from comment #7)
> Created attachment 8941643 [details] [diff] [review]
> Remove the complex custom HangStack class and replace it with a much simpler
> implementation
> 
> This patch is ~+200 -800, so I figure it's a pretty good one.
> 
> I tried to read through the HangStack class to find the problem but couldn't
> because it is big and unweildy, so I just re-implemented it in an obviously
> safe
> way. This also let me get around the need to manually write the
> IPC::ParamTraits
> impls, as I could instead just write the data types in ipdl, which also
> reduced
> the complexity of the component significantly.
> 
> MozReview-Commit-ID: qlBUnvYams

I should note that the actual commmit message I'll push will probably be something along the lines of

Simplify `HangStack` and `HangDetails` by implementing using ipdl structs and unions, r=froydnj

'cause security bugs.
code is much clearer, thank you!.


(In reply to Nika Layzell [:mystor] from comment #7)
> Created attachment 8941643 [details] [diff] [review]
> Remove the complex custom HangStack class and replace it with a much simpler
> implementation
> 
> This patch is ~+200 -800, so I figure it's a pretty good one.
Duplicate of this bug: 1426184
Duplicate of this bug: 1418125
No longer blocks: 1418125
Keywords: sec-high
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8941643 [details] [diff] [review]
Remove the complex custom HangStack class and replace it with a much simpler implementation

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

Nit: "big and unwieldy" in the commit message.

This is way more understandable, thank you.  I should have suggested this in the original review.  Some notes below.

::: toolkit/components/backgroundhangmonitor/HangDetails.h
@@ +10,5 @@
>  #include "ipc/IPCMessageUtils.h"
>  #include "mozilla/ProcessedStack.h"
>  #include "mozilla/RefPtr.h"
>  #include "mozilla/Move.h"
> +// #include "mozilla/HangStack.h"

Nit: just delete this rather than commenting it out?

::: toolkit/components/backgroundhangmonitor/ThreadStackHelper.cpp
@@ +190,5 @@
>    mDesiredStackSize += 1;
>  
>    // Perform the append if we have enough space to do so.
> +  if (mStackToFill->stack().Capacity() > mStackToFill->stack().Length()) {
> +    mStackToFill->stack().AppendElement(mozilla::Move(aFrame));

Last time I looked, IPDL-generated classes did not have move constructors.  Do we care about this inefficiency for purposes of this patch?  (Probably not?)

@@ +272,5 @@
> +    const char* entryLabel = aEntry.label();
> +
> +    // We need to do this slightly sketchy thing in order to put a literal
> +    // string value into the label. We cannot allocate here, so it's important
> +    // that we create a literal DataFlags string.

So, uh, ok.

I think a better comment here would explain why faking this as a literal string is justifiable.  I think, TryAppendFrame will copy the string anyway (see comments above), in which case we might as well do a copy here, since this comment is mis-stating the need to avoid allocation.  If I am wrong, and TryAppendFrame transfers the literalness of this string into storage, then we need an argument here about why it is safe to have a long-lived reference to aEntry.label().

Either way...something needs to change here.

@@ +275,5 @@
> +    // string value into the label. We cannot allocate here, so it's important
> +    // that we create a literal DataFlags string.
> +    nsCString label;
> +    label.AssignLiteral(entryLabel, strlen(entryLabel));
> +    TryAppendFrame(label);

This is not your fault, but I am reminded that the IPDL union constructs are implicit, and therefore this has magic going on, which is unfortunate.  Also, the implicitly-invoked constructor here will copy-construct from `label`, for which see the above objection.
Attachment #8941643 - Flags: review?(nfroyd)
Attachment #8941643 - Attachment is obsolete: true
Is this fix planned for Tuesday's 2018-01-23 release?
Comment on attachment 8942310 [details] [diff] [review]
Simplify `HangStack` and `HangDetails` by implementing using ipdl structs and unions

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

Works for me.  Thanks for clarifying the code with a comment.
Attachment #8942310 - Flags: review?(nfroyd) → review+
(In reply to Francisco A. from comment #14)
> Is this fix planned for Tuesday's 2018-01-23 release?

It's unlikely that this would make it into Firefox 58, but it will likely be in Firefox 59 (released on 2018-03-13).
(In reply to Nathan Froyd [:froydnj] from comment #16)
> It's unlikely that this would make it into Firefox 58, but it will likely be
> in Firefox 59 (released on 2018-03-13).

It's a bit disappointing.. Two months more seems too much to be honest.
(In reply to Francisco A. from comment #17)
> It's a bit disappointing.. Two months more seems too much to be honest.

This code doesn't run outside of nightly builds of Firefox. This bug cannot affect release or beta users of the software.

Once we land the fix in central there's no point in even trying to uplift.
In a related note, because of that I'm not sure this is actually sec-high.
Comment on attachment 8942310 [details] [diff] [review]
Simplify `HangStack` and `HangDetails` by implementing using ipdl structs and unions

NOTE: I am not sure this bug is sec-high. The code being fixed does not run outside of nightly builds of Firefox, so despite it being present in older versions it should never be hit.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch doesn't expose any information about the exploit other than the part of code it might be in. The patch just wholesale replaces a potentially buggy component.

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?
None.

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

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

How likely is this patch to cause regressions; how much testing does it need?
This patch may cause regressions, but they should only impact our BHR telemetry. I imagine regressions are unlikely.
Attachment #8942310 - Flags: sec-approval?
(In reply to Nika Layzell [:mystor] from comment #18)
> This code doesn't run outside of nightly builds of Firefox. This bug cannot
> affect release or beta users of the software.
> 
> Once we land the fix in central there's no point in even trying to uplift.

Oh really? That's good to know. What we do in that situation is mark other branches as disabled, but still keep the sec-high rating. If it is Nightly-only, you don't need sec-approval.
Attachment #8942310 - Flags: sec-approval?
Depends on: 1430850
(In reply to Nika Layzell [:mystor] from comment #22)
> Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/19c0ea91ed2b

Thank you!
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Group: dom-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.