Closed Bug 1380426 Opened 2 years ago Closed 2 years ago

nsWebRequestListener should be thread safe

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox-esr5255+ fixed, firefox54 wontfix, firefox55+ fixed, firefox56+ fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 55+ fixed
firefox54 --- wontfix
firefox55 + fixed
firefox56 + fixed

People

(Reporter: dragana, Assigned: mixedpuppy)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [post-critsmash-triage][adv-main55+][adv-esr52.3+] triaged)

Attachments

(1 file)

nsWebRequestListener implements nsIThreadRetargetableStreamListener therefore it should be thread safe and should declare NS_DECL_THREADSAFE_ISUPPORTS
Shane, can you please take a look?
I'm not really clear on the sec impact here, but I'm happy to make the change.

Should we also update nsStreamListenerWrapper.h and the template in nsIThreadRetargetableStreamListener.h?  Those were my go-to sources when implementing, neither have NS_DECL_THREADSAFE_ISUPPORTS
Assignee: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy) → needinfo?(dd.mozilla)
We need to switch from NS_DECL_ISUPPORTS to NS_DECL_THREADSAFE_ISUPPORTS, and then make sure that all of our member variables are released on the main thread when the destructor is called.
Flags: needinfo?(dd.mozilla)
(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> I'm not really clear on the sec impact here, but I'm happy to make the
> change.
> 
> Should we also update nsStreamListenerWrapper.h and the template in
> nsIThreadRetargetableStreamListener.h?  Those were my go-to sources when
> implementing, neither have NS_DECL_THREADSAFE_ISUPPORTS

There is a bug for nsStreamListenerWrapper.h too. if you want you can take it, bug 1380428
I think we don't need to release anything in nsStreamListenerWrapper, and as that makes it a one line patch, I just included it here.
Attachment #8887239 - Flags: review?(kmaglione+bmo)
Attachment #8887239 - Attachment is patch: true
Attachment #8887239 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8887239 [details] [diff] [review]
make stream listeners use NS_DECL_THREADSAFE_ISUPPORTS

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  I'm not clear about this, assuming dragana & others are.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  Probably not, though the bug # is in the checkin comment.

Which older supported branches are affected by this flaw? I'm assuming all for nsStreamListenerWrapper.  Beta and Nightly for nsWebRequestListener. 

If not all supported branches, which bug introduced the flaw?  Bug is in two areas, nsStreamListenerWrapper is older, nsWebRequestListener I introduced in bug 1326298

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Same patch should apply.

How likely is this patch to cause regressions; how much testing does it need?  Existing tests should cover everything.
Attachment #8887239 - Flags: sec-approval?
sec-approval+ for trunk.
We'll want a Beta branch patch nominated ASAP to go in as soon as this lands on trunk. 

ESR52 is unaffected by this?
Attachment #8887239 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #7)
> sec-approval+ for trunk.
> We'll want a Beta branch patch nominated ASAP to go in as soon as this lands
> on trunk. 
> 
> ESR52 is unaffected by this?

Only nsStreamListenerWrapper.h is in ESR52, bug 1380428, the webextension listener is not.  dragana: do we need to separate this for ESR52?
Flags: needinfo?(dd.mozilla)
Probably easier to just land it here at this point and dupe the other bug over.
Let's land this here and dup other bug.
I will follow up in the other bug to figure out if we need to uplift this.
Flags: needinfo?(dd.mozilla)
Whiteboard: triaged
Priority: -- → P1
I see a lot of orange in win debug, but I don't see anything I think is related to this.
Keywords: checkin-needed
Yeah, the Win10 tests are still being greened up at the moment. Try push LGTM too.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0527bcb7c5c83f76c80097ba5c124c85f8b9e515
https://hg.mozilla.org/integration/mozilla-inbound/rev/71ee6c0f02ff2a936d0d6c6fdb20d7d23d6cf063

To make life easier on the uplift front, I split the patch into separate nsStreamListenerWrapper (with an implied r=dragana) and nsWebRequestListener parts. IIUC, both will need to go to Beta but only the second one will need to go to ESR52.
Duplicate of this bug: 1380428
https://hg.mozilla.org/mozilla-central/rev/71ee6c0f02ff
https://hg.mozilla.org/mozilla-central/rev/0527bcb7c5c8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Can you request uplift on the patch for esr?
Flags: needinfo?(mixedpuppy)
Comment on attachment 8887239 [details] [diff] [review]
make stream listeners use NS_DECL_THREADSAFE_ISUPPORTS

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: 56
Risk to taking this patch (and alternatives if risky): Only https://hg.mozilla.org/mozilla-central/rev/71ee6c0f02ff is needed for ESR, risk is minimal
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: 326298 for the webextension part
[User impact if declined]:
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: just landed
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: 
[Why is the change risky/not risky?]:
[String changes made/needed]: none
Flags: needinfo?(mixedpuppy)
Attachment #8887239 - Flags: approval-mozilla-esr52?
Attachment #8887239 - Flags: approval-mozilla-beta?
Comment on attachment 8887239 [details] [diff] [review]
make stream listeners use NS_DECL_THREADSAFE_ISUPPORTS

sec-high uaf fix, beta55+
Attachment #8887239 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8887239 [details] [diff] [review]
make stream listeners use NS_DECL_THREADSAFE_ISUPPORTS

... and for 52.3esr
Attachment #8887239 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Had to push a follow-up bustage fix to Beta too.
https://hg.mozilla.org/releases/mozilla-beta/rev/9111407305df
Whiteboard: triaged → [adv-main55+][adv-esr52.3+] triaged
Group: toolkit-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [adv-main55+][adv-esr52.3+] triaged → [post-critsmash-triage][adv-main55+][adv-esr52.3+] triaged
Julien, why is this tracking for ESR52.4 (coming out with Firefox 56) when it was fixed in ESR52.3?
Flags: needinfo?(jcristau)
oops, thanks for noticing.
Flags: needinfo?(jcristau)
Group: core-security-release
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.