1733532 - Crash in [@ mozilla::freestanding::patched_NtMapViewOfSection | Thread32Next] caused by Samsung SDS NASCA

Status: RESOLVED FIXED

(Firefox :: Launcher Process, defect)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/cfc4f3f1-22ba-4e80-b3ae-6c0880210928

Reason: EXCEPTION_ACCESS_VIOLATION_EXEC

Top 10 frames of crashing thread:

0 firefox.exe _security_check_cookie 
1 firefox.exe mozilla::freestanding::patched_NtMapViewOfSection browser/app/winlauncher/freestanding/DllBlocklist.cpp:486
2 kernel32.dll Thread32Next 
3 nsccor0364.dll nsccor0364.dll@0x279e6 
4 nsccor0364.dll nsccor0364.dll@0x27739 
5 ntdll.dll RtlpFreeHeapInternal 
6 ntdll.dll RtlSetLastWin32Error 
7 kernelbase.dll FlsGetValue 
8 ntdll.dll RtlFreeHeap 
9 nsccor0364.dll nsccor0364.dll@0x78109 

This crash seems to be caused by a DLL from a software called Samsung Document Security Solution (NASCA). I couldn't find much info online about it, mostly that it's some kind of endpoint data encryption / DRM tool. It's made by Samsung SDS but I couldn't find any more info on it. It's either a discontinued product or it's been rebranded to something else.

Comment 1

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

Since the crash volume is low (less than 15 per week), the severity is downgraded to S3. Feel free to change it back if you think the bug is still critical.

For more information, please visit auto_nag documentation.

Comment 2

[Yannis Juglaret [:yannis]]

The version numbers for the DLLs signed by Samsung SDS seems to use the build year-month-day format and we have crashes with e.g. the following versions of NSCCOR0364.DLL: 2022.9.13.364, 2022.8.5.364, 2022.2.28.359, 2020.1.17.333.

Samsung SDS initially developed their own solution for DRM called NASCA which at some point was rebranded as Samsung SDS Endpoint Security-Document and should now be the Document Security Management feature of Samsung SDS Endpoint Security which is still an actively supported (e.g. until 2026 for the current version) and developed product. Given that we have September 2022 versions of the DLL, the DLL name has likely been preserved throughout the evolution of the product. I'll try to reach the support for Samsung SDS Endpoint Security to let them know about this crash.

Samsung SDS Endpoint Security/Document Security Management: Encrypt all your documents preventing possible data leakage while, at the same time, allowing full usability using and sharing documents by the right users.

Samsung SDS Endpoint Security may be relying on Fasoo Enterprise DRM under the hood given that we have a 100% correlation with DLLs that live in C:\Program Files\Fasoo DRM, that Fasoo was created by former Samsung SDS employees, and that the functionalities of the product seem to match with the Document part of Samsung SDS Endpoint Security. That could be a second point of contact to try.

Fasoo Enterprise DRM/Advanced Data Protection: Protect, control and track sensitive documents at rest, in transit and in use persistently on any device at any time throughout the entire document lifecycle. By encrypting files and adding granular controls, you can limit editing, printing and sharing sensitive content with unauthorized users both inside and outside your organization. Fasoo Enterprise DRM (digital rights management) or sometimes referred to as information rights management (IRM) helps you manage insider threats, prevent data breaches and enforce the highest level of protection to unstructured data.

On a side note, the distribution of crashes suggests a correlation to office work days i.e. the use of enterprise products deployed on work computers.

Comment 3

[Yannis Juglaret [:yannis]]

I contacted both Samsung SDS and Fasoo. Fasoo replied quickly and says that the two products are independent, so the correlation makes me wonder if they are maybe incompatible.

Regarding the crash itself, it occurs while jumping from mozilla::freestanding::patched_NtMapViewOfSection to _security_check_cookie, both in firefox.exe. Apparently the former lives in a page that is currently mapped executable but not the latter (probably a result of interaction with third-party DLLs or programs). I wonder how this crash would evolve if we didn't use stack buffers in mozilla::freestanding::patched_NtMapViewOfSection, as that should remove the call to _security_check_cookie. I think this should fix the current crash, but we may get a different crash instead. I'll investigate if that's possible.

Comment 4

[Yannis Juglaret [:yannis]]

Attachment: Bug 1733532 - Avoid using stack buffers during calls to Thread32Next. r=gstoll,handyman

Comment 5

[Yannis Juglaret [:yannis]]

The variables that result in generating the stack cookie checks are mbi and sectionFileName. The proposed patch moves them to a helper function, so that on the path that is triggered in this crash, we do not need to go through a stack cookie check (coincidentally, this may result in a small performance boost since we will sometimes avoid unnecessary calls to NtQueryVirtualMemory). This can hopefully fix this crash, but if the two products are incompatible then a new, different crash may appear.

Comment 6

[Yannis Juglaret [:yannis]]

I would personally recommend, in this order:

