Closed Bug 1842088 Opened 1 year ago Closed 1 year ago

TestDllBlocklist failures on Windows 7

Categories

(Core :: DLL Services, defect, P3)

Product:
Core
Component:
DLL Services
Platform:
Unspecified
Windows 7
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
117 Branch
Tracking Flags:
Tracking Status
firefox-esr102 --- wontfix
firefox-esr115 --- fixed
firefox115 --- wontfix
firefox116 --- wontfix
firefox117 --- fixed

People

(Reporter: RyanVM, Assigned: gstoll)

References

Details

Attachments

(4 files, 2 obsolete files)

Bug 1842088 - Avoid blocking blocked modules when loaded as image resources on Windows 7. r=gstoll
48 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-esr115+
Details | Review
Bug 1842088 - Explicitly test DLL blocklist assumptions in isolation with a cppunit test. r=mhowell
48 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-esr115+
Details | Review
Bug 1842088 - Avoid blocking blocked modules when loaded as image resources on Windows 7. r=gstoll
48 bytes, text/x-phabricator-request
Details | Review
Bug 1842088 - Explicitly test DLL blocklist assumptions in isolation with a cppunit test. r=mhowell
48 bytes, text/x-phabricator-request
Details | Review

While testing the fix for bug 1837242 on Try with a bit of config hackery to run Windows 7 GTests, we observed some test failures after that patch was applied:

   INFO -  TEST-START | TestDllBlocklist.BlockDllByName
   INFO -  TEST-PASS | TestDllBlocklist.BlockDllByName | test completed (time: 0ms)
   INFO -  TEST-START | TestDllBlocklist.BlockDllByVersion
WARNING -  TEST-UNEXPECTED-FAIL | TestDllBlocklist.BlockDllByVersion | Value of: hDll
   INFO -    Actual: false
   INFO -  Expected: true @ /builds/worker/checkouts/gecko/toolkit/xre/dllservices/tests/gtest/TestDLLBlocklist.cpp:162
WARNING -  TEST-UNEXPECTED-FAIL | TestDllBlocklist.BlockDllByVersion | test completed (time: 1ms)
   INFO -  TEST-START | TestDllBlocklist.AllowDllByVersion
   INFO -  TEST-PASS | TestDllBlocklist.AllowDllByVersion | test completed (time: 0ms)
   INFO -  TEST-START | TestDllBlocklist.GPUProcessOnly_AllowInMainProcess
   INFO -  TEST-PASS | TestDllBlocklist.GPUProcessOnly_AllowInMainProcess | test completed (time: 0ms)
   INFO -  TEST-START | TestDllBlocklist.SocketProcessOnly_AllowInMainProcess
   INFO -  TEST-PASS | TestDllBlocklist.SocketProcessOnly_AllowInMainProcess | test completed (time: 0ms)
   INFO -  TEST-START | TestDllBlocklist.UtilityProcessOnly_AllowInMainProcess
   INFO -  TEST-PASS | TestDllBlocklist.UtilityProcessOnly_AllowInMainProcess | test completed (time: 0ms)
   INFO -  TEST-START | TestDllBlocklist.GMPluginProcessOnly_AllowInMainProcess
   INFO -  TEST-PASS | TestDllBlocklist.GMPluginProcessOnly_AllowInMainProcess | test completed (time: 0ms)
   INFO -  TEST-START | TestDllBlocklist.MultipleEntriesDifferentProcesses_AllowInMainProcess
   INFO -  TEST-PASS | TestDllBlocklist.MultipleEntriesDifferentProcesses_AllowInMainProcess | test completed (time: 0ms)
   INFO -  TEST-START | TestDllBlocklist.MultipleEntriesSameProcessBackward_Block
WARNING -  TEST-UNEXPECTED-FAIL | TestDllBlocklist.MultipleEntriesSameProcessBackward_Block | Value of: hDll
   INFO -    Actual: false
   INFO -  Expected: true @ /builds/worker/checkouts/gecko/toolkit/xre/dllservices/tests/gtest/TestDLLBlocklist.cpp:252
WARNING -  TEST-UNEXPECTED-FAIL | TestDllBlocklist.MultipleEntriesSameProcessBackward_Block | test completed (time: 0ms)
   INFO -  TEST-START | TestDllBlocklist.MultipleEntriesSameProcessForward_Block
WARNING -  TEST-UNEXPECTED-FAIL | TestDllBlocklist.MultipleEntriesSameProcessForward_Block | Value of: hDll
   INFO -    Actual: false
   INFO -  Expected: true @ /builds/worker/checkouts/gecko/toolkit/xre/dllservices/tests/gtest/TestDLLBlocklist.cpp:271
