Closed
Bug 1214782
(CVE-2016-1945)
Opened 9 years ago
Closed 9 years ago
Latent wild-pointer/bounds bug in nsZipArchive
Categories
(Core :: Networking: JAR, defect)
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: q1, Assigned: baku)
Details
(Keywords: reporter-external, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main44+])
Attachments
(1 file)
7.14 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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);
...
Updated•9 years ago
|
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.
Updated•9 years ago
|
Component: Untriaged → Networking: JAR
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → amarchesini
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8674307 -
Flags: review?(ehsan)
Comment 3•9 years ago
|
||
This would have been prevented by bug 525063.
Updated•9 years ago
|
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.
Assignee | ||
Comment 5•9 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?
(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•9 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)
Comment 8•9 years ago
|
||
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> :(
(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•9 years ago
|
||
I sent an mail to dev-platform to unship remote jar URIs.
Updated•9 years ago
|
Keywords: sec-moderate
Comment 11•9 years ago
|
||
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?
Comment 12•9 years ago
|
||
(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•9 years ago
|
Keywords: checkin-needed
Comment 13•9 years ago
|
||
Keywords: checkin-needed
Updated•9 years ago
|
Group: core-security → network-core-security
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated•9 years ago
|
Group: network-core-security → core-security-release
Updated•9 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main44+]
Updated•9 years ago
|
Alias: CVE-2016-1945
Updated•8 years ago
|
Group: core-security-release
Updated•6 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•