Closed Bug 1215319 Opened 4 years ago Closed 4 years ago

Overflow causes out-of-bounds read attempts in nsZipArchive::BuildFileList

Categories

(Core :: Networking: JAR, defect)

41 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: q1, Assigned: mcmanus)

Details

(Keywords: sec-low, Whiteboard: [necko-active][adv-main49-])

Attachments

(2 files)

nsZipArchive::BuildFileList (modules\libjar\nsZipArchive.cpp) can incur an integer overflow, which then causes the function to attempt to read memory to which it has no right. The addresses involved are in the range 0xfffffffc to somewhere in the lower part of the first page of memory (wraparound!) on 32 bit builds. The bug can't affect 64 bit builds because invoking it would require the use of non-canonical addresses, which AFAIK causes a fault on all x64 processors.

Because the affected addresses are likely always to be inaccessible, this bug should have little practical effect, but it should be fixed.

Details
-------

The bug is in lines 655-56:

613: nsresult nsZipArchive::BuildFileList(PRFileDesc *aFd)
614: {
615:   // Get archive size using end pos
616:   const uint8_t* buf;
617:   const uint8_t* startp = mFd->mFileData;
618:   const uint8_t* endp = startp + mFd->mLen;
619: MOZ_WIN_MEM_TRY_BEGIN
620:   uint32_t centralOffset = 4;
.., [extract |centralOffset| from archive]
643: 
644:   if (!centralOffset)
645:     return NS_ERROR_FILE_CORRUPTED;
646: 

If |centralOffset| is such that |buf| becomes, e.g., 0xfffffffc on a 32-bit architecture:

647:   buf = startp + centralOffset;
648: 

control skips from line 650 to line 654:

649:   // avoid overflow of startp + centralOffset.
650:   if (buf < startp)
651:     return NS_ERROR_FILE_CORRUPTED;
652: 
653:   //-- Read the central directory headers

and the "buf +..." in line 655 computes the value 0xfffffffc + 4 == 0, which is <= |endp| (BTW, what's with the cast?):

654:   uint32_t sig = 0;
655:   while (buf + int32_t(sizeof(uint32_t)) <= endp &&

so line 656 executes, attempting to read the bytes at addresses 0xffffffffc-0xffffffff. If those bytes happen to be valid and to contain |CENTRALSIG| [1]:

656:          (sig = xtolong(buf)) == CENTRALSIG) {

then control goes to line 658. The expression |endp - 0xfffffffc| is >= |ZIPCENTRAL_SIZE| for any practical |buf|:

657:     // Make sure there is enough data available.
658:     if (endp - buf < ZIPCENTRAL_SIZE)
659:       return NS_ERROR_FILE_CORRUPTED;
660: 

So control then goes to line 662 et seq, which attempt to access fields of ZipCentral whose addresses wrap around to the beginning of the address space:

661:     // Read the fixed-size data.
662:     ZipCentral* central = (ZipCentral*)buf;
663: 
664:     uint16_t namelen = xtoint(central->filename_len);
665:     uint16_t extralen = xtoint(central->extrafield_len);
666:     uint16_t commentlen = xtoint(central->commentfield_len);
667:     uint32_t diff = ZIPCENTRAL_SIZE + namelen + extralen + commentlen;

On most architectures one or more of these reads incurs an access violation, which then triggers an exception, which is handled by line 713:

713: MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE)

A largely-similar sequence of events unfolds if the bytes tested by line 656 contain |ENDSIG|.

[1] This does not happen on WOW64 on Win7 SP1, which appears to reserve the last 128k of the 32 bit address space. It might occur on more-recent versions of Windows, or on Unixes, but I haven't tested it. Make sure to use the /LARGEADDRESSAWARE linker flag (as FF does) when testing this issue.
Keywords: sec-low
Group: core-security → network-core-security
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
We no longer support jar: URLs for remote web content (one direct way to get victims to exercise this code) but we still do support XPInstall and packaged web apps. Even if those have invalid signatures and wouldn't actually install we would have to open the zip archive (exercising this code) to find that out
Assignee: nobody → mcmanus
Whiteboard: [necko-active]
Attachment #8751792 - Flags: review?(dd.mozilla)
q1 if you would like to comment I'd be happy to hear if I understood your analysis correctly. Thanks for the detailed description.
(In reply to Patrick McManus [:mcmanus] from comment #3)
> q1 if you would like to comment I'd be happy to hear if I understood your
> analysis correctly. Thanks for the detailed description.

I will examine your code by Monday, 5/16/2016. Thanks for fixing this bug.
Attachment #8751792 - Flags: review?(dd.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/7f82f2a2113f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
I apologize for not examining this bug when I said I would. I do have a comment:

655:   while (buf + int32_t(sizeof(uint32_t)) <= endp &&
656:          (sig = xtolong(buf)) == CENTRALSIG) {

still has a bug. Per comment 0, "[i]f |centralOffset| is such that |buf| becomes, e.g., 0xfffffffc on a 32-bit architecture", then line 656 attempts to read from 0xfffffffc-0xffffffff. Presumably this in a read-protected system address -- or an invalid address -- on most OSes, so this read should, at most, cause an access violation and subsequent crash, but it's still ugly.

The fix here:

658:    if ((buf > endp) || (endp - buf < ZIPCENTRAL_SIZE)) {

does seem to fix the main bug that comment 0 highlighted.
Group: network-core-security → core-security-release
Thanks. I misinterepreted that bit to apply to the other fragment I included in the patch. I'll add a follow on, continuing to use the odd cast of a sizeof(32bits) notation cause I'm afraid to just change it to 4 not knowing why it was done that way.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8755581 - Flags: review?(dd.mozilla)
Attachment #8755581 - Flags: review?(dd.mozilla) → review+
(In reply to Patrick McManus [:mcmanus] from comment #8)
> Thanks. I misinterepreted that bit to apply to the other fragment I included
> in the patch. I'll add a follow on, continuing to use the odd cast of a
> sizeof(32bits) notation cause I'm afraid to just change it to 4 not knowing
> why it was done that way.

Your comment 9 fix looks good.
https://hg.mozilla.org/mozilla-central/rev/a73b559073f5
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Flags: qe-verify-
Whiteboard: [necko-active] → [necko-active][adv-main49-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.