Closed Bug 1586845 Opened 2 years ago Closed 2 years ago

DNS resolve callback removal outside lock

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 70+ fixed
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 + fixed
firefox71 + fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

Details

(Keywords: csectype-race, sec-moderate, Whiteboard: [necko-triaged][adv-main70+][adv-main70+r][adv-esr68.2+][adv-esr68.2+r][post-critsmash-triage])

Attachments

(1 file)

[1] should probably be called under the lock. The callback list is normally manipulated only under nsHostResolver::mLock.

[1] https://searchfox.org/mozilla-central/rev/5cb522c7baba24e55874809e0e206b001494c1e9/netwerk/dns/nsHostResolver.cpp#1117-1119

Status: NEW → ASSIGNED
Attached file Bug 1586845, r=dragana

Daniel, I found this issue during code inspection related to bug 1513519. Not sure it will help with that bug, but I rather filed it as a sec bug anyway.

I'm not sure about sec rating and if this needs some kind of an approval (probably not?)

Thanks.

Flags: needinfo?(dveditz)
Keywords: sec-want

Better to leave the rating blank if you're unsure, or ask in #security. If you put something there it disappears from triage and can get lost.

You don't need sec-approval for the new rating of sec-moderate.

Flags: needinfo?(dveditz)

Thanks. I will land the patch directly.

Comment on attachment 9099312 [details]
Bug 1586845, r=dragana

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This may be related to high-rate sec bug 1513519 affecting fennec, hence it's worth landing this otherwise harmless patch to check if it fixes or progresses that bug.
  • User impact if declined: UAFs - this is a race.
  • Fix Landed on Version: 71
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This just locks removal of a smart-ptr from a structure, no foreign code calls made under the lock, hence no chance for new deadlocks.
  • String or UUID changes made by this patch:
Attachment #9099312 - Flags: approval-mozilla-esr68?

Comment on attachment 9099312 [details]
Bug 1586845, r=dragana

Beta/Release Uplift Approval Request

  • User impact if declined: Possible race on a refptr.
  • Is this code covered by automated tests?: Unknown
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce: There is no STR to trigger the race.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): see previous comment
  • String changes made/needed:
Attachment #9099312 - Flags: approval-mozilla-beta?

Comment on attachment 9099312 [details]
Bug 1586845, r=dragana

Fix for a bug which may affect other security-sensitive issues, OK for uplift for beta 14.

Attachment #9099312 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Is there any verification we can do (for desktop and for fennec) here?

Flags: needinfo?(honzab.moz)

(In reply to Liz Henry (:lizzard) from comment #9)

Is there any verification we can do (for desktop and for fennec) here?

I'm afraid not :( If we could, bug 1513519 would be fixed or more understood now...

Flags: needinfo?(honzab.moz)
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

Comment on attachment 9099312 [details]
Bug 1586845, r=dragana

Approved for Fennec 68.2b7. Fingers crossed that it helps with the crashes!

Attachment #9099312 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Whiteboard: [necko-triaged] → [necko-triaged][adv-main70+][adv-main70-rollup]
Whiteboard: [necko-triaged][adv-main70+][adv-main70-rollup] → [necko-triaged][adv-main70+][adv-main70-rollup][adv-esr68.2+][adv-esr68.2-rollup]
Flags: qe-verify-
Whiteboard: [necko-triaged][adv-main70+][adv-main70-rollup][adv-esr68.2+][adv-esr68.2-rollup] → [necko-triaged][adv-main70+][adv-main70-rollup][adv-esr68.2+][adv-esr68.2-rollup][post-critsmash-triage]
Whiteboard: [necko-triaged][adv-main70+][adv-main70-rollup][adv-esr68.2+][adv-esr68.2-rollup][post-critsmash-triage] → [necko-triaged][adv-main70+][adv-main70+r][adv-esr68.2+][adv-esr68.2+r][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.