Closed Bug 1366140 Opened 3 years ago Closed 3 years ago

[TSF] Needs to grab TSF related objects during calling methods of them

Categories

(Core :: Widget: Win32, defect, P2)

All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
thunderbird_esr45 --- wontfix
thunderbird_esr52 ? fixed
firefox-esr52 54+ fixed
firefox53 --- wontfix
firefox54 + fixed
firefox55 + fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: csectype-uaf, inputmethod, sec-high, Whiteboard: [post-critsmash-triage][adv-main54+][adv-esr52.2+] tpi:+)

Attachments

(2 files, 2 obsolete files)

There are a lot of odd crashes if you search with class name in TSF module:
https://crash-stats.mozilla.com/search/?signature=~TextInputClient&date=%3E%3D2016-11-19T02%3A43%3A03.000Z&date=%3C2017-05-19T02%3A43%3A03.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature
https://crash-stats.mozilla.com/search/?signature=~CDocumentInputManager&date=%3E%3D2016-11-19T02%3A43%3A03.000Z&date=%3C2017-05-19T02%3A43%3A03.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature
https://crash-stats.mozilla.com/search/?signature=~CThreadInputMgr&date=%3E%3D2016-11-19T02%3A43%3A03.000Z&date=%3C2017-05-19T02%3A43%3A03.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature
https://crash-stats.mozilla.com/search/?signature=~CThreadInputMgr&date=%3E%3D2016-11-19T02%3A43%3A03.000Z&date=%3C2017-05-19T02%3A43%3A03.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports
https://crash-stats.mozilla.com/search/?signature=~CThreadInputMgr&date=%3E%3D2016-11-19T02%3A43%3A03.000Z&date=%3C2017-05-19T02%3A43%3A03.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

Some of them may be bugs of IME or TSF framework. However, in our code, there are some points that we should/can guarantee to keep object lifetime.

On of the important point is, we don't hold ITfMessagePump instance in WinUtils::PeekMessage() nor WinUtils::GetMessage(). So, this means that our code can be used to attack with any message.
https://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/widget/windows/WinUtils.cpp#688,697,700,711,715,718

So, we need to fix this bug in any branches.
Coming patch touches WinUtils::(Peek|Get)Message() (adding RefPtr as local variable). But it doesn't affect the preformance:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=2c783a7b6d05&newProject=try&newRevision=40c402736dcd&framework=1&filter=win&showOnlyImportant=0
And just calling AddRef() and Release() instead of RefPtr doesn't optimize anything.
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=40c402736dcd&newProject=try&newRevision=56233b463eb5&framework=1&showOnlyImportant=0
So, the patch must be safe for the performance.

Note that the sample code in MSDN calls QueryInterface() and Release(). It must be slower than what we do.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms628783(v=vs.85).aspx
Track 54+/55+ as the volume of crashes is large.
Comment on attachment 8869341 [details] [diff] [review]
Try to avoid crash caused by COM objects which are owned by static or instance member of TSFTextStore

I'd like Makoto-san to review TSFTextStore part and Jim to review WinUtils part.
Attachment #8869341 - Flags: review?(m_kato)
Attachment #8869341 - Flags: review?(jmathies)
Group: core-security → layout-core-security
Comment on attachment 8869341 [details] [diff] [review]
Try to avoid crash caused by COM objects which are owned by static or instance member of TSFTextStore

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

::: widget/windows/TSFTextStore.cpp
@@ +1358,5 @@
> +  // Create document manager
> +  RefPtr<ITfThreadMgr> threadMgr = sThreadMgr;
> +  RefPtr<ITfDocumentMgr> documentMgr;
> +  HRESULT hr = threadMgr->CreateDocumentMgr(getter_AddRefs(documentMgr));
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
Attachment #8869341 - Flags: review?(m_kato) → review+
Oops, sorry for the spam.
Attachment #8869986 - Attachment is obsolete: true
Attachment #8869986 - Flags: review?(jmathies)
Attachment #8870014 - Flags: review?(jmathies)
Attachment #8870014 - Flags: review?(jmathies) → review+
This bug hasn't been moderate the severity rating, so, I guess I need to wait it? (In my experience, all security bugs which I CCed to are moderated first immediately after filing, but this isn't so.)

What can attackers do is depends on what TSF, IME and active keyboard layout does. While a method of COM object of TSF is called (this includes sending every message to TSF!!), we may release the COM objects if focus is moved.  Then, TSF, IME and active keyboard layout may access their members without grabbing themselves.  Then, it causes heap-use-after-free. So, this could be sec-high.

The patch cannot applied directly to Beta and ESR52 due to:
https://hg.mozilla.org/mozilla-central/rev/8d081bf9b839
>  // static
> -nsIMEUpdatePreference
> -TSFTextStore::GetIMEUpdatePreference()
> +IMENotificationRequests
> +TSFTextStore::GetIMENotificationRequests()

