1550498 - Clone connection info object with unprotected mHashKey when sending it between threads on more places

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

+++ This bug was initially created as a clone of Bug #1548822 +++

There are more places to fix by cloning CI:

• https://searchfox.org/mozilla-central/rev/197210b8c139b64f642edaa3336d26b9c1761568/netwerk/protocol/http/nsHttpConnectionMgr.cpp#521 (NullHttpTransaction doesn't clone in its ctor)

• https://searchfox.org/mozilla-central/rev/197210b8c139b64f642edaa3336d26b9c1761568/netwerk/protocol/http/nsHttpConnectionMgr.cpp#592

Comment 1

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #0)

+++ This bug was initially created as a clone of Bug #1548822 +++

There are more places to fix by cloning CI:

• https://searchfox.org/mozilla-central/rev/197210b8c139b64f642edaa3336d26b9c1761568/netwerk/protocol/http/nsHttpConnectionMgr.cpp#521 (NullHttpTransaction doesn't clone in its ctor)

I rather moved this to nsHttpHandler::SpeculativeConnect, other callers already provide a stable clone

I'm thinking of making the connection info object imutable (similarly to how we handle uris these days) to make this really safe and clear, but that is for a followup.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: Bug 1550498, r=kershaw

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 9063803 [details]
Bug 1550498, r=kershaw

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1548822#c6, the approval comment is exactly the same as should be for this bug.
• 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?:
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?:

Comment 4

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9063803 [details]
Bug 1550498, r=kershaw

sec-approval+ for trunk.

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/7bfe542574a942df1aaca059b8cc7d4da0cfa158

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7bfe542574a9

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please rebase this for ESR60 and nominate it for approval when you get a chance.

Comment 8

[Honza Bambas (:mayhemer)]

Will do.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please proceed with that ESR60 rebase and approval request :)

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: esr60

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: UAF
• Fix Landed on Version: 68
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): trivial patch
• String or UUID changes made by this patch:

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9075168 [details] [diff] [review]
esr60

Fixes a Necko sec bug. Approved for 60.8esr.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/e1b92ba4fb11