1837242 - Crash in [@ kisfdpro64.dll]

Status: VERIFIED FIXED

(External Software Affecting Firefox :: Other, defect, P2)

Description

[Donal Meehan [:dmeehan]]

Crash report: https://crash-stats.mozilla.org/report/index/1fcd6ab6-9f28-40ed-ab0b-fbcfb0230607

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 4 frames of crashing thread:

0  kisfdpro64.dll  kisfdpro64.dll@0x1348c8  
1  kisfdpro64.dll  kisfdpro64.dll@0xca9cb  
2  kisfdpro64.dll  kisfdpro64.dll@0x1f66e8  
3  kisfdpro64.dll  kisfdpro64.dll@0xccceb  

Comment 1

[Donal Meehan [:dmeehan]]

:gstoll unsure if this is in the correct Product/Component for triage
It's new in 115.0 beta builds

Comment 2

[Greg Stoll :gstoll]

This is the right place - thanks! I'll take a look.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 20 desktop browser crashes on beta (startup)

For more information, please visit BugBot documentation.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug is marked as tracked for firefox115 (beta). However, the bug still isn't assigned.

:gcp, could you please find an assignee for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 5

[Greg Stoll :gstoll]

This is interesting because the call stacks for the crashes are entirely inside kisfdpro64.dll, but since this only started showing up in 115 beta builds it seems like something must have changed in Firefox to trigger this. (see also bug 1837246, which is also published by Kingsoft and started in 115 beta)

I think we will have to block this unless we can get a contact at Kingsoft.

Comment 6

[Greg Stoll :gstoll]

Windows DLLBlocklist request form

1. How were we aware of the problem?
   Topcrasher

2. What is a suspicious product causing the problem?
   Kingsoft Internet Security

3. Is the product downloadable? If so, do we have a local repro?
   Yes, it can be downloaded at https://www.ijinshan.com/ - I am able to reproduce the DLL getting loaded into Firefox but not the crash.

4. Which OS versions does the problem occur on?
   Windows 7, 10, 11

5. Which process types does the problem occur on?
   Only in the browser process.

6. What is the maximum version of the module in the crash reports?
   2023.5.31.209

7. Is the issue fixed by a newer version of the product?
   According to telemetry, 2023.5.31.209 is the newest version of the module we've seen.

8. Do we have data about the module in the third-party-module ping?
   Yes

9. Do we know how the module is loaded?
   The DLL gets loaded from a new thread (presumably injected by an outside process). There is a risk that blocking the DLL will make something bad happen, but in practice this seems to not happen. Firefox seems to run fine when blocking the DLL locally.

10. Describe your conclusion.
    We should block kisfdpro64.dll versions 2023.5.31.209 and earlier in the browser process only.

Comment 7

[Greg Stoll :gstoll]

Attachment: Bug 1837242 - block Kingsoft DLL (kisfdpro64.dll) to avoid crashes r=gsvelto

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4378236e581
block Kingsoft DLL (kisfdpro64.dll) to avoid crashes r=gsvelto

Comment 9

[Sandor Molnar[:smolnar]]

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

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:gstoll, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox115 to wontfix.

For more information, please visit BugBot documentation.

Comment 11

[Greg Stoll :gstoll]

Attachment: Bug 1837242 - block Kingsoft DLL (kisfdpro64.dll) to avoid crashes

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

Comment 12

[Phabricator Automation]

Uplift Approval Request

• String changes made/needed: no
• Is Android affected?: no
• Fix verified in Nightly: yes
• User impact if declined: users won't get a fix for a topcrasher
• Risk associated with taking this patch: low
• Needs manual QE test: no
• Explanation of risk level: just adding a blocklist entry
• Code covered by automated testing: no
• Steps to reproduce for manual QE testing: n/a

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 14

[Donal Meehan [:dmeehan]]

:gstoll could you take another look at this?
The patch was uplfted for 115.0b6 but there are still crashes

Comment 15

[Greg Stoll :gstoll]

Sigh, in the meantime a newer version of this DLL has been released, and we didn't block that so it's still crashing. Maybe we should block all versions...

Comment 16

[Greg Stoll :gstoll]

Attachment: Bug 1837242: Block Kingsoft DLL (kisfdpro64.dll) in all versions to avoid crashes r=gsvelto

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b736ef0e08a1
Block Kingsoft DLL (kisfdpro64.dll) in all versions to avoid crashes r=gsvelto

Comment 18

[Greg Stoll :gstoll]

Attachment: Bug 1837242: Block Kingsoft DLL (kisfdpro64.dll) in all versions to avoid crashes r=gsvelto

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

Comment 19

[Phabricator Automation]

Uplift Approval Request

• Steps to reproduce for manual QE testing: n/a
• Code covered by automated testing: no
• String changes made/needed: no
• Is Android affected?: no
• User impact if declined: users won't get a fix for a topcrasher
• Risk associated with taking this patch: low
• Explanation of risk level: just extended a blocklist entry to block all versions
• Fix verified in Nightly: yes
• Needs manual QE test: no

Comment 20

[Cristian Tuns]

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

Comment 21

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6722dee8d3773705cd9d4eddb3baf3333015aed2

Comment 22

[Greg Stoll :gstoll]

Contacted a public address for Kingsoft in the hopes they'll be able to take a look at this.

Comment 23

[Donal Meehan [:dmeehan]]

(In reply to Greg Stoll from comment #22)

Contacted a public address for Kingsoft in the hopes they'll be able to take a look at this.

:gstoll users on 115.0 are hitting this. Any response from Kingsoft?

Comment 24

[Yannis Juglaret [:yannis]]

This crash occurs while kisfdpro64.dll is reading xul.dll entirely in memory, to search for patterns in it. When it hits the .retplne section, it crashes because of its access rights, as it is not allowed to read these pages. In 114, xul.dll did not have a .retplne section; probably because we were using a different SDK version (bug 1832467, thanks [:RyanVM]). See also the bugs I am attaching.

We may have a different problem that our blocklist code fails to setup properly on Windows 7 (and Windows 10 1809), which would also explain our problems in bug 1833793. The crash spike with 115.0 release is mostly Windows 7 users, and a bit of Windows 10 1809 (10.0.17763) users. Windows 10 22H2 (10.0.19045) users do not seem impacted even though it was a high proportion of the crashes in beta, before we blocked the DLLs; so the blocking seems to work for them.

Comment 25

[Greg Stoll :gstoll]

(In reply to Donal Meehan [:dmeehan] from comment #23)

(In reply to Greg Stoll from comment #22)

Contacted a public address for Kingsoft in the hopes they'll be able to take a look at this.

:gstoll users on 115.0 are hitting this. Any response from Kingsoft?

I have not gotten any response from Kingsoft. Yannis and I are looking at why the block isn't working. (as per comment 24)

Comment 26

[Yannis Juglaret [:yannis]]

I can confirm that DLL blocking and about:third-party are broken when testing on a Windows 10 1803 physical machine and a 1809 VM, starting with Firefox 112 release:

• In Firefox 111, third-party DLLs are listed, I can block kisfdpro64.dll, and it works (if I attach to the main process, the DLL is not loaded).
• If I update to 112, third-party DLLs are not listed, kisfdpro64.dll appears as blocked (because I blocked it while in 111) but in fact the DLL is nonetheless loaded into the main process.

I believe that my patch in bug 1733532 has broken the blocklist for old versions on Windows and will work on fixing that.

Comment 27

[Pascal Chevrel:pascalc]

Thanks. Yannis for the heads up, marking 115 as affected as we are still crashing, also marking it as a release blocker as this is a high volume of crashes for a release just shipped, we will want a fix in a dot release.

Comment 28

[Yannis Juglaret [:yannis]]

Attachment: Bug 1837242 - Fix the DLL blocklist code for older versions of Windows. r=gstoll

While fixing a crash in bug 1733532, we accidentally broke the DLL
blocklist on older versions of Windows (Windows 7, some versions
of Windows 10, and possibly Windows 8 and 8.1). This is currently
preventing us from mitigating crashes with third-party injected DLLs, in
particular the crash incident from bug 1837242. Considering the volumes
involved, let's temporarily reintroduce bug 1733532 to ensure everyone
has a working blocklist, and deal with bug 1733532 later.

Comment 29

[Yannis Juglaret [:yannis]]

Attachment: blocklist.release.patch

Comment 30

[Yannis Juglaret [:yannis]]

Comment on attachment 9342601 [details] [diff] [review]
blocklist.release.patch

HG changeset patch

User Yannis Juglaret <[e-mail]>

Date 1688661533 -7200

Thu Jul 06 18:38:53 2023 +0200

Node ID 7723771c88a19c656e2b8d47103247b2fd106164

Parent d8b26f0caa1febd65581c8a1bc08f9dc7d6dd648

Bug 1837242 - Fix the DLL blocklist code for older versions of Windows. r=gstoll

While fixing a crash in bug 1733532, we accidentally broke the DLL
blocklist on older versions of Windows (Windows 7, some versions
of Windows 10, and possibly Windows 8 and 8.1). This is currently
preventing us from mitigating crashes with third-party injected DLLs, in
particular the crash incident from bug 1837242. Considering the volumes
involved, let's temporarily reintroduce bug 1733532 to ensure everyone
has a working blocklist, and deal with bug 1733532 later.

Differential Revision: https://phabricator.services.mozilla.com/D182917

diff --git a/browser/app/winlauncher/freestanding/DllBlocklist.cpp b/browser/app/winlauncher/freestanding/DllBlocklist.cpp
--- a/browser/app/winlauncher/freestanding/DllBlocklist.cpp
+++ b/browser/app/winlauncher/freestanding/DllBlocklist.cpp
@@ -549,13 +549,6 @@ NTSTATUS NTAPI patched_NtMapViewOfSectio
return stubStatus;
}

• if (!(aProtectionFlags & kPageExecutable)) {
• // Bail out early if an executable mapping was not asked. In particular,
• // we will not use stack buffers during calls to Thread32Next, which can
• // result in crashes with third-party software (see bug 1733532).
• return stubStatus;
• }
• return AfterMapExecutableViewOfSection(aProcess, aBaseAddress, stubStatus);
  }

diff --git a/mozglue/misc/NativeNt.h b/mozglue/misc/NativeNt.h
--- a/mozglue/misc/NativeNt.h
+++ b/mozglue/misc/NativeNt.h
@@ -972,6 +972,11 @@ class MOZ_RAII PEHeaders final {
auto dirEnt =
reinterpret_cast<PIMAGE_RESOURCE_DIRECTORY_ENTRY>(aCurLevel + 1) +
aCurLevel->NumberOfNamedEntries;

• if (!(IsWithinImage(dirEnt) &&

•      IsWithinImage(&dirEnt[aCurLevel->NumberOfIdEntries - 1].Id))) {
  

•  return nullptr;
  

• }
• for (WORD i = 0; i < aCurLevel->NumberOfIdEntries; ++i) {
  if (dirEnt[i].Id == aId) {
  return &dirEnt[i];

Comment 31

[Yannis Juglaret [:yannis]]

Comment on attachment 9342598 [details]
Bug 1837242 - Fix the DLL blocklist code for older versions of Windows. r=gstoll

Beta/Release Uplift Approval Request

• User impact if declined: The blocklist will not work for Windows 7 users (and some Windows 10 users, and potentially 8 and 8.1 users). These users will experiment crashes which we are trying to mitigate, in particular the crash incident with Kingsoft.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Expected behavior:

• On Windows 7 64-bit
• With Kingsoft Security installed
• Install and launch Firefox 64-bit
• Navigate to about:third-party
• 2 DLLs are listed: kwsui64.dll and kisfdpro64.dll
• Expand the information for kisfdpro64.dll by clicking the “v” icon
• It shows Status: Blocked for the Main process
• Restart Firefox, repeat the steps, the same result is expected

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch does not introduce new logic. It just removes the problematic path that was introduced in bug 1733532, and adds simple sanity checks to avoid a crash.
• String changes made/needed:
• Is Android affected?: No

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This patch is especially important for Windows 7 users who will be redirected to ESR 115 after end of support.
• User impact if declined: The blocklist will not work for Windows 7 users (and some Windows 10 users, and potentially 8 and 8.1 users). These users will experiment crashes which we are trying to mitigate, in particular the crash incident with Kingsoft.
• Fix Landed on Version: 115
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch does not introduce new logic. It just removes the problematic path that was introduced in bug 1733532, and adds simple sanity checks to avoid a crash.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b14fb1500cc
Fix the DLL blocklist code for older versions of Windows. r=gstoll

Comment 33

[Dianna Smith [:diannaS]]

Comment on attachment 9342598 [details]
Bug 1837242 - Fix the DLL blocklist code for older versions of Windows. r=gstoll

Approved for 116.0b2

Comment 34

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1b6b2288a0f4

Comment 35

[Donal Meehan [:dmeehan]]

Comment on attachment 9342598 [details]
Bug 1837242 - Fix the DLL blocklist code for older versions of Windows. r=gstoll

Approved for 115.0.1

Approved for 115.0.1esr

Comment 36

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr115/rev/cdb5fb7c2c9c (FIREFOX_ESR_115_0_X_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-esr115/rev/f3068b8a97f8 (esr115 default)

Comment 37

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-release/rev/a88637810806

Comment 38

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/0b14fb1500cc

Comment 39

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Verified the fix on Firefox 115.0.1, 115.0.1esr and 116.0b2 on both Windows 7 and Windows 10 and everything is working as expected.