Closed Bug 1387937 Opened 7 years ago Closed 7 years ago

Suspicious use-after-free at intl/icu/source/i18n/zonemeta.cpp

Categories

(Core :: JavaScript: Internationalization API, defect, P3)

54 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: alexc, Assigned: m_kato)

References

Details

(Keywords: sec-other, Whiteboard: [adv-main57-][post-critsmash-triage])

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36

Steps to reproduce:

Hi,

Our code scanner has reported a use-after-free issue at  function `ZoneMeta::createMetazoneMappings(const UnicodeString &tzid)` of intl/icu/source/i18n/zonemeta.cpp,


At the following code piece and the implementation of `deleteOlsonToMetaMappingEntry`:

                if (mzMappings == NULL) {
                    mzMappings = new UVector(deleteOlsonToMetaMappingEntry, NULL, status);
                    if (U_FAILURE(status)) {
                        delete mzMappings;
                        deleteOlsonToMetaMappingEntry(entry);                 // <== this is `uprv_free(entry)`
                        uprv_free(entry);                                                 // `uprv_free(entry)` again ?
                        break;
                    }
                }


// implementation of `deleteOlsonToMetaMappingEntry`
/**
 * Deleter for OlsonToMetaMappingEntry
 */
static void U_CALLCONV
deleteOlsonToMetaMappingEntry(void *obj) {
    icu::OlsonToMetaMappingEntry *entry = (icu::OlsonToMetaMappingEntry*)obj;
    uprv_free(entry);
}


seems there is a duplication of uprv_free? Could any one have look and see if it is a true issue?

Regards,
SourceBrella Inc.
Component: Untriaged → JavaScript: Internationalization API
Product: Firefox → Core
This seems to be double free, and this is 3rd party library.  I mark as sec at first.
Group: core-security
Hi Kato,

for the link you gave me earlier, http://bugs.icu-project.org/trac/ticket/13301
seems they haven't open for registration, and unfortunately the url needs TICKET_VIEW access right which I don't have,

it the url referenced to this issue?
Group: core-security → javascript-core-security
(In reply to Alex CHEN from comment #2)
> Hi Kato,
> 
> for the link you gave me earlier,
> http://bugs.icu-project.org/trac/ticket/13301
> seems they haven't open for registration, and unfortunately the url needs
> TICKET_VIEW access right which I don't have,
> 
> it the url referenced to this issue?

I filed this as sensitive bug.  But it is no way to create account, so I cannot check this too :-<.
Since the two frees follow each other immediately it's hard to see how an attacker could take advantage of it. maybe something on another thread could be allocating things meanwhile, but that'd be a nightmarish timing needle to thread.
Keywords: sec-low
(In reply to Daniel Veditz [:dveditz] from comment #4)
> Since the two frees follow each other immediately it's hard to see how an
> attacker could take advantage of it. maybe something on another thread could
> be allocating things meanwhile, but that'd be a nightmarish timing needle to
> thread.

Does that mean this bug is confirmed but it has low security risk?
The newest version of libicu has fix this bug. It deletes the extra free statement: ``deleteOlsonToMetaMappingEntry(entry);".

Thus, this bug is confirmed and fixed in the trunk of libicu.
(In reply to Alex CHEN from comment #6)
> The newest version of libicu has fix this bug. It deletes the extra free
> statement: ``deleteOlsonToMetaMappingEntry(entry);".
> 
> Thus, this bug is confirmed and fixed in the trunk of libicu.

Thanks, this seems to be http://bugs.icu-project.org/trac/changeset/40324
Attached patch Part 2. Add patch file (obsolete) — Splinter Review
Attachment #8906504 - Attachment is obsolete: true
Is the correct action here for us to make the same change or upgrade our libicu or something else? Thanks
Flags: needinfo?(m_kato)
Priority: -- → P3
(In reply to Naveed Ihsanullah [:naveed] from comment #11)
> Is the correct action here for us to make the same change or upgrade our
> libicu or something else? Thanks

This fix will be ICU 60.  This isn't critical because this double-free occurs on low memory situation only.  But we should land this in m-c since ICU 60 will be next month.
Flags: needinfo?(m_kato)
Comment on attachment 8906503 [details] [diff] [review]
Part 1. Import changeset 40324

This is import patch from http://bugs.icu-project.org/trac/changeset/40324.  Although this is double-free issue, this isn't critical issue because new UVector has to return error status since malloc in UVector::_init is failure.

If we should want until ICU60 release, please set r-.
Attachment #8906503 - Flags: review?(jfkthame)
Attachment #8906505 - Flags: review?(jfkthame)
Comment on attachment 8906503 [details] [diff] [review]
Part 1. Import changeset 40324

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

Although the risk here looks very low in practice, given that it's already confirmed upstream and fixed for the next release, I think it's fine to go ahead and patch this for now.
Attachment #8906503 - Flags: review?(jfkthame) → review+
Attachment #8906505 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/mozilla-central/rev/a9bf7b6cf6d4
Assignee: nobody → m_kato
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main57+]
Alias: CVE-2017-7843
Alias: CVE-2017-7843
Keywords: sec-lowsec-other
Whiteboard: [adv-main57+]
Whiteboard: [adv-main57-]
Flags: qe-verify-
Whiteboard: [adv-main57-] → [adv-main57-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: