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 .

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

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.

You are right. This patch does a better check because in the end we need to read ZIPLOCAL_SIZE bytes. Feedback is welcome. Thanks!

I assume this is exposed to web content.

Please remember to get sec-approval+ before landing. :)

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.

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.

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

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.

I added a check for the extralen.

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.

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.

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.

Dan, please use the form "[checkin on" for the whiteboard tag so that various searches find it.

checkin-needed but read comment 17 before landing.

Let's set it once it's ready to land so it's not uselessly showing up in queries for a week.

https://hg.mozilla.org/releases/mozilla-esr38/rev/2fea7877193d https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2ea8394e08a2 https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/d6cbb10868b0 https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/d6cbb10868b0 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/3aa5d3f91ad5 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/3aa5d3f91ad5 This needs a bit of rebasing in nsZipArchive.cpp for esr31.

Patch for esr31 ready.

Thanks :) https://hg.mozilla.org/releases/mozilla-esr31/rev/ec83c431fcec

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

I don't think this can be tested manually.

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