WARNING -  TEST-UNEXPECTED-FAIL | TestDllBlocklist.MultipleEntriesSameProcessForward_Block | test completed (time: 0ms)
   INFO -  TEST-START | TestDllBlocklist.NoOpEntryPoint
   INFO -  TEST-PASS | TestDllBlocklist.NoOpEntryPoint | test completed (time: 0ms)
   INFO -  TEST-START | TestDllBlocklist.UserBlocked
   INFO -  TEST-PASS | TestDllBlocklist.UserBlocked | test completed (time: 0ms)
   INFO -  TEST-START | TestDllBlocklist.BlocklistIntegrity
   INFO -  TEST-PASS | TestDllBlocklist.BlocklistIntegrity | test completed (time: 0ms)
   INFO -  TEST-START | TestDllBlocklist.BlockThreadWithLoadLibraryEntryPoint
   INFO -  TEST-PASS | TestDllBlocklist.BlockThreadWithLoadLibraryEntryPoint | test completed (time: 0ms)
   INFO -  TEST-START | TestDllBlocklist.SingleNotification
   INFO -  TEST-PASS | TestDllBlocklist.SingleNotification | test completed (time: 5ms)

To reproduce this on Try yourself, you can apply the below two patches to ESR115 tip:
https://hg.mozilla.org/try/rev/500a0a128668c62a1f561ee3575456363118311e
https://hg.mozilla.org/try/rev/ffbbf9e293b8807bb4d1d97e2c47cf413ead8b19

Push using ./mach try empty and then use the Add new jobs (search) function in Treeherder to add the Windows 7 opt GTest job to the push. It will be orange due to a number of other failures also occurring, but it should get far enough to at least run TestDllBlocklist and be findable in the logs.

The three places that are failing are where we're loading the library as LOAD_LIBRARY_AS_IMAGE_RESOURCE - the test expects that these will not be blocked but they are being blocked. (only on Win7, presumably)

My suspicion is that these are meant to be LOAD_LIBRARY_AS_DATAFILE instead. I'm not sure why this makes a difference only on Win7, though.

Assignee: nobody → gstoll
Status: NEW → ASSIGNED

Actually, I'm going to see whether Yannis's fix for bug 1733532 fixes this.

Severity: -- → S3
Priority: -- → P3

Our blocklist code must allow loading blocked modules using
LoadLibraryExW with LOAD_LIBRARY_AS_IMAGE_RESOURCE, so that we can
collect information about them when we want to send untrusted module
pings. This means that we need a trustworthy way to distinguish between
these loads and regular DLL loads.

Currently, we do the distinction by looking at the AllocationProtect
field for the virtual memory mapped for the view. This solution was
introduced with bug 1702717, but unfortunately it doesn't work with
Windows 7. This - mixed with other reasons - has resulted in the crash
spike in bug 1842368.

We should thus move to a more trustworthy solution to distinguish
between these two kinds of DLL loads. The new solution is to instead
check whether the permission to map executable views was asked when the
section that we are mapping was created. Because this solution is past
proof, it also has more chances to be future proof.

Attachment #9342783 - Attachment is obsolete: true

In mozilla::freestanding::patched_NtMapViewOfSection, the DLL blocklist
code relies on implicit assumptions about how LoadLibrary(Ex)W and
Thread32Next work internally.

Some of these assumptions turned out to be false on Windows 7. This led
to crash spikes with 115 release; first in bug 1837242, and then later
in bug 1842368. After dealing with the crash spikes, we realized that
some blocklist gtests are still failing on Windows 7, due to such wrong
assumptions. So, in bug 1842088, we are changing the assumptions.

This patch adds a dedicated cppunit test to test these assumptions in
isolation. It is easier to run than blocklist gtests, and should lead to
easier diagnostics in case the assumptions break in the future. This
test also checks the Thread32Next assumptions, which protect us against
the crash from bug 1733532.

Depends on D183530

Blocks: 1845163
Pushed by yjuglaret@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/aa7e824fcdb5 Avoid blocking blocked modules when loaded as image resources on Windows 7. r=gstoll https://hg.mozilla.org/integration/autoland/rev/114dbd0f8526 Explicitly test DLL blocklist assumptions in isolation with a cppunit test. r=mhowell

Please nominate this for ESR115 approval.

Flags: needinfo?(gstoll)

