1269246 - Possible use-after-free in nsCollationMacUC::AllocateRawSortKey

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Makoto Kato [:m_kato]]

https://dxr.mozilla.org/mozilla-central/rev/1461a4071341c282afcf7b72e33036412d2251d4/intl/locale/mac/nsCollationMacUC.cpp#178

NS_IMETHODIMP nsCollationMacUC::AllocateRawSortKey(int32_t strength, const nsAString& stringIn,
                                                   uint8_t** key, uint32_t* outLen)
...
  const UChar* str = (const UChar*)PromiseFlatString(stringIn).get();

  int32_t keyLength = ucol_getSortKey(mCollatorICU, str, stringInLen, nullptr, 0);

str is already free when using it at ucol_getSortKey if stringIn isn't null-terminated string.

Comment 1

[Makoto Kato [:m_kato]]

I believe that this isn't critical because AllocateRawSortKey isn't scriptable and this method is used on xpath only (xpath uses nsString that is null-terminated)

Comment 2

[Makoto Kato [:m_kato]]

1045958

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Remove unnecessary PromiseFlatString. r?hsivonen

Unnecessary to use PromiseFlatString.  ICU don't require null terminated string that has length parameter.

Comment 4

[Andrew McCreight [:mccr8]]

It sounds like this is not a danger right now, so I'm going to mark it sec-other.

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8747592 [details] [diff] [review]
Remove unnecessary PromiseFlatString. r?hsivonen

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

This is possible use-after-free.  But this interface doesn't export to extensions and web, so I think that attacker cannot create exploit code via this issue.

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

This code causes possible use-after-free.
const UChar* str = (const UChar*)PromiseFlatString(stringIn).get();

It means
{
  PromiseFlatString nullstr(stringIn);
  const UChar* str = (const UChar*)nullstr.get();
}
...

If stringIn isn't null terminated nsAString, it causes use-after-free.  But This method uses from XPath only now, then it uses nsString that is null-terminated.  So use-after-free won't occur on real situation.


Which older supported branches are affected by this flaw?
This code is from bug 1045958.  So all branches has this bug.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yet.  But we can port this to branches by same patch.

How likely is this patch to cause regressions; how much testing does it need?
Too low.  This code is used for XPath only.  xpath's test code covers this.

Comment 6

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

This is a sec-other, which doesn't need sec-approval to go in. Is there a specific reason you're asking for sec-approval?

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 7

[Makoto Kato [:m_kato]]

(In reply to Al Billings [:abillings] from comment #6)
> This is a sec-other, which doesn't need sec-approval to go in. Is there a
> specific reason you're asking for sec-approval?
> 
> https://wiki.mozilla.org/Security/Bug_Approval_Process

Ah, OK,  This is no reason for sec-approval.

Comment 8

[Makoto Kato [:m_kato]]

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

Comment 9

[Carsten Book [:Tomcat]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1269246