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)

Unspecified
Windows
enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

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.
Assignee: nobody → carlco
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: inj+
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-
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 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+
Keywords: checkin-needed
Blocks: 1435773
No longer blocks: 1435776
Pushed by ncsoregi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/66fb307a2a70 Prevent thread start in LoadLibrary; r=aklotz
Keywords: checkin-needed
Depends on: 1446396
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....
Blocks: 1435776
No longer blocks: 1435773
Keywords: checkin-needed
I've queued it up on autoland.
Keywords: checkin-needed
Pushed by aklotz@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d1250e3ead1a Prevent thread start in LoadLibrary; r=aklotz
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: