Closed Bug 1538619 Opened 1 year ago Closed 1 year ago

TransactionDatabaseOperationBase::SendFailureResult not called if the actor has been destroyed

Categories

(Core :: Storage: IndexedDB, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 67+ fixed
firefox66 --- wontfix
firefox67 + fixed
firefox68 + fixed

People

(Reporter: janv, Assigned: janv)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main67+][adv-esr60.7+])

Crash Data

Attachments

(2 files)

This is probably the root cause of sec bugs like bug 1499719 and bug 1499108.
It also contributes to quota manager shutdown hangs (there may still be other issues that cause quota manager shutdown hangs).

Not calling TransactionDatabaseOperationBase::SendFailureResult is normally ok, but in the case of OpenDatabaseOp::VersionChangeOp, it means that the parent operation (OpenDatabaseOp) won't have a chance to correctly cleanup.

I found this when I was working on bug 1533789 which involves new local storage implementation which uses a similar design like IDB.

Assignee: nobody → jvarga
Status: NEW → ASSIGNED

Just apply the patch and then run:
mach xpcshell-test dom/indexedDB/test/unit/test_put_get_values.js
and you should get a quota manager shutdown hang.

Priority: -- → P1

Andrew, do you need anything else for this ?
It would be nice to see if this also helps with lowering down number of quota manager shutdown hangs.

Group: core-security → dom-core-security

Andrew, please see comment 5.

Flags: needinfo?(bugmail)
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Ok, we will see now in bug 1541776 what effect this has on shutdown hangs.

Flags: needinfo?(bugmail)

Just FYI, this didn't have big impact on shutdown hangs, but it should fix some use after free sec issues.

Comment on attachment 9053338 [details]
Bug 1538619 - TransactionDatabaseOperationBase::SendFailureResult not called if the actor has been destroyed; r=asuth

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Probably last IDB rewrite
  • User impact if declined: Potential use after free security issues.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Verified on Nightly (landed 9 days ago), small changes and thorough review.
  • String changes made/needed: None

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: Potential use after free security issues.
  • Fix Landed on Version: 68
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Verified on Nightly (landed 9 days ago), small changes and thorough review.
  • String or UUID changes made by this patch: None
Attachment #9053338 - Flags: approval-mozilla-esr60?
Attachment #9053338 - Flags: approval-mozilla-beta?
Crash Signature: [@ mozilla::dom::indexedDB::`anonymous namespace'::QuotaClient::ShutdownTimedOut] [@ mozilla::dom::indexedDB::(anonymous namespace)::QuotaClient::ShutdownTimedOut]

I think this should have had sec-approval for landing on mozilla-central as this is rated as a sec-high and wasn't caused by a recent regression on mozilla-central (https://wiki.mozilla.org/Security/Bug_Approval_Process). Jan could you fill in the form a posteriori so as that it gets on our security team radar please? Thanks

Flags: needinfo?(jvarga)

Comment on attachment 9053338 [details]
Bug 1538619 - TransactionDatabaseOperationBase::SendFailureResult not called if the actor has been destroyed; r=asuth

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: It's a use after free issue, but the patch doesn't indicate that at all.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
  • Which older supported branches are affected by this flaw?: all
  • 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?: Should be very easy to provide a patch for beta and ESR, the original patch may apply cleanly.
  • How likely is this patch to cause regressions; how much testing does it need?: It's been tested on Nightly and there were no regressions.
Flags: needinfo?(jvarga)
Attachment #9053338 - Flags: sec-approval?

Sec-approval+ for trunk. Yes, this should have had approval before going in.

I'll approve Beta as well.

Attachment #9053338 - Flags: sec-approval?
Attachment #9053338 - Flags: sec-approval+
Attachment #9053338 - Flags: approval-mozilla-beta?
Attachment #9053338 - Flags: approval-mozilla-beta+
Crash Signature: [@ mozilla::dom::indexedDB::`anonymous namespace'::QuotaClient::ShutdownTimedOut] [@ mozilla::dom::indexedDB::(anonymous namespace)::QuotaClient::ShutdownTimedOut] → [@ mozilla::dom::indexedDB::`anonymous namespace'::QuotaClient::ShutdownTimedOut] [@ mozilla::dom::indexedDB::(anonymous namespace)::QuotaClient::ShutdownTimedOut] [@ shutdownhang | mozilla::SpinEventLoopUntil<T> | mozilla::dom::quota::QuotaManager::Obs…
Flags: qe-verify-

Giving this another few days to see the results on beta before we do the ESR uplift.

Comment on attachment 9053338 [details]
Bug 1538619 - TransactionDatabaseOperationBase::SendFailureResult not called if the actor has been destroyed; r=asuth

Fix for sec-high crash, OK for esr 60.7.

Attachment #9053338 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [adv-main67+][adv-esr60.7+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.