Closed Bug 1214782 (CVE-2016-1945) Opened 4 years ago Closed 4 years ago

Latent wild-pointer/bounds bug in nsZipArchive

Categories

(Core :: Networking: JAR, defect)

41 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: q1, Assigned: baku)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main44+])

Attachments

(1 file)

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?
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: nobody → amarchesini
Attached patch crash.patchSplinter Review
Attachment #8674307 - Flags: review?(ehsan)
This would have been prevented by bug 525063.
Attachment #8674307 - Flags: review?(ehsan) → review+
(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.
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?
(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.
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)
(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.
I sent an mail to dev-platform to unship remote jar URIs.
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)
Keywords: checkin-needed
Group: core-security → network-core-security
https://hg.mozilla.org/mozilla-central/rev/aa49df6c573f
Status: NEW → RESOLVED
Closed: 4 years ago
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.