Closed
Bug 1435816
Opened 7 years ago
Closed 7 years ago
Enhance BaseThreadInitThunk hook to block threads with LoadLibrary* functions as entry points
Categories
(Core :: General, enhancement, P1)
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: bugzilla, Assigned: ccorcoran)
References
(Blocks 1 open bug)
Details
(Whiteboard: inj+)
Attachments
(1 file)
This gives us the ability to track/block loads using the CreateRemoteThread technique.
Updated•7 years ago
|
Assignee: nobody → carlco
Reporter | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: inj+
Comment hidden (mozreview-request) |
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-
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) |
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) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Pushed by ncsoregi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/66fb307a2a70 Prevent thread start in LoadLibrary; r=aklotz
Keywords: checkin-needed
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....
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
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
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
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•