1167888 - (CVE-2015-2736) nsZipArchive::BuildFileList has memory-safety bug

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Description

[q1]

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 .

Comment 1

[Andrea Marchesini [:baku]]

Attachment: zip2.patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8610176 [details] [diff] [review]
zip2.patch

This needs to land very late in a cycle :/

Comment 3

[Andrea Marchesini [:baku]]

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.

Comment 4

[Daniel Veditz [:dveditz]]

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.

Comment 5

[Andrea Marchesini [:baku]]

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.

Comment 6

[Daniel Veditz [:dveditz]]

sounds good.

Comment 7

[Andrea Marchesini [:baku]]

dveditz, when can we land these 2 patches?

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 8610176 [details] [diff] [review]
zip2.patch

a=dveditz and sec-approval for all the branches, to land on June 4

Comment 9

[Andrea Marchesini [:baku]]

checkin-needed but read comment 8 before landing.

Comment 10

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c29eb9667ed

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/46d586760c74

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/62bb5056f458

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/fcb018657fb6
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f2157a04d75b
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/25ae62aef2c1
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/25ae62aef2c1
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/5e363e84b661
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/5e363e84b661
https://hg.mozilla.org/releases/mozilla-esr31/rev/917d8e07d42b

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1c29eb9667ed

Comment 15

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Andrea, is manual verification needed for this fix? If yes, could you provide us with some testing details?

Comment 16

[Andrea Marchesini [:baku]]

I don't think this can be test manually.

Comment 17

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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