Bug 1166900 (CVE-2015-2735)

Memory safety bug due to bad test in nsZipArchive.cpp

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: q1, Assigned: baku)

Tracking

({csectype-bounds, sec-high})

38 Branch
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox38 wontfix, firefox39+ fixed, firefox38.0.5 wontfix, firefox40+ fixed, firefox41+ fixed, firefox-esr3139+ fixed, firefox-esr3839+ fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main39+][adv-esr38.1+][adv-esr31.8+])

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

4 years ago
User Agent: Mozilla/5.0 (Windows; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20150305021524

Steps to reproduce:

(Firefox 38.0.1)

nsZipArchive::GetDataOffset does not validate the archive's local header correctly. It uses the code:

790:  uint32_t len = mFd->mLen;
791:  const uint8_t* data = mFd->mFileData;
792:  uint32_t offset = aItem->LocalOffset();
793:  if (offset + ZIPLOCAL_SIZE > len)
794:    return 0;
795:
796:  // -- check signature before using the structure, in case the zip file is corrupt
797:  ZipLocal* Local = (ZipLocal*)(data + offset);
798:  if ((xtolong(Local->signature) != LOCALSIG))
799:    return 0;

The problem is with the line 793. If offset (which comes directly from the archive field central->localhdr_offset) is >= 0xffffffe2, the quantity offset + ZIPLOCAL_SIZE will overflow a uint32_t, resulting in a value <= len. Line 797 will then compute data+offset, which will point somewhere arbitrary in memory. In almost every case, line 798 will then either cause a read-access violation or a failed comparison to LOCALSIG, which presumably means that nothing will use the corrupt item. It is, however, faintly possible that data+offset points to valid memory that contains LOCALSIG, which would lead to unpredictable behavior somewhere down the road. Hence, I have marked this as a security bug.

This bug seems likely to be the cause of https://bugzilla.mozilla.org/show_bug.cgi?id=1164141 . It might also be involved in https://bugzilla.mozilla.org/show_bug.cgi?id=596826 .

Updated

4 years ago
Component: Untriaged → Networking: JAR
Product: Firefox → Core
Assignee

Comment 1

4 years ago
We have similar implementation in ArchiveReader. When this bug is fixed, please update the ArchiveZipFile code as well.
Assignee

Comment 2

4 years ago
Posted patch zip.patch (obsolete) — Splinter Review
Attachment #8608661 - Flags: review?(bugs)
Assignee

Updated

4 years ago
Assignee: nobody → amarchesini
Assignee

Comment 3

4 years ago
Posted patch zip.patch (obsolete) — Splinter Review
Attachment #8608661 - Attachment is obsolete: true
Attachment #8608661 - Flags: review?(bugs)
Attachment #8608664 - Flags: review?(bugs)
Reporter

Comment 4

4 years ago
The proposed fix creates a different but related vulnerability: an underflow. Consider the change to nsZipArchive::GetDataOffset:

  if (offset > len - ZIPLOCAL_SIZE)
    return 0;

If len < ZIPLOCAL_SIZE, the quantity len - ZIPLOCAL_SIZE will underflow, creating a huge unsigned value. Unless len is properly checked somewhere else, this problem renders this test ineffective.

I suggest something along these lines:

  if ((offset > len) || (offset + ZIPLOCAL_SIZE > len))
  {
    return 0;
  }

Since len is limited to INT32_MAX (0x7fffffff) by nsZipHandle::Init (modules\libjar\nsZipArchive.cpp line 187), a value of offset > 0x7fffffff is guaranteed to trigger the first clause. This makes it impossible for the second clause to overflow, no matter what value offset has.
Assignee

Comment 5

4 years ago
Posted patch zip.patch (obsolete) — Splinter Review
You are right. This patch does a better check because in the end we need to read ZIPLOCAL_SIZE bytes. Feedback is welcome. Thanks!
Attachment #8608664 - Attachment is obsolete: true
Attachment #8608664 - Flags: review?(bugs)
Attachment #8608843 - Flags: review?(bugs)
I assume this is exposed to web content.
Attachment #8608843 - Flags: review?(bugs) → review+
Please remember to get sec-approval+ before landing. :)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Comment 8

4 years ago
Comment on attachment 8608843 [details] [diff] [review]
zip.patch

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

I suspect easy enough in thunderbird. And maybe firefox too. Just a simple malformed zip file can make this code crash.

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

Yes. But we don't have a test. I'll write a check-in comment to describe the problem before landing this patch.

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 #8608843 - Flags: sec-approval?
Comment on attachment 8608843 [details] [diff] [review]
zip.patch

