Robustness against third-party tampering with MSAA registry settings

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

Trunk
mozilla55
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: aes+)

Attachments

(1 attachment, 1 obsolete attachment)

I just discovered that Adobe Acrobat overwrites IAccessible's typelib GUID with its own, thus pointing all MSAA clients to some Acrobat stuff.

Who knows whether it is uninstalled correctly... but some of our marshaling errors that we've been seeing in COM are error codes such as TYPE_E_LIBNOTREGISTERED and TYPE_E_CANTLOADLIBRARY, which is suggesting to me that the standard MSAA components are no longer accessible due to registry corruption either from Acrobat or some other application.

I think we should see if we can override these settings by using registration-free COM tags to point to the correct system GUIDs in xul.dll's manifest. That would force anything running inside xul.dll to ignore the registry and always use the right things.
These entries override the registry, thus forcing COM to use the canonical GUIDs for resolving IAccessible.

Here's what the attributes in the comInterfaceExternalProxyStub tag represent:

iid is set to IID_IAccessible

proxyStubClsid32 is set to the GUID of the universal marshaler (i.e. the CLSID that consumes typelibs to generate proxies)

tlbid is the GUID of the IAccessible typelib that is embedded in oleacc.dll. This GUID is identical across all supported Windows versions.


Note that these changes are perfectly benign in non-a11y builds, so I didn't consider it necessary to worry about whether or not ACCESSIBILITY is defined (plus merging into these xml files would be an unnecessary pain in the butt).

I am updating both firefox and plugin-container manifests to ensure that a11y queries to plugins are also unaffected by registry tampering.


I was able to confirm that this patch eliminates the Adobe crap from being loaded in our process when a11y instantiates. Hopefully it helps with other corruption as well.
Attachment #8848245 - Flags: review?(jmathies)
As a further note, I changed the exe manifests as opposed to adding it to a manifest for xul.dll because the contents of the exe's manifest is implicitly activated for the lifetime of the process, whereas dll manifests would require explicit (de)activation would would add significant complexity.
Are we good about IAccessible2, ISimpleDOMNode, IAccessibleEx stuff?

Does the approach interfere with Adobe's plugin accessibility, i.e is it still accessible?
This really only affects interfaces that come with Windows and are registered in the system registry, so IAccessible and IAccessibleEx are the only ones that could potentially be affected.

Looking at my own system, it doesn't look to me like IAccessibleEx was touched by Acrobat.

This should not interfere with Flash accessibility.
Comment on attachment 8848245 [details] [diff] [review]
Add comInterfaceExternalProxyStub entries for IAccessible to firefox and plugin-container manifests

Interesting feature - some background for people interested. This looks good to me.
https://msdn.microsoft.com/en-us/library/ms973913.aspx
Attachment #8848245 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ca413eb20998d25a649c66aea14d7e8f094531
Bug 1348069: Add comInterfaceExternalProxyStub entries for IAccessible to firefox and plugin-container manifests; r=jimm
My bad, 64-bit doesn't use that typelib. I'll have to split this up so that these manifest changes only apply to 32-bit builds.
Comment on attachment 8852609 [details] [diff] [review]
Add comInterfaceExternalProxyStub entries for IAccessible to 32-bit firefox and plugin-container manifests

Review of attachment 8852609 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good from a build system perspective.
Attachment #8852609 - Flags: review?(gps) → review+
Attachment #8852609 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4e6fb6fb40afe99415863dcdc771dcd6f6e9e02
Bug 1348069: Add comInterfaceExternalProxyStub entries for IAccessible to 32-bit firefox and plugin-container manifests; r=jimm, gps
https://hg.mozilla.org/mozilla-central/rev/d4e6fb6fb40a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1354785
Depends on: 1358276
You need to log in before you can comment on or make changes to this bug.