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

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: 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

Comment 1

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Crashing on nonnull, need to determine if this is exploitable.

Comment 3

[Jason Duell]

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

[Josh Aas]

Attachment: fix v1.0

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.

Comment 5

[Josh Aas]

Jason already came up with this exact same patch in bug 832160! Somehow I missed that.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

[Josh Aas]

(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

[Josh Aas]

(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

[Josh Aas]

Attachment: fix v1.1

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 13

[Jason Duell]

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.

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 15

[Daniel Veditz [:dveditz]]

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.

Comment 16

[(dormant account)]

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.

Comment 17

[bhavana bajaj [:bajaj]]

Please nominate for uplift to Aurora/Beta no later than Monday. Please also prepare patches for ESR17/B2G.

Comment 19

[Jason Duell]

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.

Comment 20

[Johnny Stenback (:jst)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ed53e2df9157

Comment 21

[Jason Duell]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bc8378e25441
https://hg.mozilla.org/releases/mozilla-beta/rev/d8a545b0cb29
https://hg.mozilla.org/releases/mozilla-esr17/rev/2ea34ecded8e

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17306ccdd8d5