1536227 - (CVE-2019-11758) Crash in [@ nsTArray_Impl<T>::RemoveElement<T>(class `anonymous namespace'::ParentImpl*& const, const class nsDefaultComparator<T>& const) | mozilla::a11y::DocAccessibleParent::RecvHideEvent]

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[[:philipp]]

This bug is for crash report bp-ee4cf772-6446-4451-86a1-253e90190315.

Top 7 frames of crashing thread:

0 xul.dll static bool nsTArray_Impl< xpcom/ds/nsTArray.h:1778
1 xul.dll mozilla::a11y::DocAccessibleParent::RecvHideEvent accessible/ipc/DocAccessibleParent.cpp:184
2 xul.dll mozilla::a11y::PDocAccessibleParent::OnMessageReceived ipc/ipdl/PDocAccessibleParent.cpp:287
3 xul.dll void mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2078
4 xul.dll nsresult mozilla::ipc::MessageChannel::MessageTask::Run ipc/glue/MessageChannel.cpp:1968
5 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1179
6 xul.dll XPTC__InvokebyIndex 

these crashes are starting to show up since firefox 63 on 64bit versions of windows - in rather low volume though. most of the affected builds and comments are in chinese.

a portion of crashes are looking like wild-pointer issues.

Comment 1

[yxu]

Through the information I get from some users, I do see some 3rd-part programs that use accessible api collect data. A crash is not necessarily the end result, there are some hangs, and it affects network loading. There usually have some dll injections. I think after release firefox 66, we can see if Bug 1306406 is helpful.

Comment 2

[Daniel Veditz [:dveditz]]

Interesting correlations, like
(100.0% in signature vs 01.41% overall) Module "safemon64.dll" = true

Comment 3

[Eitan Isaacson [:eeejay]]

Adding Jamie. Quick fix would be to disable that dll, but I don't think that fixes the root cause, which we are still responsible for.

Comment 4

[Pascal Chevrel:pascalc]

The crash rate on beta and release is noticable, Jamie could we get somebody to investigate a solution or a mitigation for this bug please? Thanks

Comment 5

[James Teh [:Jamie]]

From what I can see, this happens when a content process tells the parent process that an accessible has been hidden (removed). The parent process crashes in ProxyAccessible::RemoveChild when we try to remove the just hidden ProxyAccessible from its parent. This suggests that the parent was already destroyed or that it somehow got destroyed between when we fetched it and when we called RemoveChild. Maybe re-entry somewhere within a win event callback? I don't know what code could execute on our side to destroy a ProxyAccessible while we're blocked on an IPC call, though; we should only mutate the ProxyAccessible tree in response to IPC calls (and I'm fairly certain it's not possible to receive an IPC call while an earlier call is still running).

Without being able to reproduce this, debugging it and fixing it properly is going to be practically impossible.

To mitigate this, given the very strong correlation with safemon64.dll, I think we should just block this. Brief searching suggests this is related to 360 Safeguard/360 Total Security, which is security/anti-malware software. However, it's unclear why it needs to inject a dll into our process anyway.

Aaron, thoughts? Do we have any kind of process for approving/determining whether we should block in-process injection by "security" software?

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to James Teh [:Jamie] from comment #5)

Aaron, thoughts? Do we have any kind of process for approving/determining whether we should block in-process injection by "security" software?

Our current "process" (I am using that term rather loosely) is to add it to the blocklist and send it to me for review. Ideally we'd also verify this with a QA pass to make sure that the browser still starts with that product installed, though I checked our new Untrusted Modules telemetry data and I don't see any loads in there that look particularly risky.

Comment 7

[James Teh [:Jamie]]

I can provide a patch and a try build. Do we have someone who can test this with the security software packages in question?

Comment 8

[James Teh [:Jamie]]

Attachment: Bug 1536227: Block safemon64.dll (360 Safeguard/360 Total Security) for causing a11y crashes.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to James Teh [:Jamie] from comment #7)

I can provide a patch and a try build. Do we have someone who can test this with the security software packages in question?

I think QA has done this before. Maybe it needs a PI-request?

Comment 10

[Pascal Chevrel:pascalc]

(In reply to James Teh [:Jamie] from comment #7)

I can provide a patch and a try build. Do we have someone who can test this with the security software packages in question?

asking QA

Comment 11

[James Teh [:Jamie]]

Try run (Win64 opt build only):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9e51fb7427a3b36aec7d33e5e36ef979019fbf6

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Hey Jamie, what ever ended up happening with this bug?

Comment 13

[James Teh [:Jamie]]

As per comment 7 and comment 11, I've provided a patch and a try build, but it needs to be tested with the security software in question, ideally before it gets landed. I see qe-verify+ was set, but I don't really know what this means: I didn't hear anything from QA.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

qe-verify is something normally used for verifying a bug after it lands. A PI request should be submitted for testing this patch before landing.

Comment 15

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

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Jamie, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 16

[James Teh [:Jamie]]

I tried to submit a PI request, but don't have permission to access Jira. I've emailed pi-request and am awaiting a response.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you also request sec-approval on the patch before landing anything? Thanks!

Comment 18

[Rares Doghi, Desktop QA]

Hi guys, I've been trying to reproduce this crash on release and beta for a while now on Windows 10 as well as 7 using the 32 bit version of Firefox as well as the 64 bit version of it and without any success, I even tried downloading the chinese version of 360 from https://www.360.cn/ and used the zh-CN build of Firefox, still no luck.

I've also downloaded the build from https://www.360totalsecurity.com/ (both of these versions have the safemon64.dll) but I can't trigger the crash with sync activated, 20 tabs open most popular websites, youtube playing and still no crash.

Are there any settings or preferences in 360 that might cause the crash in Firefox ? Or at least some forums where this issue is reported so we can ask for some more information ?

Comment 19

[[:philipp]]

i can just go by crash reports: they are all coming from 64bit firefox builds, comments in reports often talk about media playback but also flash content that would be triggering this crash.
next to having safemon64.dll hook into the browser process, a11y is the other factor contributing to the crashes. for those users with zh-cn builds, wps.exe (apparently related to WPS Office, https://www.wps.com) is the most common recorded client on that's triggering a11y services to become active.

if there is no way of reproducing the crash itself, it may be beneficial to trigger an artificial crash with the try build to see if the patch is successful of getting the safemon64.dll module out of our process and that this causes no other obvious negative impacts like loss of connectivity, etc....

Comment 20

[Daniel Veditz [:dveditz]]

I did find a small handful of crashes on en-US builds rather than zh-CN. Of the two I looked at both had a UAF-looking 0xe5e5e5e5e5e5e5e5 in rcx. One had the Qihu safemon64.dll mentioned above; the other didn't but had a bunch of .dlls signed by "Freedom Scientific" which makes accessibility devices (https://www.freedomscientific.com/).
bp-ebfb57cf-51be-45bc-87d2-8fb480190706
bp-8864f78f-68b4-42e3-b6a8-df0ec0190710

When do we send hide events to Accessibility nodes? How is the 360 software even invoking/using acessibility? Since there's a UAF involved in the call parent->RemoveChild(root); maybe something's not holding on to the parent long enough.

Comment 21

[James Teh [:Jamie]]

(In reply to Daniel Veditz [:dveditz] from comment #20)

I did find a small handful of crashes on en-US builds rather than zh-CN. Of the two I looked at both had a UAF-looking 0xe5e5e5e5e5e5e5e5 in rcx. One had the Qihu safemon64.dll mentioned above; the other didn't but had a bunch of .dlls signed by "Freedom Scientific" which makes accessibility devices (https://www.freedomscientific.com/).

Ug. That's probably with the JAWS screen reader.

When do we send hide events to Accessibility nodes?

This is sent whenever an accessibility object is removed from the tree; e.g. when a node is removed from the DOM or gets hidden.

How is the 360 software even invoking/using acessibility?

We honestly don't know, and figuring that out would take a lot of debugging. Given the strong correlation with this dll (and given that it doesn't have an obvious reason for needing a11y), we felt the easiest way to "stop the bleeding" was to block it.

Since there's a UAF involved in the call parent->RemoveChild(root); maybe something's not holding on to the parent long enough.

That would seem to be the case, but I can't see how that's possible unless there's some re-entry I'm not accounting for. See my thoughts in comment 5.

Comment 22

[James Teh [:Jamie]]

Comment on attachment 9057749 [details]
Bug 1536227: Block safemon64.dll (360 Safeguard/360 Total Security) for causing a11y crashes.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch doesn't fix the issue, but rather, blocks an injected third party dll that is very strongly correlated with the crash. Thus, there is nothing to learn about the issue from the patch other than a problematic dll being blocked for causing crashes.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: beta and release (but release has been declared wontfix)
• If not all supported branches, which bug introduced the flaw?: unknown
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Easy to create, low risk. Only difference is where in the file the entry is placed.
• How likely is this patch to cause regressions; how much testing does it need?: QA has verified (with a try build) that this doesn't cause unanticipated side effects with the problematic third party software installed. To be certain, it's worth having this tested again on nightly and beta once landed.

Comment 23

[Julien Cristau [:jcristau]]

[Tracking Requested - why for this release]: If it affects release then it presumably also affects esr68.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

(In reply to James Teh [:Jamie] from comment #22)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch doesn't fix the issue, but rather, blocks an injected third party dll that is very strongly correlated with the crash. Thus, there is nothing to learn about the issue from the patch other than a problematic dll being blocked for causing crashes.

Do we want to backport this?

sec-approval+ for trunk.

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/eae7203ff8a586da9eccc90764a74de96af70561
https://hg.mozilla.org/mozilla-central/rev/eae7203ff8a5

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR68 approval when you get a chance.

Comment 27

[Rares Doghi, Desktop QA]

Hi guys, I have tested this issue in our latest Nightly 70.0a1 (2019-07-17) on a windows 10 64bits with the 360 Total Security antivirus installed that had the above mentioned safemon64.dll, using over 30 tabs, multiple youtube videos playing in the background some on Loop, multiple flash sites with started games and a lot of tabs with some of the more popular websites i.e. reddit, 9gag, deviantart, twitter, facebook, pinterest and I can safely say that no crashes occurred. This issue seems Fixed in our latest Nightly build but please keep in mind that I was unable to reproduce this crash in Release or Beta either.

Comment 28

[James Teh [:Jamie]]

Comment on attachment 9057749 [details]
Bug 1536227: Block safemon64.dll (360 Safeguard/360 Total Security) for causing a11y crashes.

Beta/Release Uplift Approval Request

• User impact if declined: Crashes which we suspect to be caused by third party security software.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Test daily Firefox usage with 360 Total Security installed and ensure this does not cause instability or unreliability in 360 Total Security.
  Same with 360 Safeguard.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Blocks injection of third party security software into Firefox which has no known reason to inject. QA confirmed that running with this block does not negatively impact the third party software.
• String changes made/needed: None.

Comment 29

[James Teh [:Jamie]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)

Please nominate this for Beta and ESR68 approval when you get a chance.

This patch is somewhat speculative. There was a very high correlation for these crashes with the third party dll in question, but we can't know for certain that this particular dll was causing them. Therefore, I think it makes sense to get this into beta and observe for a while before uplifting to 68. Does this seem reasonable?

(In reply to Al Billings [:abillings] from comment #24)

Do we want to backport this?

sec-approval+ for trunk.

We want to backport it to beta at least. Is there an additional process I need to follow to get sec-approval for beta?

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9057749 [details]
Bug 1536227: Block safemon64.dll (360 Safeguard/360 Total Security) for causing a11y crashes.

Approved for 69.0b6.

(In reply to James Teh [:Jamie] from comment #29)

We want to backport it to beta at least. Is there an additional process I need to follow to get sec-approval for beta?

sec-approval only applies to trunk

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/302d6a3d1ca8673034456b04a90489f427793910

Comment 32

[Al Billings [:abillings - ex-MoCo]]

(In reply to James Teh [:Jamie] from comment #29)

We want to backport it to beta at least. Is there an additional process I need to follow to get sec-approval for beta?

No, just ask for beta approval on a patch like normal. Sec-approval is just for mozilla-central.

Comment 33

[Rares Doghi, Desktop QA]

Hi I have retested this issue in Our latest beta 69.0b6 and no Issues occur with the 360 Antivirus. I will mark this issue accordingly.

Comment 34

[yxu]

About two week ago I tried to contact 360 to ask about the purpose of this dll using the accessibility API. Whether the blocking affects its feature. But no response has been received so far. If there are any messages I will update. It seems 360 Antivirus software is not abnormal during the test. I think we can apply this fix even if we don't receive a reply.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Hi Jamie, should we take this on ESR68 for 68.1esr also? If so, please nominate :)

Comment 36

[James Teh [:Jamie]]

Ideally, since this fix is somewhat speculative, I'd like some confirmation that this reduces the crash rate on beta/release before nominating for ESR uplift. That said, despite comment 4 noting that this was significant on beta, I can't see many crashes on beta versions going back several months. Am I missing something here? If I'm correct, that would mean we'd need to wait for 69 final release to see if this improves things.

Comment 37

[Julien Cristau [:jcristau]]

[Tracking Requested - why for this release]: re-nominating for 68.2

Comment 38

[Ryan VanderMeulen [:RyanVM]]

This is looking good on Release 69 now. I think we're good to request ESR68 approval now :)

Comment 39

[James Teh [:Jamie]]

Comment on attachment 9057749 [details]
Bug 1536227: Block safemon64.dll (360 Safeguard/360 Total Security) for causing a11y crashes.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Crashes.
• User impact if declined: Crashes which we suspect to be caused by third party security software.
• Fix Landed on Version: 69
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Blocks injection of third party security software into Firefox which has no known reason to inject. QA confirmed that running with this block does not negatively impact the third party software.
• String or UUID changes made by this patch: None.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9057749 [details]
Bug 1536227: Block safemon64.dll (360 Safeguard/360 Total Security) for causing a11y crashes.

Fixes a startup crash caused by some third-party software. Approved for 68.2esr.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/050d4498c37c

Comment 42

[Rares Doghi, Desktop QA]

Hi, I have retested this issue in our latest esr68.2 build and after unblocking Firefox from the 360 Total security I couldnt find any issues, no crashes occured while testing flash games, flash sites, videos or some of the most popular websites. I think we can mark this issue as Verified, I will update the flags.