Closed Bug 1611482 Opened 2 years ago Closed 2 years ago

Double free inside nsZipHandle upon FindDataStart failure

Categories

(Core :: Networking: JAR, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla76
Tracking Status
firefox-esr68 76+ fixed
firefox74 --- wontfix
firefox75 - wontfix
firefox76 + fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

(Regression)

Details

(Keywords: regression, sec-moderate, Whiteboard: [necko-triaged][post-critsmash-triage][adv-main76+r][adv-ESR68.8+r])

Crash Data

Attachments

(1 file)

https://hg.mozilla.org/releases/mozilla-release/annotate/8260da04c9b13f7c0e9cc6984a75e689b5fcb8c8/modules/libjar/nsZipArchive.cpp#l217 de-allocated the map, however at that point it has already been set on the handle object, handle->mMap and handle->mFileStart are not dangling pointers.

In ~nsZipHandle it will proceed to attempt to de-allocate the memory and trigger a double free.

Double free sounds like a security issue

Group: core-security
Priority: -- → P1
Whiteboard: [necko-triaged]

It can be a problem, but I think in this case there's a very small window between failure and the deconstructor so it'd be a hard race to win reliably. Calling it sec-moderate for now.

Group: core-security → network-core-security
Keywords: sec-moderate

Comment on attachment 9122942 [details]
Bug 1611482: Prevent the existance of dangling pointers upon failure of FindDataStart. r=spohl

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Window is pretty small as Daniel mentioned. And it's not even clear to me what triggers this situation. It's based on code analysis and crash stats.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • Which older supported branches are affected by this flaw?: All I believe
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: Low risk, this patch should apply.
  • How likely is this patch to cause regressions; how much testing does it need?: Not really.
Attachment #9122942 - Flags: sec-approval?
Crash Signature: https://crash-stats.mozilla.com/report/index/08463fc8-dd72-4d69-a966-dcbe80200124 → [@ arena_t::DallocSmall | je_free | PR_CloseFileMap | nsZipHandle::~nsZipHandle ]

Comment on attachment 9122942 [details]
Bug 1611482: Prevent the existance of dangling pointers upon failure of FindDataStart. r=spohl

It's too late to get into next week's releases, but looking at the crash volume we'll want this fix on ESR for next time too.

Attachment #9122942 - Flags: sec-approval? → sec-approval+
Group: network-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76

Please nominate this for ESR68 approval when you get a chance.

Flags: needinfo?(bas)

Comment on attachment 9122942 [details]
Bug 1611482: Prevent the existance of dangling pointers upon failure of FindDataStart. r=spohl

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sec moderate, but low risk and an easy fix to take.
  • User impact if declined: Potential UAF
  • Fix Landed on Version: 77
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Simply removes a dangling pointer.
  • String or UUID changes made by this patch: None.
Flags: needinfo?(bas)
Attachment #9122942 - Flags: approval-mozilla-esr68?

Comment on attachment 9122942 [details]
Bug 1611482: Prevent the existance of dangling pointers upon failure of FindDataStart. r=spohl

approved for 68.8esr

Attachment #9122942 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Flags: qe-verify-
Whiteboard: [necko-triaged] → [necko-triaged][post-critsmash-triage]
Whiteboard: [necko-triaged][post-critsmash-triage] → [necko-triaged][post-critsmash-triage][adv-main76+r]
Whiteboard: [necko-triaged][post-critsmash-triage][adv-main76+r] → [necko-triaged][post-critsmash-triage][adv-main76+r][adv-ESR68.8+r]
Group: core-security-release
Has Regression Range: --- → yes
Keywords: regression
You need to log in before you can comment on or make changes to this bug.