1448505 - Probable free of uninitialized pointer in ProxyAccessible::RelationByType()

Status: RESOLVED FIXED

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

Description

[mozillabugs]

ProxyAccessible::RelationByType() (accessible\ipc\win\ProxyAccessible.cpp) probably can attempt to free an uninitialized pointer.

The bug is on line 394, which doesn't initialize |targets|. If something inside |acc->get_relationTargetsOfType()| (resolves to isaAccessible::get_relationTargetsOfType()) on line 396 fails before that function allocates memory for |targets| (e.g., the Accessible object becomes defunct), line 407 will attempt to free an uninitialized pointer, probably causing insecure behavior.

I do not currently have a POC (how do you make this function run?)

BTW, it also appears that lines 379, 391, and 398 are meant to read

	return nsTArray<ProxyAccessible*>();

instead of

	nsTArray<ProxyAccessible*>();

374: nsTArray<ProxyAccessible*>
375: ProxyAccessible::RelationByType(RelationType aType) const
376: {
377:   RefPtr<IAccessible2_2> acc = QueryInterface<IAccessible2_2>(this);
378:   if (!acc) {
379:     nsTArray<ProxyAccessible*>();
380:   }
381: 
382:   _bstr_t relationType;
383:   for (uint32_t idx = 0; idx < ArrayLength(sRelationTypePairs); idx++) {
384:     if (aType == sRelationTypePairs[idx].first) {
385:       relationType = sRelationTypePairs[idx].second;
386:       break;
387:     }
388:   }
389: 
390:   if (!relationType) {
391:     nsTArray<ProxyAccessible*>();
392:   }
393: 
394:   IUnknown** targets;
395:   long nTargets = 0;
396:   HRESULT hr = acc->get_relationTargetsOfType(relationType, 0, &targets, &nTargets);
397:   if (FAILED(hr)) {
398:     nsTArray<ProxyAccessible*>();
399:   }
400: 
401:   nsTArray<ProxyAccessible*> proxies;
402:   for (long idx = 0; idx < nTargets; idx++) {
403:     IUnknown* target = targets[idx];
404:     proxies.AppendElement(GetProxyFor(Document(), target));
405:     target->Release();
406:   }
407:   CoTaskMemFree(targets);
408: 
409:   return Move(proxies);
410: }

Comment 1

[Daniel Veditz [:dveditz]]

(In reply to mozillabugs from comment #0)
> I do not currently have a POC (how do you make this function run?)

From the directory, on windows with some kind of accessibility software or device running.

Comment 2

[mozillabugs]

(In reply to Daniel Veditz [:dveditz] from comment #1)
> (In reply to mozillabugs from comment #0)
> > I do not currently have a POC (how do you make this function run?)
> 
> From the directory, on windows with some kind of accessibility software or
> device running.

Directory? I do have accessibility software, and the accessibility icon is in FF's window, but I cannot seem to cause FF to call this function.

Comment 3

[Daniel Veditz [:dveditz]]

alexander: can you give Ron some tips on how to trigger this accessibility code?

Comment 4

[alexander :surkov (:asurkov)]

you could try to trigger the method in console:

let accService = Components.classes[
      '@mozilla.org/accessibilityService;1'].getService(
        Components.interfaces.nsIAccessibilityService);

then 
let acc = accService.getAccessibleFor(DOMNode);

and then
acc.getRelationByType(RELTYPE);

Comment 5

[mozillabugs]

(In reply to alexander :surkov from comment #4)
> you could try to trigger the method in console:
> 
> let accService = Components.classes[
>       '@mozilla.org/accessibilityService;1'].getService(
>         Components.interfaces.nsIAccessibilityService);
> 
> then 
> let acc = accService.getAccessibleFor(DOMNode);
> 
> and then
> acc.getRelationByType(RELTYPE);

Thanks. I'll let you know what happens.

Comment 6

[alexander :surkov (:asurkov)]

setting as P2 until we have more details on the problem.

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to mozillabugs from comment #0)

BTW, it also appears that lines 379, 391, and 398 are meant to read

return nsTArray<ProxyAccessible*>();

instead of

nsTArray<ProxyAccessible*>();

Alexander: did you look at that part of the original comment?

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 9

[James Teh [:Jamie]]

(In reply to mozillabugs from comment #2)

I do have accessibility software, and the accessibility icon is
in FF's window, but I cannot seem to cause FF to call this function.

Real accessibility clients on Windows communicate with the content process directly via COM. Thus, they don't use this code. This code is only used for Gecko XPCOM clients, which in practice means only browser tests.

Comment 10

[James Teh [:Jamie]]

Comment on attachment 9038205 [details] [diff] [review]
patch

Thanks!

Comment 11

[mozillabugs]

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

(In reply to mozillabugs from comment #2)

I do have accessibility software, and the accessibility icon is
in FF's window, but I cannot seem to cause FF to call this function.

Real accessibility clients on Windows communicate with the content process directly via COM. Thus, they don't use this code. This code is only used for Gecko XPCOM clients, which in practice means only browser tests.

Aha. Thank you for clearing up my confusion. Please consider also initializing |targets| in the patch, just for cleanliness.

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 9038205 [details] [diff] [review]
patch

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: requires some accessibility API and screen reader internals knowledge

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?: all

If not all supported branches, which bug introduced the flaw?: Bug 1336637

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?: low risk

Comment 13

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

Comment on attachment 9038205 [details] [diff] [review]
patch

As a sec-moderate, this doesn't need sec-approval to land on trunk so feel free to do so. It doesn't seem high risk in any case.

Comment 14

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8c53434337

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/1b8c53434337

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Not sure if we need this on ESR60 given the severity and where we are in the cycle, but please nominate this for Beta approval at least. Also ESR60 if you feel strongly about it.

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 9038205 [details] [diff] [review]
patch

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1336637

User impact if declined

sec-issue

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

Yes

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)

getting back missing 'return' statements, trivial change

String changes made/needed

no

Comment 18

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

Comment on attachment 9038205 [details] [diff] [review]
patch

Fix for sec-moderate issue in a11y code, verified in nightly.
Let's uplift for beta 5.

Comment 19

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

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