These lines are included in the tail of Hunk #6. So, (although I'll post a patch for Beta and ESR52,) you should be able to use hg graft after landing the patch to m-c.
Flags: needinfo?(dveditz)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #8)
> Then, it causes heap-use-after-free. So, this could be sec-high.

Agreed. Many of the crashes are out of bounds writes and executing wild addresses so it could be worse. Please request sec-approval on the patch and answer the generated questions.
Flags: needinfo?(dveditz)
Comment on attachment 8870014 [details] [diff] [review]
Try to avoid crash caused by COM objects which are owned by static or instance member of TSFTextStore

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not sure. Most crashes occur in TSF module of Windows. So, may need to guess or analyze what TSF tried does.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

This is calling COM methods whose objects are grabbed by only member variables, not local variables. The check-in comment explains that. (But the patch obviously indicates it, anyway.)

Which older supported branches are affected by this flaw?

ESR52.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I created the patch for Beta and ESR52 too. However, this patch is rejected only by the last 2 lines of Hunk #6. So, I guess that |hg graft| can be used.

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

This touched only the code initializing TSF objects when starting the main process and moving focus in web pages. So, this could cause regression around them if there were some simple mistake. I'm using patched build to test this before requesting the first review but I have not found any regressions.
Attachment #8870014 - Flags: sec-approval?
FYI: bug 1361132 might be fixed by the patch for this bug.
Priority: -- → P4
Whiteboard: tpi:+
Priority: P4 → P2
sec-approval+.
We'll want this on Beta and ESR52 as well so please nominate patches.
Attachment #8870014 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #13)
> sec-approval+.
> We'll want this on Beta and ESR52 as well so please nominate patches.

Should I land it to m-c first and then, should request approvals for Beta and ESR52?

Or should I request approvals and wait somebody to land them once?
Flags: needinfo?(abillings)
Push to inbound now. Request Beta/ESR52 approval in the mean time :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f32c73555c98ab565cb7ae10a761d6817aa5c89a
Bug 1366140 Try to avoid crash caused by COM objects which are owned by static or instance member of TSFTextStore r=m_kato, jimm
Comment on attachment 8870014 [details] [diff] [review]
Try to avoid crash caused by COM objects which are owned by static or instance member of TSFTextStore

[Approval Request Comment]
User impact if declined: 

COM objects are used when handling every message of Windows and initializing TextStore which is created when editable content gets focus. In this time, the object may be destroyed during a method-call. So, if TSF, IME or active keyboard layout's dll accesses destroyed instance members or methods, attackers might do a lot. (I.e., this is NOT limited to IME users.)

Fix Landed on Version:
55

Risk to taking this patch (and alternatives if risky):
Simple mistake could be there, but otherwise, this must be safe because making object life time guarantee during every method-call.

String or UUID changes made by this patch:
No.

Approval Request Comment
[Feature/Bug causing the regression]:
Regression when we enable TSF mode in default settings.

[User impact if declined]:
See above.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
No, just landed immediately before.

[Needs manual test from QE? If yes, steps to reproduce]:
No. This is not fixing specific issue.

[List of other uplifts needed for the feature/fix]:
No.

[Is the change risky?]:
No, see above.

[Why is the change risky/not risky?]:
See above.

[String changes made/needed]:
No.


Note that if you land this to Beta or ESR52, you'll see failure to merge. Then you should use attachment 8870232 [details] [diff] [review]. However, this should be available if you use |hg graft|.
Flags: needinfo?(abillings)
Attachment #8870014 - Flags: approval-mozilla-esr52?
Attachment #8870014 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/f32c73555c98
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8870014 [details] [diff] [review]
Try to avoid crash caused by COM objects which are owned by static or instance member of TSFTextStore

Sec-high, Beta54+, ESR52+
Attachment #8870014 - Flags: approval-mozilla-esr52?
Attachment #8870014 - Flags: approval-mozilla-esr52+
Attachment #8870014 - Flags: approval-mozilla-beta?
Attachment #8870014 - Flags: approval-mozilla-beta+
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from comment #12)
> FYI: bug 1361132 might be fixed by the patch for this bug.
this particular signature is still there in today's nightly build: bp-265d921f-35ac-42f8-8fa1-277b40170526
(In reply to [:philipp] from comment #20)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from
> comment #12)
> > FYI: bug 1361132 might be fixed by the patch for this bug.
> this particular signature is still there in today's nightly build:
> bp-265d921f-35ac-42f8-8fa1-277b40170526

Thanks! I'll keep investigating the crash reason if it can avoid by ourside.
Depends on: 1368318
Flags: qe-verify-
Whiteboard: tpi:+ → [post-critsmash-triage] tpi:+
Group: layout-core-security → core-security-release
Whiteboard: [post-critsmash-triage] tpi:+ → [post-critsmash-triage][adv-main54+][adv-esr52.2+] tpi:+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.