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 |
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
•