Closed Bug 1536227 (CVE-2019-11758) Opened 3 years ago Closed 2 years ago

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

Categories

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

63 Branch
x86_64
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 70+ verified
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 + verified
firefox70 + verified

People

(Reporter: philipp, Assigned: Jamie)

Details

(5 keywords, Whiteboard: [adv-main69+][adv-esr68.2-rollup])

Crash Data

Attachments

(1 file)

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.

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.

Group: core-security → dom-core-security

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

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.

Group: dom-core-security → layout-core-security
Keywords: sec-high

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

Flags: needinfo?(jteh)

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?

Assignee: nobody → jteh
Flags: needinfo?(jteh) → needinfo?(aklotz)

(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.

Flags: needinfo?(aklotz)

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

(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?

(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

Flags: qe-verify?
Flags: qe-verify? → qe-verify+

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

Flags: needinfo?(jteh)

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.

Flags: needinfo?(jteh)

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.

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.

Flags: needinfo?(jteh)
Priority: -- → P1

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.

Flags: needinfo?(jteh)

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

Flags: needinfo?(jteh)

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 ?

Flags: needinfo?(yxu)
Flags: needinfo?(madperson)
Flags: needinfo?(dveditz)

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....

Flags: needinfo?(madperson)

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.

Flags: needinfo?(dveditz)
Keywords: csectype-uaf

(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.

Flags: needinfo?(jteh)

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.
Attachment #9057749 - Flags: sec-approval?

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

(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.

Attachment #9057749 - Flags: sec-approval? → sec-approval+
Group: layout-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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

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 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.
Flags: needinfo?(jteh)
Attachment #9057749 - Flags: approval-mozilla-beta?

(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?

Flags: needinfo?(ryanvm)
Flags: needinfo?(abillings)
QA Whiteboard: [qa-triaged]

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

Flags: needinfo?(ryanvm)
Attachment #9057749 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

(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.

Flags: needinfo?(abillings)

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.

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.

Flags: needinfo?(yxu)

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

Flags: needinfo?(jteh)

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.

Flags: needinfo?(jteh)
Whiteboard: [adv-main69+]

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

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

Flags: needinfo?(jteh)

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.
Flags: needinfo?(jteh)
Attachment #9057749 - Flags: approval-mozilla-esr68?

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.

Attachment #9057749 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Whiteboard: [adv-main69+] → [adv-main69+][adv-esr68.2+]

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.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Whiteboard: [adv-main69+][adv-esr68.2+] → [adv-main69+][adv-esr68.2-rollup]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.