Closed Bug 1679741 Opened 3 years ago Closed 2 years ago

Crash in [@ ReadFile] with Beijing Kingsoft Security software

Categories

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

All
Windows

Tracking

(Fission Milestone:MVP, firefox-esr78 wontfix, firefox-esr91 wontfix, firefox89 wontfix, firefox90 wontfix, firefox91 wontfix, firefox93 wontfix, firefox94+ wontfix, firefox95+ fixed)

RESOLVED FIXED
Fission Milestone MVP
Tracking Status
firefox-esr78 --- wontfix
firefox-esr91 --- wontfix
firefox89 --- wontfix
firefox90 --- wontfix
firefox91 --- wontfix
firefox93 --- wontfix
firefox94 + wontfix
firefox95 + fixed

People

(Reporter: jcristau, Assigned: toshi)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

Crash report: https://crash-stats.mozilla.org/report/index/a99fa469-bb31-4bc6-bb81-5cdc60201129

Reason: EXCEPTION_ACCESS_VIOLATION_EXEC

Top 10 frames of crashing thread:

0  @0x246 
1 kernelbase.dll ReadFile 
2 kernelbase.dll WriteFile 
3 kisfdpro64.dll kisfdpro64.dll@0x1b00ce 
4 kernel32.dll ReadFileImplementation 
5 kisfdpro64.dll kisfdpro64.dll@0x13eb3 
6 kisfdpro64.dll kisfdpro64.dll@0x13b74 
7 kisfdpro64.dll kisfdpro64.dll@0x13fab 
8 kisfdpro64.dll kisfdpro64.dll@0x1b00ce 
9 kisfdpro64.dll kisfdpro64.dll@0x1b00ce 

We've seen this dll before, e.g. bug 1499194.

bp-49fb9a32-0b32-40b1-abda-647c40201129 is similar, different dll from the same vendor.

Why did this crash signature spike in late April and again in early June? The crashes are mostly in Beta and Nightly. The Beta crashes are not correlated with the Fission's Beta 90 experiment.

All Windows versions are affected, but 94% are Windows 7.

OS: Windows 7 → Windows
Hardware: Unspecified → All
Whiteboard: [not-a-fission-bug]

There's a strong correlation with the zh-cn locale, which accounts for over 80% of the recent crashes. (Perhaps not surprising given the dll's vendor.)

Spiking again ?

Unfortunately, this crash significantly affects Fission on Windows 7/8/8.1. We will probably need to delay Fission on Windows 7/8/8.1 until we can resolve this crash.

Severity: -- → S1
Fission Milestone: --- → MVP
Priority: -- → P1
Whiteboard: [not-a-fission-bug]

Tentatively assigning this bug to Toshi because he said he would to investigate.

The Kingsoft DLLs are kisfdpro64.dll and kwsui64.dll.

Assignee: nobody → tkikuchi

Here are versions of the crashing DLLs (from crash ping telemetry):

kisfdpro64.dll
2021.9.27.105
2021.9.14.34
2021.9.6.100
2021.8.26.99
2021.8.16.95
2021.8.4.91
2021.6.2.82
2021.4.26.78
2021.1.5.47
2020.12.9.38

kwsui64.dll
2021.9.14.102
2021.6.2.984
2020.4.10.24299

Luckily I was able to reproduce the same crash. Here are the steps.

  1. Download the installer of 金山毒覇 from https://www.duba.net/ and install it on Windows 7 (As of today, the version was 2021.9.27.105)
  2. Enable Fission on Nightly
  3. Set the homepage to https://www.newduba.cn/?f=foxdh&pru=1
  4. Launch Firefox

What happened is KERNELBASE!ReadFile called NtReadFile, but somehow it jumped to a near-null address. The interesting finding is that NtReadFile was hooked to InterposedNtReadFile by us. This may be a timing issue around the global variable gOriginalNtReadFile, and enabling Fission changes the timing when NtReadFile is called in the content process.

I'll debug this further, but I think blocking the module is not the right approach to this. IOInterposer's hooking of ntdll functions seems not stable. Because of bug 1666310, this hook is applied only in Nightly and early Beta for now. If this is a potential ship blocker of Fission, we should consider not enabling IOInterposer if Fission is enabled.

See Also: → 1705042
See Also: → 1666310

Here's what happens:

  1. Kingsoft (kwsprotect64.exe) hooks firefox.exe's entrypoint (firefox!wmainCRTStartup) and injects a code to load kwsui64.dll
  2. [Main thread] the entrypoint function loads kwsui64.dll via the injected code
  3. [Main thread] kwsui64.dll's DllMain creates a thread that starts from kwsui64+0x3120
  4. [kwsui64's thread] kwsui64.dll calls ReadFile for their named pipe
  5. [Main thread] our code applies IOInterposer's hooks (only in nightly and early beta)

There is a race between Step 4 and 5. When kwsui64's thread calls NtReadFile, the register r11 is usually 0x246. On the other hand, our thread applies a hook whose pattern is mov r11, <target>; jmp r11 onto NtReadFile. On Windows 7, this race magically gets the kwsui64's thread execute jmp r11 with the original value 0x246, resulting in this crash, execution AV at 0x246. This flow is the exact same on Windows 10, but somehow the crash never happens.

Any third-party module injected before InitPoisonIOInterposer can trigger this problem with or without Fission. A possible solution is to apply the hooks earlier. I think we can do that with our blocklist's technique or chromium-sandbox technique.

I just found bug 1646804 proposing the same thing. We can land a mitigation for this bug and work on a long-term solution in bug 1646804.

See Also: → 1646804

Kingsoft's modules are injected before IOInterposer and starts a thread that
calls NtReadFile. It conflicts with the main thread where IOInterposer hooks
NtReadFile.

This patch is a mitigation until we make IOInterposer compatible with such
third-party injections (bug 1646804).

Pushed by tkikuchi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e03fdd340bd
Not enable IOInterposer when Kingsoft Internet Security is installed. r=gerald
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

Your IOInterposer workaround checks for kwsui64.dll, but some of the Kingsoft crashes include kisfdpro64.dll but not kwsui64.dll.

This crash report's modules include Kingsoft DLLs kwsui64.dll and KSoftLaunch64.dll, but not kisfdpro64.dll. CORRECTION: I see now that my crash report search was incorrect.

https://crash-stats.mozilla.org/report/index/53b4451d-a18d-4677-b4c3-073e90211027#tab-modules

Should this patch also check for kisfdpro64.dll?

Flags: needinfo?(tkikuchi)

(In reply to Chris Peterson [:cpeterson] from comment #14)

Should this patch also check for kisfdpro64.dll?

I think it's not needed for two reasons. The first reason is all the crashes coming from kisfdpro64.dll (like this) always have kwsui64.dll loaded. The second is our third-party modules ping shows kisfdpro64.dll is loaded through kwsui64.dll. So checking kwsui64.dll is enough to detect all Kingsoft users.

Flags: needinfo?(tkikuchi)

(In reply to Toshihito Kikuchi [:toshi] from comment #15)

I think it's not needed for two reasons. The first reason is all the crashes coming from kisfdpro64.dll (like this) always have kwsui64.dll loaded. The second is our third-party modules ping shows kisfdpro64.dll is loaded through kwsui64.dll. So checking kwsui64.dll is enough to detect all Kingsoft users.

Sounds good. I see now that my crash report search was incorrect.

I'm setting status-firefox94=wontfix. We don't need to uplift this IOInterposer fix to Firefox 94 Release because the IOInterposer is only enabled in Nightly and Early Beta.

See Also: → 1706031
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: