Closed Bug 1300140 Opened 4 years ago Closed 4 years ago

Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | mozilla::widget::IMMHandler::DispatchCompositionChangeEvent

Categories

(Core :: Widget, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
relnote-firefox --- 50+
firefox50 + verified
firefox51 + verified

People

(Reporter: mccr8, Assigned: m_kato)

References

Details

(Keywords: crash, csectype-bounds, Whiteboard: [sg:dos])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-36b47258-a46f-4f20-9788-64ab02160902.
=============================================================

I'm not sure what is going on here, but it could be an off-by-one-error. The reports I looked at all had ElementAt(aIndex = 1, aLength = 1).
Group: core-security → layout-core-security
Calling this sec-moderate on the assumption that these events are based on what the user types and not something a malicious page could trigger directly.
Sorry for previous comment.  That has invalid.

When using mAttributeArray, we allocate 65 length by SetCapacity at least.  So even if index = 1 on length = 1, it doesn't occur memory corruption / crash.  Also, since this is into IMM code, it will be on Windows XP only.

When this occurs, mClauseArray[n] has out of index value of mAttributeArray And its value isn't last of mClauseArray.  At least, user need input 65 length character to crash by this issue, and ImmGetCompositionStringW(..., GCS_COMPATTR) has to return unexpected data that you doesn't want.
Comment on attachment 8789749 [details] [diff] [review]
Return error when IME attribute array doesn't have valid. r?masayuki

If the value of non last item of mClauseArray is string length, this issue may occur.  Although I think that this may be IME bug, we should check this situation.

Nakano-san, I think that returning error may not be good because we already add attributes for all string at this time.  should I set eCaret attribute on this situation instead of returning error?
Attachment #8789749 - Flags: review?(masayuki)
Assignee: nobody → m_kato
Comment on attachment 8789749 [details] [diff] [review]
Return error when IME attribute array doesn't have valid. r?masayuki

I'd like you to use NS_WARN_IF() in the condition. Otherwise, looks good to me.
Attachment #8789749 - Flags: review?(masayuki) → review+
https://hg.mozilla.org/mozilla-central/rev/facf5812faff
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1304599
Group: layout-core-security → core-security-release
Looks like a top 20 crash in the first day of 50 release. Since this is sec-moderate we can probably wait to fix it in our minor release, 50.1.0, in December.  Can you request uplift to mozilla-release? Thanks!
Flags: needinfo?(m_kato)
Tracked for 50. Like Liz said, we can plan to uplift this for 50.1.0
Comment on attachment 8789749 [details] [diff] [review]
Return error when IME attribute array doesn't have valid. r?masayuki

Approval Request Comment
[Feature/regressing bug #]:
Bug 1159244

[User impact if declined]:
When using Windows XP + 3rd party Chinese IME, Firefox crashes by inputting IME text.

[Describe test coverage new/current, TreeHerder]:
Landed in m-c (Firefox 51).  No report from 51.

[Risks and why]: 
Too low.  Originally, this isn't memory corruption because we already allocate enough memory by nsTArray.SetCapacity().  But we forget setting length of array, so array length is unknown.  So since length is unknown (it means 0), it causes crash. 

[String/UUID change made/needed]:
None
Flags: needinfo?(m_kato)
Attachment #8789749 - Flags: approval-mozilla-release?
Crash Signature: [@ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | mozilla::widget::IMMHandler::DispatchCompositionChangeEvent] → [@ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | mozilla::widget::IMMHandler::DispatchCompositionChangeEvent] [@ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | mozilla::widget::IMMHandler::HandleComposition]
Marco, HandleComposition cause is bug 1300143, not this.
Crash Signature: [@ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | mozilla::widget::IMMHandler::DispatchCompositionChangeEvent] [@ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | mozilla::widget::IMMHandler::HandleComposition] → [@ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | mozilla::widget::IMMHandler::DispatchCompositionChangeEvent]
s/cause/case/
(In reply to Makoto Kato [:m_kato] from comment #13)
> Marco, HandleComposition case is bug 1300143, not this.

Yes, I meant to add it to that bug (the signature was in bug 1304601, which was duplicated to bug 1300143).
These intentional crashes don't look exploitable. The rating was considering the severity in the unfixed Fx49 (and maybe ESR-45!) which presumably had the bug but not the self-crash protection.
Verified with the latest Fx51.0b1 (BuildID:20161115182233) on Windows XP + Simplified Chinese IME, this bug has gone.
Status: RESOLVED → VERIFIED
Comment on attachment 8789749 [details] [diff] [review]
Return error when IME attribute array doesn't have valid. r?masayuki

This is a 50.0.1 dot release driver. Let's land this on m-r relbranch.
Attachment #8789749 - Flags: approval-mozilla-release? → approval-mozilla-release+
Verified on Fx50.0.1, fixed!
Added to Fx50 release notes as a known issue.
Group: core-security-release
Keywords: sec-moderate
Whiteboard: [sg:dos]
You need to log in before you can comment on or make changes to this bug.