Closed Bug 1448505 Opened 3 years ago Closed 2 years ago

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

Categories

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

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed
firefox67 --- fixed

People

(Reporter: mozillabugs, Assigned: surkov)

References

Details

(Keywords: regression, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main66+])

Attachments

(1 file)

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: }
Summary: Probable free of uninitialized memory in ProxyAccessible::RelationByType() → Probable free of uninitialized pointer in ProxyAccessible::RelationByType()
Flags: sec-bounty?
Group: core-security → layout-core-security
(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.
Keywords: sec-moderate
(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.
alexander: can you give Ron some tips on how to trigger this accessibility code?
Flags: needinfo?(surkov.alexander)
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);
Flags: needinfo?(surkov.alexander)
(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.
Flags: needinfo?(mozillabugs)
setting as P2 until we have more details on the problem.
Priority: -- → P2

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

Flags: needinfo?(surkov.alexander)
Flags: needinfo?(surkov.alexander)
Attached patch patchSplinter Review
Assignee: nobody → surkov.alexander
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #9038205 - Flags: review?(jteh)

(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 on attachment 9038205 [details] [diff] [review]
patch

Thanks!
Attachment #9038205 - Flags: review?(jteh) → review+

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

Flags: needinfo?(mozillabugs)
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
Attachment #9038205 - Flags: sec-approval?
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.
Attachment #9038205 - Flags: sec-approval?
Group: layout-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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.

Blocks: 1336637
Flags: needinfo?(surkov.alexander)

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

Flags: needinfo?(surkov.alexander)
Attachment #9038205 - Flags: approval-mozilla-beta?
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.
Attachment #9038205 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: sec-bounty? → sec-bounty+
Keywords: regression
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main66+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.