Closed Bug 1167888 (CVE-2015-2736) Opened 9 years ago Closed 9 years ago

nsZipArchive::BuildFileList has memory-safety bug

Categories

(Core :: Networking: JAR, defect)

38 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox38 --- wontfix
firefox39 + fixed
firefox38.0.5 --- wontfix
firefox40 + fixed
firefox41 + fixed
firefox-esr31 39+ fixed
firefox-esr38 39+ fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: q1, Assigned: baku)

References

Details

(Keywords: csectype-intoverflow, regression, sec-high, Whiteboard: [adv-main39+][adv-esr38.1+][adv-esr31.8+])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows; rv:***) Gecko/20100101 Firefox/**.*
Build ID: 20150305021524

Steps to reproduce:

An attacker could force nsZipArchive::BuildFileList to read memory that it does not own, possibly leading to a crash, disclosure of sensitive information, or execution of code chosen by an attacker.

The problem is in the function's failure to validate the offsets of zip central directories.

Code beginning at line 624 extracts the first central directory offset from the zip-end record:

624:    for (buf = endp - ZIPEND_SIZE; buf > startp; buf--)
625:      {
626:        if (xtolong(buf) == ENDSIG) {
627:          centralOffset = xtolong(((ZipEnd *)buf)->offset_central_dir);
628:          break;
629:        }
630:      }
631:  }

Line 637 then sets the buffer pointer buf to "startp + centralOffset" without doing more than a cursory validation of centralOffset:

632:
633:  if (!centralOffset)
634:    return NS_ERROR_FILE_CORRUPTED;
635:
636:  //-- Read the central directory headers
637:  buf = startp + centralOffset;

The code then uses buf to find and read central directories:

638:  uint32_t sig = 0;
639:  while (buf + int32_t(sizeof(uint32_t)) <= endp &&
640:         (sig = xtolong(buf)) == CENTRALSIG) {
641:    // Make sure there is enough data available.
642:    if (endp - buf < ZIPCENTRAL_SIZE)
643:      return NS_ERROR_FILE_CORRUPTED;
644:
645:    // Read the fixed-size data.
646:    ZipCentral* central = (ZipCentral*)buf;
647:
648:    uint16_t namelen = xtoint(central->filename_len);
649:    ... // lots more where that came from

The problem occurs if the first-encountered zip-end record contains an offset_central_dir field that is very large, thus making centralOffset very large. In this case, the RHS of line 637 can overflow (especially on 32-bit platforms), setting buf to somewhere < startp. Since buf is then perforce < endp, this does not trip the check on line 639.

If *buf happens to contain CENTRALSIG, the code at line 638 et seq interprets it as a valid central directory, with unpredictable consequences further down the road when it's used. I guess that this defect could be used, for example, to inject Javascript from domain A into domain B.

This bug might explain https://bugzilla.mozilla.org/show_bug.cgi?id=596826 .
Attached patch zip2.patchSplinter Review
Attachment #8610176 - Flags: review?(bugs)
Component: Untriaged → Networking: JAR
Product: Firefox → Core
Comment on attachment 8610176 [details] [diff] [review]
zip2.patch

This needs to land very late in a cycle :/
Attachment #8610176 - Flags: review?(bugs) → review+
Comment on attachment 8610176 [details] [diff] [review]
zip2.patch

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

A malformed zip file can cause 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?

Yes.

Which older supported branches are affected by this flaw?

This code landed in 2009. All the branches are effected.

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

bug 511754

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

It's easy to create a backport patch. And it's not risky at all.

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

No regressions.
Attachment #8610176 - Flags: sec-approval?
Assignee: nobody → amarchesini
Comment on attachment 8610176 [details] [diff] [review]
zip2.patch

This patch (especially combined with the other similar one) draws attention to overflow math in this file so we need to fix the similar problems a few lines down while we're here. (namelen is sanity checked, but extralen and commentlen aren't, and addition of small values could overflow anyway under the right circumstances). One possible check would be to compare (endp - buf) to the sizes.
Attachment #8610176 - Flags: sec-approval?
Attachment #8610176 - Flags: sec-approval-
Attachment #8610176 - Flags: review-
I guess we can combine this patch with the other one. I asked a review for the other patch and in that patch I fix what you suggest here.
Flags: needinfo?(dveditz)
sounds good.
Flags: needinfo?(dveditz)
dveditz, when can we land these 2 patches?
Flags: needinfo?(dveditz)
Flags: needinfo?(dveditz)
Whiteboard: [wait until June 4 to land]
Comment on attachment 8610176 [details] [diff] [review]
zip2.patch

a=dveditz and sec-approval for all the branches, to land on June 4
Attachment #8610176 - Flags: sec-approval-
Attachment #8610176 - Flags: sec-approval+
Attachment #8610176 - Flags: review-
Attachment #8610176 - Flags: approval-mozilla-esr38+
Attachment #8610176 - Flags: approval-mozilla-esr31+
Attachment #8610176 - Flags: approval-mozilla-beta+
Attachment #8610176 - Flags: approval-mozilla-aurora+
Whiteboard: [wait until June 4 to land] → [checkin on 6/4]
checkin-needed but read comment 8 before landing.
Keywords: checkin-needed
Flags: sec-bounty?
https://hg.mozilla.org/releases/mozilla-aurora/rev/46d586760c74
Flags: in-testsuite?
Whiteboard: [checkin on 6/4]
Target Milestone: --- → mozilla41
Andrea, is manual verification needed for this fix? If yes, could you provide us with some testing details?
Flags: needinfo?(amarchesini)
I don't think this can be test manually.
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #16)
> I don't think this can be test manually.

Thank you Andrea! Setting as qe-verify- then.
Flags: qe-verify-
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main39+][adv-esr38.1+][adv-esr31.8+]
Alias: CVE-2015-2736
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.