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

Status: VERIFIED FIXED

(Core :: Widget, defect)

Description

[Andrew McCreight [:mccr8]]

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

Comment 1

[Daniel Veditz [:dveditz]]

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.

Comment 2

[Makoto Kato [:m_kato]]

I think that this doesn't occur memory corruption.  Because before accessing array, we already make space by SetCapacity.  But we don't call SetLength at this point yet, so it will hit this assertion.

So I don't think that this isn't critical and may not be security issue.

But we should call SetLength to avoid this assertion before accessing array by index.

Comment 3

[Makoto Kato [:m_kato]]

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 4

[Makoto Kato [:m_kato]]

Attachment: Return error when IME attribute array doesn't have valid. r?masayuki

Comment 5

[Makoto Kato [:m_kato]]

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?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 7

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/facf5812faffa05e6037e96c92a5835d12937966

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/facf5812faff

Comment 10

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 50. Like Liz said, we can plan to uplift this for 50.1.0

Comment 12

[Makoto Kato [:m_kato]]

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

Comment 13

[Makoto Kato [:m_kato]]

Marco, HandleComposition cause is bug 1300143, not this.

Comment 14

[Makoto Kato [:m_kato]]

s/cause/case/

Comment 15

[Marco Castelluccio [:marco]]

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

Comment 16

[Daniel Veditz [:dveditz]]

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.

Comment 17

[Yanfang Liu]

Verified with the latest Fx51.0b1 (BuildID:20161115182233) on Windows XP + Simplified Chinese IME, this bug has gone.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

FIREFOX_50_0_1_RELBRANCH (50.0.1):
https://hg.mozilla.org/releases/mozilla-release/rev/9be70e2394de

default (50.1):
https://hg.mozilla.org/releases/mozilla-release/rev/4f4eabf6bdd0

Comment 20

[Yanfang Liu]

Verified on Fx50.0.1, fixed!

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx50 release notes as a known issue.