Closed Bug 1166900 (CVE-2015-2735) Opened 9 years ago Closed 9 years ago

Memory safety bug due to bad test in nsZipArchive.cpp


(Core :: Networking: JAR, defect)

38 Branch
Not set



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


(Reporter: q1, Assigned: baku)



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


(2 files, 3 obsolete files)

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;
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 . It might also be involved in .
Component: Untriaged → Networking: JAR
Product: Firefox → Core
We have similar implementation in ArchiveReader. When this bug is fixed, please update the ArchiveZipFile code as well.
Attached patch zip.patch (obsolete) — Splinter Review
Attachment #8608661 - Flags: review?(bugs)
Assignee: nobody → amarchesini
Attached patch zip.patch (obsolete) — Splinter Review
Attachment #8608661 - Attachment is obsolete: true
Attachment #8608661 - Flags: review?(bugs)
Attachment #8608664 - Flags: review?(bugs)
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.
Attached 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. :)
Ever confirmed: true
Comment on attachment 8608843 [details] [diff] [review]

[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]

If you fix this you need to fix the same basic overflow that happens just after in nsZipArchive::GetData()

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-
(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()
> nsZipArchive.cpp#l833

This is exactly what the patch does. Right?
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)
Attached 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]

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+
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]

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]
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?
Flags: in-testsuite?
Whiteboard: [checkin on 6/4]
Target Milestone: --- → mozilla41
Attached 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)
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
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.