Our blocklist code must allow loading blocked modules using
LoadLibraryExW with LOAD_LIBRARY_AS_IMAGE_RESOURCE, so that we can
collect information about them when we want to send untrusted module
pings. This means that we need a trustworthy way to distinguish between
these loads and regular DLL loads.

Currently, we do the distinction by looking at the AllocationProtect
field for the virtual memory mapped for the view. This solution was
introduced with bug 1702717, but unfortunately it doesn't work with
Windows 7. This - mixed with other reasons - has resulted in the crash
spike in bug 1842368.

We should thus move to a more trustworthy solution to distinguish
between these two kinds of DLL loads. The new solution is to instead
check whether the permission to map executable views was asked when the
section that we are mapping was created. Because this solution is past
proof, it also has more chances to be future proof.

Original Revision: https://phabricator.services.mozilla.com/D183530

Uplift Approval Request

  • Is Android affected?: no
  • User impact if declined: won't be able to detect DLL blocklist regressions on Win7
  • Needs manual QE test: no
  • String changes made/needed: none
  • Risk associated with taking this patch: low
  • Code covered by automated testing: yes
  • Steps to reproduce for manual QE testing: n/a
  • Explanation of risk level: already in nightly, have pretty good automated tests around this
  • Fix verified in Nightly: yes

Our blocklist code must allow loading blocked modules using
LoadLibraryExW with LOAD_LIBRARY_AS_IMAGE_RESOURCE, so that we can
collect information about them when we want to send untrusted module
pings. This means that we need a trustworthy way to distinguish between
these loads and regular DLL loads.

Currently, we do the distinction by looking at the AllocationProtect
field for the virtual memory mapped for the view. This solution was
introduced with bug 1702717, but unfortunately it doesn't work with
Windows 7. This - mixed with other reasons - has resulted in the crash
spike in bug 1842368.

We should thus move to a more trustworthy solution to distinguish
between these two kinds of DLL loads. The new solution is to instead
check whether the permission to map executable views was asked when the
section that we are mapping was created. Because this solution is past
proof, it also has more chances to be future proof.

Original Revision: https://phabricator.services.mozilla.com/D183530

In mozilla::freestanding::patched_NtMapViewOfSection, the DLL blocklist
code relies on implicit assumptions about how LoadLibrary(Ex)W and
Thread32Next work internally.

Some of these assumptions turned out to be false on Windows 7. This led
to crash spikes with 115 release; first in bug 1837242, and then later
in bug 1842368. After dealing with the crash spikes, we realized that
some blocklist gtests are still failing on Windows 7, due to such wrong
assumptions. So, in bug 1842088, we are changing the assumptions.

This patch adds a dedicated cppunit test to test these assumptions in
isolation. It is easier to run than blocklist gtests, and should lead to
easier diagnostics in case the assumptions break in the future. This
test also checks the Thread32Next assumptions, which protect us against
the crash from bug 1733532.

Depends on D183530

Original Revision: https://phabricator.services.mozilla.com/D183757

Uplift Approval Request

  • Needs manual QE test: no
  • Fix verified in Nightly: yes
  • Explanation of risk level: just adding a test
  • Risk associated with taking this patch: very very low
  • String changes made/needed: no
  • Code covered by automated testing: yes
  • Steps to reproduce for manual QE testing: n/a
  • Is Android affected?: no
  • User impact if declined: less test coverage of assumptions that the DLL blocklist makes

Nominated both patches for ESR115 uplift. I think I did it right :-)

Flags: needinfo?(gstoll)

Looks like we ended up with a duplicate revision there? And D184593 has the proper dependency with D184594, but D184590 has the approval request.

Flags: needinfo?(gstoll)
Attachment #9345822 - Attachment is obsolete: true

Uplift Approval Request

  • Is Android affected?: no
  • User impact if declined: won't be able to detect DLL blocklist regressions on Win7
  • Needs manual QE test: no
  • String changes made/needed: none
  • Code covered by automated testing: yes
  • Risk associated with taking this patch: low
  • Steps to reproduce for manual QE testing: n/a
  • Explanation of risk level: already in nightly, have pretty good automated tests around this
  • Fix verified in Nightly: yes

Ugh, sorry. Both D184593 and D184594 have uplift requests now, and I abandoned D184590.

Flags: needinfo?(gstoll)

Comment on attachment 9343790 [details]
Bug 1842088 - Avoid blocking blocked modules when loaded as image resources on Windows 7. r=gstoll

Approved for 115.2esr.

Attachment #9343790 - Flags: approval-mozilla-esr115+
Attachment #9344217 - Flags: approval-mozilla-esr115+
Regressions: 1850969
Regressions: 1852841
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: