1435816 - Enhance BaseThreadInitThunk hook to block threads with LoadLibrary* functions as entry points

(No longer employed by Mozilla) Aaron Klotz

Reporter

Description

•

7 years ago

This gives us the ability to track/block loads using the CreateRemoteThread technique.

Selena Deckelmann :selenamarie :selena

Updated

•

7 years ago

Assignee: nobody → carlco

(No longer employed by Mozilla) Aaron Klotz

Reporter

Updated

•

7 years ago

Status: NEW → ASSIGNED

Priority: -- → P1

Whiteboard: inj+

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/224962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224962/

(No longer employed by Mozilla) Aaron Klotz

Reporter

Comment 2

•

7 years ago

mozreview-review

Comment on attachment 8956016 [details]
Bug 1435816: Prevent thread start in LoadLibrary;

https://reviewboard.mozilla.org/r/224962/#review231762

Mostly some style nits, but I would like to see this patch once more before landing.

::: mozglue/build/WindowsDllBlocklist.cpp:26
(Diff revision 1)
>  #pragma warning( push )
>  #pragma warning( disable : 4275 4530 ) // See msvc-stl-wrapper.template.h
>  #include <map>
>  #pragma warning( pop )
>  
> +#include <vector>

Change this to use mozilla/Vector.h (see below)

::: mozglue/build/WindowsDllBlocklist.cpp:811
(Diff revision 1)
>  }
>  
> +#if defined(NIGHTLY_BUILD)
> +// Map of specific thread proc addresses we should block. In particular,
> +// LoadLibrary* APIs which indicate DLL injection
> +static std::vector<void*> gStartAddressesToBlock;

In general we avoid static initializers in our code unless they are provably trivial.

Since we know ahead of time how big our vector needs to be, and MFBT contains a vector implementation with in-place storage, I think it would be good to change this variable to be mozilla::Vector<void*, 4>* and then new that Vector down in the initialization code. It also supports begin() and end(), so you can still use std::find.

::: mozglue/build/WindowsDllBlocklist.cpp:824
(Diff revision 1)
>      return false;
>  
> +#if defined(NIGHTLY_BUILD)
> +  if (std::find(gStartAddressesToBlock.begin(), gStartAddressesToBlock.end(),
> +	  aStartAddress) != gStartAddressesToBlock.end()) {
> +	  return true;

Nit: indentation.

I'd suggest aligning aStartAddress underneath the first parameter to std::find. And the return statement should be intented 2 spaces into the if block.

::: mozglue/build/WindowsDllBlocklist.cpp:827
(Diff revision 1)
> +  if (std::find(gStartAddressesToBlock.begin(), gStartAddressesToBlock.end(),
> +	  aStartAddress) != gStartAddressesToBlock.end()) {
> +	  return true;
> +  }
> +#endif
> +  

Nit: There is some whitespace within this blank line, please remove it.

::: mozglue/build/WindowsDllBlocklist.cpp:943
(Diff revision 1)
> +    pProc = GetProcAddress(hKernel, "LoadLibraryExA");
> +    if (pProc)
> +      gStartAddressesToBlock.push_back(pProc);
> +
> +    pProc = GetProcAddress(hKernel, "LoadLibraryExW");
> +    if (pProc)

Nit: Our style guide (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style) prefers that single-line consequents be surrounded by braces even though they are redundant.

Attachment #8956016 - Flags: review?(aklotz) → review-

(No longer employed by Mozilla) Aaron Klotz

Reporter

Comment 3

•

7 years ago

mozreview-review

Comment on attachment 8956016 [details]
Bug 1435816: Prevent thread start in LoadLibrary;

https://reviewboard.mozilla.org/r/224962/#review231772

::: mozglue/build/WindowsDllBlocklist.cpp:811
(Diff revision 1)
>  }
>  
> +#if defined(NIGHTLY_BUILD)
> +// Map of specific thread proc addresses we should block. In particular,
> +// LoadLibrary* APIs which indicate DLL injection
> +static std::vector<void*> gStartAddressesToBlock;

Grrr, markdown! I meant mozilla::Vector<void*, 4>*

Comment hidden (mozreview-request)