If you fix this you need to fix the same basic overflow that happens just after in nsZipArchive::GetData()
http://hg.mozilla.org/mozilla-central/annotate/86203ac87a08/modules/libjar/nsZipArchive.cpp#l833

and the equivalent spot in ArchiveZipReader::Init() (though hopefully the Seek() will simply fail and save us from any bad stuff).

This patch may inspire people to look for the same flaw nearby, and these were pretty obvious. There might be others in there.

Also XHR believes the return value of nsZipArchive::GetDataOffset() without any sanity checking. Even with this patch all we know for sure is that the local zip structure was inside the valid buffer, but the resulting offset (with a fake local filename and/or "extra" len) could be completely bogus. It does look like that guts of AllocateMappedContent() does do sanity checking though, so in practice this is OK.
Attachment #8608843 - Flags: sec-approval? → sec-approval-
Assignee

Comment 10

4 years ago
(In reply to Daniel Veditz [:dveditz] from comment #9)
> Comment on attachment 8608843 [details] [diff] [review]
> zip.patch
> 
> If you fix this you need to fix the same basic overflow that happens just
> after in nsZipArchive::GetData()
> http://hg.mozilla.org/mozilla-central/annotate/86203ac87a08/modules/libjar/
> nsZipArchive.cpp#l833

This is exactly what the patch does. Right?
Assignee

Updated

4 years ago
Flags: needinfo?(dveditz)
You're right, I don't know what I was thinking. Maybe I transferred my angst over the addition on line 815 to the wrong spot. The filename and extralen values are only 16-bits so would be harder to create an overflow, but not impossible.
Flags: needinfo?(dveditz)
Assignee

Comment 12

4 years ago
Posted patch zip.patchSplinter Review
I added a check for the extralen.
Attachment #8608843 - Attachment is obsolete: true
Attachment #8610409 - Flags: review?(dveditz)
Comment on attachment 8610409 [details] [diff] [review]
zip.patch

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

Thanks! r=dveditz

We should probably wait a week or two to land this, just so there's less time between check-in and release for this pretty obvious patch.
Attachment #8610409 - Flags: review?(dveditz) → review+
Assignee

Comment 14

4 years ago
dveditz, your r+ counts as sec-approval+ ? :)
no, I think we should wait a little to land this. Apparently b2g is going to branch on or around June 8 so it's probably best to land before then, but if we could wait until June 4 that would be great.
Whiteboard: [wait until June 4 to land]
Comment on attachment 8610409 [details] [diff] [review]
zip.patch

a=dveditz and sec-approval for all the branches, but please wait until June 4 to land.
Attachment #8610409 - Flags: sec-approval+
Attachment #8610409 - Flags: approval-mozilla-esr38+
Attachment #8610409 - Flags: approval-mozilla-esr31+
Attachment #8610409 - Flags: approval-mozilla-beta+
Attachment #8610409 - Flags: approval-mozilla-aurora+
Dan, please use the form "[checkin on" for the whiteboard tag so that various searches find it.
Whiteboard: [wait until June 4 to land] → [checkin on 6/4]
Assignee

Comment 18

4 years ago
checkin-needed but read comment 17 before landing.
Keywords: checkin-needed
Let's set it once it's ready to land so it's not uselessly showing up in queries for a week.
Keywords: checkin-needed
Flags: sec-bounty?
https://hg.mozilla.org/releases/mozilla-aurora/rev/f1515be9e882
Flags: in-testsuite?
Whiteboard: [checkin on 6/4]
Target Milestone: --- → mozilla41
https://hg.mozilla.org/mozilla-central/rev/a4549e040626
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Assignee

Comment 25

4 years ago
Posted patch esr31Splinter Review
Patch for esr31 ready.
Attachment #8610409 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8615837 - Attachment is patch: true
Attachment #8610409 - Attachment is obsolete: false
Andrea, is manual verification needed for this fix? If yes, could you provide us with some testing details?
Flags: needinfo?(amarchesini)
Assignee

Comment 28

4 years ago
I don't think this can be tested manually.
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #28)
> I don't think this can be tested manually.

Thank you Andrea! Setting as qe-verify- then.
Flags: qe-verify-
Flags: sec-bounty? → sec-bounty+
Blocks: 511754
Whiteboard: [adv-main39+][adv-esr38.1+][adv-esr31.8+]
Alias: CVE-2015-2735

Updated

4 years ago
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.