JAR/ZIP crash [@nsZipArchive::BuildSynthetics/HashName]

RESOLVED FIXED in Firefox 19

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: posidron, Assigned: jduell.mcbugs)

Tracking

(Blocks 1 bug, {crash, sec-critical, testcase})

Trunk
mozilla21
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, firefox18 wontfix, firefox19+ fixed, firefox20+ fixed, firefox21+ fixed, firefox-esr1719+ fixed, b2g1819+ fixed, b2g18-v1.0.0 fixed, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [adv-main19+][adv-esr1703+])

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

6 years ago
Posted file callstack
This crash happened while fuzzing the JAR/ZIP parser.

./modules/libjar/nsZipArchive.cpp:791

static uint32_t HashName(const char* aName, uint16_t len)
{
...
  while (p != endp) {
    val = val*37 + *p++;
  }
...

To reproduce load the testcase like:

jar:file:///Users/cdiehl/Desktop/testcase.jar!/

Tested with m-c changeset: 119051:ff2e30afa205
Reporter

Comment 1

6 years ago
Posted file testcase
Crashing on nonnull, need to determine if this is exploitable.
blocking-b2g: --- → tef+
Reporter

Updated

6 years ago
Blocks: 821596

Updated

6 years ago
Assignee: nobody → jduell.mcbugs
Assignee

Comment 3

6 years ago
3rd and following calls to HashName are passing what looks like an empty strings, but with long lengths (3-6K).  Probably generating bogus hashes for a while before one of the calls wanders off far enough to get a page fault.  Will look into this some more this weekend.

The code is generating a hash number with the memory touched.  Not sure if it's exploitable.  Probably as easy to fix as it is to figure that out :)

Comment 4

6 years ago
Posted patch fix v1.0 (obsolete) — Splinter Review
I don't know this code very well but I spent some time debugging and tracked down the problem. The problem is here:

nsZipArchive::BuildSynthetics

624       uint16_t namelen = item->nameLength;
625       const char *name = item->Name();
626       for (uint16_t dirlen = namelen - 1; dirlen > 0; dirlen--)
627       {
628         if (name[dirlen-1] != '/')
629           continue;

'namelen' is zero here, there is no check for that, and 'dirlen' is 'namelen - 1'. My patch checks for zero to avoid the bad subtraction and things seem to work fine with it.
Attachment #704699 - Flags: review?(taras.mozilla)

Updated

6 years ago
Assignee: jduell.mcbugs → joshmoz

Comment 5

6 years ago
Jason already came up with this exact same patch in bug 832160! Somehow I missed that.
Comment on attachment 704699 [details] [diff] [review]
fix v1.0

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

::: modules/libjar/nsZipArchive.cpp
@@ +622,5 @@
>        //-- start just before the last char so as to not add the item
>        //-- twice if it's a directory
>        uint16_t namelen = item->nameLength;
> +      if (namelen == 0)
> +        continue;

is a ZIP file with an entry with a zero-length name even legal? I doubt it.

I suggest we return en error in this case that there is an empty with a zero-length name.

Also, I think this check for a zero-length name should happen in BuildFileList, not here, because BuildFileList is the code that is actually processing the untrusted input. Notice this existing check:

    // Sanity check variable sizes and refuse to deal with
    // anything too big: it's likely a corrupt archive.
    if (namelen > kMaxNameLength || buf >= endp)
      return NS_ERROR_FILE_CORRUPTED;

should be:

    if (namelen < 1 || 
        namelen > kMaxNameLength ||
        buf >= endp) {
      return NS_ERROR_FILE_CORRUPTED;
    }

Then the check that was added here can become a MOZ_ASSERT.
Attachment #704699 - Flags: feedback-
Additionally, we should also be checking that we are correctly handling the case of paths like "foo////" or "//foo" or "foo//bar". That is, we must be sure to reject empty path components, to avoid problems with any code we have that is expecting non-empty path component names.

Comment 9

6 years ago
(In reply to Brian Smith (:bsmith) from comment #6)

> is a ZIP file with an entry with a zero-length name even legal? I doubt it.

I have no idea if it's legal but I'm fine with the strategy you're proposing.

Comment 10

6 years ago
(In reply to Josh Aas (Mozilla Corporation) from comment #9)
> (In reply to Brian Smith (:bsmith) from comment #6)
> 
> > is a ZIP file with an entry with a zero-length name even legal? I doubt it.
> 
> I have no idea if it's legal but I'm fine with the strategy you're proposing.

Of course, your code is failing on a zero-length name entry, instead of just skipping it like my first patch did. I don't know enough to care either way, just pointing that out.

Comment 11

6 years ago
Posted patch fix v1.1Splinter Review
Attachment #704699 - Attachment is obsolete: true
Attachment #704699 - Flags: review?(taras.mozilla)
Attachment #704895 - Flags: review?(taras.mozilla)

Updated

6 years ago
Attachment #704895 - Flags: review?(taras.mozilla) → review+
Comment on attachment 704895 [details] [diff] [review]
fix v1.1

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

::: modules/libjar/nsZipArchive.cpp
@@ +634,5 @@
>  
> +        // The character before this is '/', so if this is also '/' then we
> +        // have an empty path component. Skip it.
> +        if (name[dirlen] == '/')
> +          continue;

Somewhat of a nit:

The check for empty path components should be done before we construct the item. If we make sure that every item we construct is valid, we don't have to worry about protecting against invalid items everywhere else in the code.

(Put another way, the empty path component check is actually a generalization of the empty name check, and it should go in the same place, ideally in the constructor-ish function we use to create items.)
Attachment #704895 - Flags: review?(taras.mozilla)
Attachment #704895 - Flags: review+
Attachment #704895 - Flags: feedback-

Updated

6 years ago
Assignee: joshmoz → jduell.mcbugs
Assignee

Comment 13

6 years ago
I agree with Brian's point that optimally we'd validate entries in mFiles rather than sanity-check them at each use.  But to implement that I'd need some list of rules about what kinds of entry names are illegal (are the foo//bar examples with extra slashes in comment 8 illegal?  Would we look for null or other illegal chars in the name?)

I'd also be fine with taking josh's patch as-is.  It's up to taras, of course.
(In reply to Jason Duell (:jduell) from comment #13)
> I agree with Brian's point that optimally we'd validate entries in mFiles
> rather than sanity-check them at each use.  But to implement that I'd need
> some list of rules about what kinds of entry names are illegal (are the
> foo//bar examples with extra slashes in comment 8 illegal?  Would we look
> for null or other illegal chars in the name?)

For the purposes of this bug, the important thing is that there are no empty components. Also, see http://www.pkware.com/documents/casestudies/APPNOTE.TXT:

4.4.17.1 The name of the file, with optional relative path.
       The path stored MUST not contain a drive or
       device letter, or a leading slash.  All slashes
       MUST be forward slashes '/' as opposed to
       backwards slashes '\' for compatibility with Amiga
       and UNIX file systems etc.  If input came from standard
       input, there is no file name field.

AFAICT, we can safely ignore the Unicode file name extension.

I suggest we implement the rules in 4.4.17.1, plus the restriction that there are no empty paths (and thus no support for the "standard input" case), plus the restriction that there are no '\0'. And we can reserve anything beyond that for another bug.
Blocks: 832164
Marking all the branches on the assumption we haven't been messing with this code lately and that we'll need this patch on all supported branches.
Blocks: 832164
Comment on attachment 704895 [details] [diff] [review]
fix v1.1

nit:
if (namelen < 1 ||
I prefer !namelen

I think doing "//" check in BuildSynthetics is fine. Full name validation feels out of scope for this bug.
Attachment #704895 - Flags: review?(taras.mozilla) → review+
Please nominate for uplift to Aurora/Beta no later than Monday. Please also prepare patches for ESR17/B2G.
Assignee

Updated

6 years ago
Duplicate of this bug: 832164
Assignee

Updated

6 years ago
Blocks: 834540
Assignee

Comment 19

6 years ago
Comment on attachment 704895 [details] [diff] [review]
fix v1.1

> Full name validation feels out of scope for this bug.

OK, I'd already written it--moved that to bug 834540.

Landed in inbound:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/17306ccdd8d5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): very old.
User impact if declined: bad crashes
Testing completed: yes. 
Risk to taking this patch (and alternatives if risky):  low.
String or UUID changes made by this patch: none

> Please nominate for uplift to Aurora/Beta no later than Monday

Then I'm going to assume I should skip the normal sec-approval process.  But here's the form I'd already filled out.

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

Attack requires constructing a bad JAR and getting B2G to load it--probably not that hard for app developer to put up a packaged app and cause crash.  Not sure how easy to exploit that crash.

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

Which older supported branches are affected by this flaw?  all of them

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Done

> How likely is this patch to cause regressions; how much testing does it need?

Not likely--only tests for illegal cases.  Existing mochi/unit tests seem like enough coverage.
Attachment #704895 - Flags: feedback-
Attachment #704895 - Flags: approval-mozilla-esr17?
Attachment #704895 - Flags: approval-mozilla-beta?
Attachment #704895 - Flags: approval-mozilla-b2g18?
Attachment #704895 - Flags: approval-mozilla-aurora?
Attachment #704895 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Attachment #704895 - Flags: approval-mozilla-esr17?
Attachment #704895 - Flags: approval-mozilla-esr17+
Attachment #704895 - Flags: approval-mozilla-beta?
Attachment #704895 - Flags: approval-mozilla-beta+
Attachment #704895 - Flags: approval-mozilla-aurora?
Attachment #704895 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/17306ccdd8d5
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Whiteboard: [adv-main19+][adv-esr1703+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.