Comment on attachment 8956016 [details]
Bug 1435816: Prevent thread start in LoadLibrary;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224962/diff/1-2/

(No longer employed by Mozilla) Aaron Klotz

Reporter

Comment 5

•

7 years ago

mozreview-review

Comment on attachment 8956016 [details]
Bug 1435816: Prevent thread start in LoadLibrary;

https://reviewboard.mozilla.org/r/224962/#review233720

Okay, thanks! Landable when these two open issues are resolved.

::: mozglue/build/WindowsDllBlocklist.cpp:26
(Diff revision 2)
>  #pragma warning( push )
>  #pragma warning( disable : 4275 4530 ) // See msvc-stl-wrapper.template.h
>  #include <map>
>  #pragma warning( pop )
>  
> +#include <mozilla/Vector.h>

Nit: Can you move this include such that it is placed alphabetically within the remaining headers included below it?

::: mozglue/build/WindowsDllBlocklist.cpp:823
(Diff revision 2)
>    if (aStartAddress == 0)
>      return false;
>  
> +#if defined(NIGHTLY_BUILD)
> +  if (std::find(gStartAddressesToBlock->begin(), gStartAddressesToBlock->end(),
> +      aStartAddress) != gStartAddressesToBlock->end()) {

Nit: You can align aStartAddress underneath the first argument to std::find

Attachment #8956016 - Flags: review?(aklotz) → review+

Comment hidden (mozreview-request)

Comment on attachment 8956016 [details]
Bug 1435816: Prevent thread start in LoadLibrary;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224962/diff/2-3/

Carl Corcoran [:ccorcoran]

Assignee

Updated

•

7 years ago

Keywords: checkin-needed

Carl Corcoran [:ccorcoran]

Assignee

Updated

•

7 years ago

Blocks: 1435773

Carl Corcoran [:ccorcoran]

Assignee

Updated

•

7 years ago

No longer blocks: 1435776

Pulsebot

Comment 7

•

7 years ago

Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66fb307a2a70
Prevent thread start in LoadLibrary; r=aklotz

Keywords: checkin-needed

Treeherder Bug Filer

Updated

•

7 years ago

Depends on: 1446396

Natalia Csoregi [:nataliaCs]

Comment 8

•

7 years ago

Backed out changeset for windows mingw failures on WindowsDllBlocklist.cpp:822
backout: https://hg.mozilla.org/integration/autoland/rev/1844f2f666237323616c67178f515e2fb14483ca

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=66fb307a2a709ec7df5e542aed9291879d063697&selectedJob=168496305

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=168496305&repo=autoland&lineNumber=14341

[task 2018-03-16T13:24:23.165Z] 13:24:23     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/mozglue/build'
[task 2018-03-16T13:24:23.166Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/mingw32/bin/i686-w64-mingw32-g++ -mwindows -o WindowsDllBlocklist.o -c -DDEBUG=1 -DIMPL_MFBT -DMOZ_HAS_MOZGLUE -I/builds/worker/workspace/build/src/mozglue/build -I/builds/worker/workspace/build/src/obj-firefox/mozglue/build -I/builds/worker/workspace/build/src/memory/build -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wno-unknown-pragmas -Wno-unused-function -Wno-conversion-null -Wno-switch -Wno-enum-compare -fno-sized-deallocation -fno-exceptions -fno-strict-aliasing -mms-bitfields -mstackrealign -fno-keep-inline-dllexport -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -fno-omit-frame-pointer  -MD -MP -MF .deps/WindowsDllBlocklist.o.pp   /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp
[task 2018-03-16T13:24:23.166Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp: In function 'bool ShouldBlockThread(void*)':
[task 2018-03-16T13:24:23.166Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp:822:30: error: no matching function for call to 'find(void**, void**, void*&)'
[task 2018-03-16T13:24:23.166Z] 13:24:23     INFO -                   aStartAddress) != gStartAddressesToBlock->end()) {
[task 2018-03-16T13:24:23.166Z] 13:24:23     INFO -                                ^
[task 2018-03-16T13:24:23.166Z] 13:24:23     INFO -  In file included from /builds/worker/workspace/build/src/mingw32/i686-w64-mingw32/include/c++/6.4.0/bits/locale_facets.h:48:0,
[task 2018-03-16T13:24:23.166Z] 13:24:23     INFO -                   from /builds/worker/workspace/build/src/mingw32/i686-w64-mingw32/include/c++/6.4.0/bits/basic_ios.h:37,
[task 2018-03-16T13:24:23.166Z] 13:24:23     INFO -                   from /builds/worker/workspace/build/src/mingw32/i686-w64-mingw32/include/c++/6.4.0/ios:44,
[task 2018-03-16T13:24:23.166Z] 13:24:23     INFO -                   from /builds/worker/workspace/build/src/mingw32/i686-w64-mingw32/include/c++/6.4.0/ostream:38,
[task 2018-03-16T13:24:23.166Z] 13:24:23     INFO -                   from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Maybe.h:22,
[task 2018-03-16T13:24:23.166Z] 13:24:23     INFO -                   from /builds/worker/workspace/build/src/mozglue/build/Authenticode.h:10,
[task 2018-03-16T13:24:23.167Z] 13:24:23     INFO -                   from /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp:26:
[task 2018-03-16T13:24:23.170Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mingw32/i686-w64-mingw32/include/c++/6.4.0/bits/streambuf_iterator.h:369:5: note: candidate: template<class _CharT2> typename __gnu_cxx::__enable_if<std::__is_char<_CharT2>::__value, std::istreambuf_iterator<_CharT> >::__type std::find(std::istreambuf_iterator<_CharT>, std::istreambuf_iterator<_CharT>, const _CharT2&)
[task 2018-03-16T13:24:23.170Z] 13:24:23     INFO -       find(istreambuf_iterator<_CharT> __first,
[task 2018-03-16T13:24:23.170Z] 13:24:23     INFO -       ^~~~
[task 2018-03-16T13:24:23.172Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mingw32/i686-w64-mingw32/include/c++/6.4.0/bits/streambuf_iterator.h:369:5: note:   template argument deduction/substitution failed:
[task 2018-03-16T13:24:23.172Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp:822:30: note:   mismatched types 'std::istreambuf_iterator<_CharT>' and 'void**'
[task 2018-03-16T13:24:23.172Z] 13:24:23     INFO -                   aStartAddress) != gStartAddressesToBlock->end()) {
[task 2018-03-16T13:24:23.172Z] 13:24:23     INFO -                                ^
[task 2018-03-16T13:24:23.172Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp: In function 'void DllBlocklist_Initialize(uint32_t)':
[task 2018-03-16T13:24:23.173Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp:930:27: error: invalid conversion from 'FARPROC {aka int (__attribute__((__stdcall__)) *)()}' to 'void*' [-fpermissive]
[task 2018-03-16T13:24:23.173Z] 13:24:23     INFO -       pProc = GetProcAddress(hKernel, "LoadLibraryA");
[task 2018-03-16T13:24:23.173Z] 13:24:23     INFO -               ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
[task 2018-03-16T13:24:23.173Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp:935:27: error: invalid conversion from 'FARPROC {aka int (__attribute__((__stdcall__)) *)()}' to 'void*' [-fpermissive]
[task 2018-03-16T13:24:23.173Z] 13:24:23     INFO -       pProc = GetProcAddress(hKernel, "LoadLibraryW");
[task 2018-03-16T13:24:23.173Z] 13:24:23     INFO -               ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
[task 2018-03-16T13:24:23.174Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp:940:27: error: invalid conversion from 'FARPROC {aka int (__attribute__((__stdcall__)) *)()}' to 'void*' [-fpermissive]
[task 2018-03-16T13:24:23.174Z] 13:24:23     INFO -       pProc = GetProcAddress(hKernel, "LoadLibraryExA");
[task 2018-03-16T13:24:23.174Z] 13:24:23     INFO -               ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2018-03-16T13:24:23.174Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp:945:27: error: invalid conversion from 'FARPROC {aka int (__attribute__((__stdcall__)) *)()}' to 'void*' [-fpermissive]
[task 2018-03-16T13:24:23.174Z] 13:24:23     INFO -       pProc = GetProcAddress(hKernel, "LoadLibraryExW");
[task 2018-03-16T13:24:23.174Z] 13:24:23     INFO -               ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2018-03-16T13:24:23.175Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp:932:44: warning: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = void*&; T = void*; unsigned int MinInlineCapacity = 4u; AllocPolicy = mozilla::MallocAllocPolicy]', declared with attribute warn_unused_result [-Wunused-result]
[task 2018-03-16T13:24:23.175Z] 13:24:23     INFO -         gStartAddressesToBlock->append(pProc);
[task 2018-03-16T13:24:23.175Z] 13:24:23     INFO -                                              ^
[task 2018-03-16T13:24:23.175Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp:937:44: warning: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = void*&; T = void*; unsigned int MinInlineCapacity = 4u; AllocPolicy = mozilla::MallocAllocPolicy]', declared with attribute warn_unused_result [-Wunused-result]
[task 2018-03-16T13:24:23.175Z] 13:24:23     INFO -         gStartAddressesToBlock->append(pProc);
[task 2018-03-16T13:24:23.175Z] 13:24:23     INFO -                                              ^
[task 2018-03-16T13:24:23.176Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp:942:44: warning: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = void*&; T = void*; unsigned int MinInlineCapacity = 4u; AllocPolicy = mozilla::MallocAllocPolicy]', declared with attribute warn_unused_result [-Wunused-result]
[task 2018-03-16T13:24:23.176Z] 13:24:23     INFO -         gStartAddressesToBlock->append(pProc);
[task 2018-03-16T13:24:23.176Z] 13:24:23     INFO -                                              ^
[task 2018-03-16T13:24:23.176Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/mozglue/build/WindowsDllBlocklist.cpp:947:44: warning: ignoring return value of 'bool mozilla::Vector<T, N, AllocPolicy>::append(U&&) [with U = void*&; T = void*; unsigned int MinInlineCapacity = 4u; AllocPolicy = mozilla::MallocAllocPolicy]', declared with attribute warn_unused_result [-Wunused-result]
[task 2018-03-16T13:24:23.176Z] 13:24:23     INFO -         gStartAddressesToBlock->append(pProc);
[task 2018-03-16T13:24:23.176Z] 13:24:23     INFO -                                              ^
[task 2018-03-16T13:24:23.176Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1045: recipe for target 'WindowsDllBlocklist.o' failed
[task 2018-03-16T13:24:23.176Z] 13:24:23     INFO -  make[4]: *** [WindowsDllBlocklist.o] Error 1
[task 2018-03-16T13:24:23.177Z] 13:24:23     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/mozglue/build'
[task 2018-03-16T13:24:23.177Z] 13:24:23     INFO -  /builds/worker/workspace/build/src/config/recurse.mk:73: recipe for target 'mozglue/build/target' failed
[task 2018-03-16T13:24:23.177Z] 13:24:23     INFO -  make[3]: *** [mozglue/build/target] Error 2
[task 2018-03-16T13:24:23.177Z] 13:24:23     INFO -  make[3]: *** Waiting for unfinished jobs....

Carl Corcoran [:ccorcoran]

Assignee

Updated

•

7 years ago

Blocks: 1435776
No longer blocks: 1435773

Comment hidden (mozreview-request)

Comment on attachment 8956016 [details]
Bug 1435816: Prevent thread start in LoadLibrary;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224962/diff/3-4/

Carl Corcoran [:ccorcoran]

Assignee

Updated

•

7 years ago

Keywords: checkin-needed

(No longer employed by Mozilla) Aaron Klotz

Reporter

Comment 10

•

7 years ago

I've queued it up on autoland.

Keywords: checkin-needed

Pulsebot

Comment 11

•

7 years ago

Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1250e3ead1a
Prevent thread start in LoadLibrary; r=aklotz

Andrei Ciure[:aciure]

Comment 13

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/d1250e3ead1a

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox61: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla61

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

Updated

•

7 years ago

Depends on: 1446680

Julien Cristau [:jcristau]

Updated

•

7 years ago

status-firefox60: affected → wontfix

Carl Corcoran [:ccorcoran]

Assignee

Updated

•

6 years ago