• wait for the end of soft code freeze;
• push to nightly and waiting for ~1 week to make sure this change does not seem to introduce unexpected stability issues for users not impacted by this bug;
• request uplifts for beta and release if all looks good.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/82ef04760c5a
Avoid using stack buffers during calls to Thread32Next. r=gstoll

Comment 8

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/82ef04760c5a

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Since the crash volume is low (less than 15 per week), the severity is downgraded to S3. Feel free to change it back if you think the bug is still critical.

This spiked again, so re-raising.

Comment 10

[Yannis Juglaret [:yannis]]

Comment on attachment 9317015 [details]
Bug 1733532 - Avoid using stack buffers during calls to Thread32Next. r=gstoll,handyman

Beta/Release Uplift Approval Request

• User impact if declined: Integrating this patch could potentially avoid 1000~2000 main process crashes in 111.0.2 (if a 111.0.2 is planned).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes (in Beta). The users are mostly using Release channel except for a few Beta users. We've always had a few crashes in Beta (see below) and so far we received none in 112 beta versions, suggesting that the patch works.
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a small, low-impact change that has now lived for some time in Nightly and Beta without introducing any problems there (to the best of my knowledge). This looks like an easy win to prevent some users from crashing if a 111.0.2 is planned; although it's not a huge problem either to let them wait for 112.0.
• String changes made/needed:
• Is Android affected?: No

Crashes in beta versions sorted by Version:
5 111.0rc2 8 5.56 %
3 111.0rc1 13 9.03 %
8 111.0b5 6 4.17 %
19 111.0b4 3 2.08 %
30 111.0b3 1 0.69 %
2 110.0rc1 15 10.42 %
4 110.0b9 12 8.33 %
18 110.0b7 3 2.08 %
29 110.0b6 1 0.69 %
15 110.0b5 4 2.78 %
14 110.0b2 4 2.78 %
[...]

Comment 11

[Donal Meehan [:dmeehan]]

Comment on attachment 9317015 [details]
Bug 1733532 - Avoid using stack buffers during calls to Thread32Next. r=gstoll,handyman

There is no 111 dot release planned before 112 go-live.
This will ride the trains with 112.

Comment 12

[Yannis Juglaret [:yannis]]

Unfortunately this crash will be reintroduced in 115.0.1 after bug 1837242.

Comment 13

[Yannis Juglaret [:yannis]]

Attachment: Bug 1733532 - New attempt to avoid using stack buffers during calls to Thread32Next. r=gstoll

The previous attempt at avoiding the crash from bug 1733532 resulted in
breaking DLL blocklist code on older versions of Windows, in particular
Windows 7, which led to a crash spike (see bug 1837242).

To detect executable DLL mappings, we must not look at the protection
asked in the arguments of NtMapViewOfSection, but rather at the
protection that was used to create the section. We already do this
through NtQueryVirtualMemory, but in a helper function rather than
directly in patched_NtMapViewOfSection.

Because the helper function does not have MOZ_NO_STACK_PROTECTOR, it
does not avoid stack cookie checks when NtMapViewOfSection is called
from Thread32Next. To mitigate the crash from bug 1733532, we need to
move the call to NtQueryVirtualMemory to the main patched function, at
the (reasonable) cost of losing the stack cookie check on local
variable mbi since this function has MOZ_NO_STACK_PROTECTOR.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7a8474c209d
New attempt to avoid using stack buffers during calls to Thread32Next. r=gstoll

Comment 15

[Iulian Moraru]

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

Comment 16

[Yannis Juglaret [:yannis]]

Comment on attachment 9342706 [details]
Bug 1733532 - New attempt to avoid using stack buffers during calls to Thread32Next. r=gstoll

Beta/Release Uplift Approval Request

• User impact if declined: This crash was reintroduced in 115.0.1 after bug 1837242 and would still be present in 115.0.2.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change just moves code around without changing the logic. But it will be enough to avoid the 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: We have observed that a high proportion of users experiencing this crash where on the ESR channel.
• User impact if declined: This crash was reintroduced in 115.0.1 after bug 1837242 and would still be present in 115.0.2.
• Fix Landed on Version: 115
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change just moves code around without changing the logic. But it will be enough to avoid the crash.

Comment 17

[Dianna Smith [:diannaS]]

Comment on attachment 9342706 [details]
Bug 1733532 - New attempt to avoid using stack buffers during calls to Thread32Next. r=gstoll

Approved for 116.0b3

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/c495a056c4cc

Comment 19

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c495a056c4cc

Comment 20

[Donal Meehan [:dmeehan]]

Comment on attachment 9342706 [details]
Bug 1733532 - New attempt to avoid using stack buffers during calls to Thread32Next. r=gstoll

Approved for 115.0.2
Approved for 115.0.2esr

Comment 21

[Pulsebot]

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

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/f327d181b3e3

Comment 23

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/08d45dd7217d