Bug 1214782 (CVE-2016-1945)

Latent wild-pointer/bounds bug in nsZipArchive

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: q1, Assigned: baku)

Tracking

({sec-moderate})

41 Branch
mozilla44
sec-moderate
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main44+])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Class nsZipArchive (modules\libjar\nsZipArchive.cpp) does not initialize its mCommentPtr and mCommentLen fields unless the archive contains a valid comment. Further, nsZipArchive::GetComment uses mCommentPtr and mCommentLen without checking whether the archive contains a valid comment. Thus, it can return a comment string containing data from memory that it has no right to access, potentially leaking sensitive data from one context into another:

850: bool nsZipArchive::GetComment(nsACString &aComment)
851: {
852: MOZ_WIN_MEM_TRY_BEGIN
853:   aComment.Assign(mCommentPtr, mCommentLen);
854: MOZ_WIN_MEM_TRY_CATCH(return false)
855:   return true;
856: }

At present, this bug appears to be latent, or nearly so, because GetComment's only caller is StartupCache::LoadArchive (startupcache\StartupCache.cpp), and (as far as I know) the startup cache stream is trusted, so it should contain a valid comment. Presumably there might not be a valid comment if the cache file became corrupted, or if a cache file having a different format were restored from a backup, in which case LoadArchive might be affected:

240: nsresult
241: StartupCache::LoadArchive(enum TelemetrifyAge flag)
242: {
...
253:   rv = mArchive->OpenArchive(mFile);
254:   if (NS_FAILED(rv) || flag == IGNORE_AGE)
255:     return rv;
256:
257:   nsCString comment;
258:   if (!mArchive->GetComment(comment)) {
259:     return rv;
260:   }
261:
262:   const char *data;
263:   size_t len = NS_CStringGetData(comment, &data);
264:   PRTime creationStamp;
265:   // We might not have a comment if the startup cache file was created
266:   // before we started recording creation times in the comment.
267:   if (len == sizeof(creationStamp)) {
268:     memcpy(&creationStamp, data, len);
...
Flags: sec-bounty?
(Reporter)

Comment 1

3 years ago
In the context of StartupCache::LoadArchive, the nsZipArchive object is filled with 0xe4 bytes before construction, so |mCommentPtr| == 0xe4e4e4e4, and |mCommentLen| == 0xe4e4. 0xe4e4e4e4 can be a valid address on FF Win32 running on Win64 (WOW64). BTW, this fill appears to be done by one of the arena malloc functions in jemalloc.c.
Component: Untriaged → Networking: JAR
(Assignee)

Updated

3 years ago
Assignee: nobody → amarchesini
(Assignee)

Comment 2

3 years ago
Created attachment 8674307 [details] [diff] [review]
crash.patch
Attachment #8674307 - Flags: review?(ehsan)

Comment 3

3 years ago
This would have been prevented by bug 525063.

Updated

3 years ago
Attachment #8674307 - Flags: review?(ehsan) → review+
(Reporter)

Comment 4

3 years ago
(In reply to Ehsan Akhgari (don't ask for review please) from comment #3)
> This would have been prevented by bug 525063.

That would be a very worthwhile project.
(Assignee)

Comment 5

3 years ago
Comment on attachment 8674307 [details] [diff] [review]
crash.patch

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

nsZipArchive is not directly exposed to content. I don't think it's possible to exploit from the content.

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

Yes. We don't initialize some member of some class in nsZipArchive.* (and probably in many other classes...). We need some static analysis tool for this.

Which older supported branches are affected by this flaw?

All.

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

0 risk.

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

no risks.
Attachment #8674307 - Flags: sec-approval?
(Reporter)

Comment 6

3 years ago
(In reply to Andrea Marchesini (:baku) from comment #5)
> Comment on attachment 8674307 [details] [diff] [review]
> crash.patch
> 
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> 
> nsZipArchive is not directly exposed to content. I don't think it's possible
> to exploit from the content.

It's exposed at least to XPIs. Create foo.xpi as a local file with any content. Then attach a debugger to FF and set a BP in nsZipArchive::OpenArchive, open the file from the URL bar, and watch the BP trigger.

Comment 7

3 years ago
It looks like we support loading remote jar files served with application/x-java-archive mime type.  If that is indeed true, this is remotely exploitable.  :(
Flags: needinfo?(dveditz)
(Reporter)

Comment 9

3 years ago
(In reply to Ehsan Akhgari (don't ask for review please) from comment #8)
> That is, application/java-archive.
> 
> This URL can be loaded n Firefox:
> <jar:http://mxr.mozilla.org/mozilla-central/source/modules/libjar/test/
> mochitest/bug403331.zip?raw=1&ctype=application/java-archive!/test.html>  :(

Yes, that triggers a call to nsZipArchive::OpenArchive.

Comment 10

3 years ago
I sent an mail to dev-platform to unship remote jar URIs.
Keywords: sec-moderate
Comment on attachment 8674307 [details] [diff] [review]
crash.patch

I made this a sec-moderate rated issue. I've cleared the sec-approval? since it isn't necessary to checkin now.
Attachment #8674307 - Flags: sec-approval?
(In reply to Ehsan Akhgari (don't ask for review please) from comment #7)
> It looks like we support loading remote jar files served with
> application/x-java-archive mime type.  If that is indeed true, this is
> remotely exploitable.  :(

We support reading archive file contents from remote jar: (originally intended for "packaged" content like slideshows or whatever to make them easy to share, handy for multi-file bug PoCs in bugzilla) but the comments aren't used anywhere other than the startup cache as an optimization for locally controlled archives. Some other hypothetical zip archive bug might be remotely exploitable but this one is not.
Flags: needinfo?(dveditz)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Group: core-security → network-core-security
https://hg.mozilla.org/mozilla-central/rev/aa49df6c573f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: network-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main44+]
Alias: CVE-2016-1945
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.