Add an option to disable SSDP in Firefox

VERIFIED FIXED in Firefox 37

Status

()

defect
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: raysatiro, Assigned: Gijs)

Tracking

37 Branch
mozilla39
x86_64
Windows 7
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox36? affected, firefox37+ verified, firefox38+ verified, firefox39+ verified)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20141215030201

Steps to reproduce:

Feature request


Actual results:

Both Aurora and Nightly are doing something with SSDP.


Expected results:

I want an option to stop Firefox from sending out SSDP packets and listening for SSDP. I have UPnP disabled on my network.
(Assignee)

Updated

4 years ago
Blocks: 1054959
Severity: normal → enhancement
Component: Untriaged → General
Product: Firefox → Toolkit

Comment 1

4 years ago
I guess the SSDP requests are part and parcel of the 'Send Video To Device' service? Personally I have no need for this and I too would like the ability to disable the network discovery requests. I also have SSDP/UPnP services disable in Windows.

Comment 2

4 years ago
I can confirm this behaviour now exists in Firefox 36 release on desktop. Seen on WinXP x32 and Win7 x64. 

Every couple of minutes the local firewall prompts about connection attempt from Firefox towards 239.255.255.250:1900 which seems like a multicast address. 

Before writing here I have done some reaserch and appears to be related to the "Send Video To Device" functionality. 
SSDP and UPnP services are disabled.

I have no use for this functionality. How can I disable it in Firefox without the need to add blocking rules to my firewall? 

Thank you.

Comment 3

4 years ago
I Have encountered Windows Firewall allowance request/prompts on both a newly formatted system via the install stub, and another system via About>Update method of updating. I have heard that this only happens on some systems, but it has happened on both I have tested.

I do not own, nor do I intend to own a Roku or Chromecast. Is this function something that could be disabled by default, and enabled via some means (A button that reads "Click to Scan Network", or an official plug-in or add-on?) for those who want such functionality? As it is now, I'm reluctant to update and use Firefox. I generally don't just click "allow" on a Windows Firewall prompt without a really good reason.
(Assignee)

Comment 4

4 years ago
Gavin/Brad, can you clarify if we would consider adding a pref?
Flags: needinfo?(gavin.sharp)
Flags: needinfo?(blassey.bugs)

Comment 5

4 years ago
IMO this functionality should be both as a pref (about:config) and have GUI option under Tools/Options. Probably on the Advanced section, network tab.

Comment 6

4 years ago
There could also be a menu item somewhere, as suggested above, to scan for compatible devices, which should activate this feature either by default, or with confirmation.
(In reply to :Gijs Kruitbosch from comment #4)
> Gavin/Brad, can you clarify if we would consider adding a pref?

No objection to adding a pref, but I don't think this is something we want to turn off by default or require a user to manually scan for. I believe local network device discovery is something we're going to be doing more and more of going forward. That said, its not my call.
Flags: needinfo?(blassey.bugs)

Comment 8

4 years ago
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #7)
> (In reply to :Gijs Kruitbosch from comment #4)
> > Gavin/Brad, can you clarify if we would consider adding a pref?
> 
> No objection to adding a pref, but I don't think this is something we want
> to turn off by default or require a user to manually scan for. I believe
> local network device discovery is something we're going to be doing more and
> more of going forward. That said, its not my call.

Personally, I would have found no issue with a "Firefox Hello-like" button that simply enabled the network settings/SSDP. I could have just not clicked it, removed it from the toolbar, and not given it another consideration. That would make it easy for those who want the functionality, and not compel users to begin poking holes in their security for functionality they have no use for. Having these settings on by default just seems like it's poking holes, personally, especially when many users aren't informed about the potential risks associated with SSDP/UPnP.

Right now, I'm kind of waiting on the side lines to see how this plays out. Reluctant to update to v36 on any other systems, at present.

Comment 9

4 years ago
(In reply to Chaze Zulli from comment #8)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #7)
> > (In reply to :Gijs Kruitbosch from comment #4)
> > > Gavin/Brad, can you clarify if we would consider adding a pref?
> > 
> > No objection to adding a pref, but I don't think this is something we want
> > to turn off by default or require a user to manually scan for. I believe
> > local network device discovery is something we're going to be doing more and
> > more of going forward. That said, its not my call.
> 
> Personally, I would have found no issue with a "Firefox Hello-like" button
> that simply enabled the network settings/SSDP. I could have just not clicked
> it, removed it from the toolbar, and not given it another consideration.
> That would make it easy for those who want the functionality, and not compel
> users to begin poking holes in their security for functionality they have no
> use for. Having these settings on by default just seems like it's poking
> holes, personally, especially when many users aren't informed about the
> potential risks associated with SSDP/UPnP.
> 
> Right now, I'm kind of waiting on the side lines to see how this plays out.
> Reluctant to update to v36 on any other systems, at present.

I totally agree. Wish I hadn't updated at this point, too lazy to roll back. I also think "going forward with local network device discovery" should include considering security issues and that by default, it shouldn't be enabled. Personally, I consider this current UDP calling out to be added bloat.
New features should have come with a disabling pref, _especially_ new features that implement new network protocols.
Severity: enhancement → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
We have a few prefs in the Android side to control the feature. The main one controls the SSDP feature here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/CastingApps.js#83

I don't see that used in the desktop side.
We should have a pref, and maybe even expose it in the UI.
Points: --- → 2
Flags: qe-verify-
Flags: needinfo?(gavin.sharp)
Flags: firefox-backlog+
[Tracking Requested - why for this release]: If this is related to bug 1136772 we should probably track this for 37.
(Assignee)

Comment 14

4 years ago
I opted to use the pref in comment #11 here, and expose UI that would be understandable to most users. The downside is that the link to SSDP/UPnP might not be obvious to non-technical users. OTOH, if we make a 'technical' pref under advanced/network, that's not very discoverable for 'normal' users, and users might be confused why their roku/whatever doesn't 'just work'.
Attachment #8573898 - Flags: review?(gavin.sharp)
(Assignee)

Updated

4 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Iteration: --- → 39.1 - 9 Mar
(Assignee)

Comment 15

4 years ago
I realized that the code in content.js needed tweaking s, as did the commit msg.
Attachment #8573910 - Flags: review?(gavin.sharp)
(Assignee)

Updated

4 years ago
Attachment #8573898 - Attachment is obsolete: true
Attachment #8573898 - Flags: review?(gavin.sharp)
How about adding the "Enable media sharing" menu item under the "Send Video To Device" menu item if it is disabled? (Feel free to bikeshed the wording.) It would be well understandable and discoverable.
Comment on attachment 8573910 [details] [diff] [review]
add UI-exposed pref for casting,

Let's not add the UI pref just yet.

Why is the "Register targets" code block inside the SimpleServiceDiscovery lazy getter in content.js there? Isn't that redundant given that it's also in _initServiceDiscovery in nsBrowserGlue?
Attachment #8573910 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 18

4 years ago
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #17)
> Why is the "Register targets" code block inside the SimpleServiceDiscovery
> lazy getter in content.js there? Isn't that redundant given that it's also
> in _initServiceDiscovery in nsBrowserGlue?

I don't know the answer to this question. Isn't content.js in-content-process, though, where I assume nsBrowserGlue only runs in-chrome-process? Brad?

(In reply to Masatoshi Kimura [:emk] from comment #16)
> How about adding the "Enable media sharing" menu item under the "Send Video
> To Device" menu item if it is disabled? (Feel free to bikeshed the wording.)
> It would be well understandable and discoverable.

Which menu item? They are hidden if no media sharing devices are detected.

I would also argue that "media sharing" is too generic (Sharing how? Which media? With whom?) to be useful, and that having a toplevel menuitem for something which only works with devices which a tiny subset of our user population own isn't a good tradeoff.
Flags: needinfo?(blassey.bugs)
(In reply to Masatoshi Kimura [:emk] from comment #16)
> How about adding the "Enable media sharing" menu item under the "Send Video
> To Device" menu item if it is disabled? (Feel free to bikeshed the wording.)
> It would be well understandable and discoverable.

Something like this is worth considering - can you file a separate bug?
(Assignee)

Comment 20

4 years ago
Per IRC discussion, leaving content.js/nsBrowserGlue.js aside for the moment.
Attachment #8574651 - Flags: review?(gavin.sharp)
(Assignee)

Updated

4 years ago
Attachment #8573910 - Attachment is obsolete: true
(In reply to :Gijs Kruitbosch from comment #18)
> (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #17)
> > Why is the "Register targets" code block inside the SimpleServiceDiscovery
> > lazy getter in content.js there? Isn't that redundant given that it's also
> > in _initServiceDiscovery in nsBrowserGlue?
> 
> I don't know the answer to this question. Isn't content.js
> in-content-process, though, where I assume nsBrowserGlue only runs
> in-chrome-process? Brad?
I don't know.
Flags: needinfo?(blassey.bugs)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #19)
> (In reply to Masatoshi Kimura [:emk] from comment #16)
> > How about adding the "Enable media sharing" menu item under the "Send Video
> > To Device" menu item if it is disabled? (Feel free to bikeshed the wording.)
> > It would be well understandable and discoverable.
> 
> Something like this is worth considering - can you file a separate bug?

Filed bug 1141054.
(Assignee)

Comment 23

4 years ago
I think this is what you meant, but I can't find a way to verify that modules aren't loaded... Cu.unload just fails silently, it seems.
Attachment #8574660 - Flags: review?(gavin.sharp)
(Assignee)

Updated

4 years ago
Attachment #8574651 - Attachment is obsolete: true
Attachment #8574651 - Flags: review?(gavin.sharp)
(Assignee)

Comment 24

4 years ago
Comment on attachment 8574660 [details] [diff] [review]
honor browser.casting.enabled pref for casting on desktop,

Nope, because:

Cu.isModuleLoaded("resource://gre/modules/SimpleServiceDiscovery.jsm")

does exist and returns true, so I've missed something, though I'm not sure what...
Attachment #8574660 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 25

4 years ago
Comment on attachment 8574660 [details] [diff] [review]
honor browser.casting.enabled pref for casting on desktop,

... it would help if I did actually toggle the pref before checking, in which case this works, AFAICT. Gavin? :-)
Attachment #8574660 - Flags: review- → review?(gavin.sharp)
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
(Assignee)

Comment 26

4 years ago
Comment on attachment 8574660 [details] [diff] [review]
honor browser.casting.enabled pref for casting on desktop,

Mike, do you have time to review this? We're trying to get this to make 37 still...
Attachment #8574660 - Flags: review?(mconley)
Comment on attachment 8574660 [details] [diff] [review]
honor browser.casting.enabled pref for casting on desktop,

Review of attachment 8574660 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me. It's unfortunate that this didn't have a disabling pref baked in, as it's not amazing to have to check Services.prefs in so many places. :/

As a patch one can land on beta though, this looks fine. I do think we'll want to have folks kick the tires on this though to make sure casting still works properly, and that flipping the pref empties out all of the UI and kills network discovery.

::: browser/base/content/content.js
@@ +104,5 @@
>    docShell.mixedContentChannel = null;
>  });
>  
>  addMessageListener("SecondScreen:tab-mirror", function(message) {
> +  if (!Services.prefs.getBoolPref("browser.casting.enabled")) {

This is probably not entirely necessary, since browser.casting.enabled = false will hide all of the UI that might send this message, no? Is this a belt + suspenders kind of thing, or am I missing something?
Attachment #8574660 - Flags: review?(mconley) → review+
(Assignee)

Comment 28

4 years ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #27)
> Comment on attachment 8574660 [details] [diff] [review]
> honor browser.casting.enabled pref for casting on desktop,
> 
> Review of attachment 8574660 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks OK to me. It's unfortunate that this didn't have a disabling pref
> baked in, as it's not amazing to have to check Services.prefs in so many
> places. :/
> 
> As a patch one can land on beta though, this looks fine. I do think we'll
> want to have folks kick the tires on this though to make sure casting still
> works properly, and that flipping the pref empties out all of the UI and
> kills network discovery.

For definite.

> ::: browser/base/content/content.js
> >  addMessageListener("SecondScreen:tab-mirror", function(message) {
> > +  if (!Services.prefs.getBoolPref("browser.casting.enabled")) {
> 
> This is probably not entirely necessary, since browser.casting.enabled =
> false will hide all of the UI that might send this message, no? Is this a
> belt + suspenders kind of thing

Yup.
(Assignee)

Comment 29

4 years ago
Comment on attachment 8574660 [details] [diff] [review]
honor browser.casting.enabled pref for casting on desktop,

remote:   https://hg.mozilla.org/integration/fx-team/rev/7d4d1f4e9b72
Attachment #8574660 - Flags: review?(gavin.sharp)
Flags: qe-verify- → qe-verify+

Comment 30

4 years ago
Took me a while to track down who was spamming my network with UPnP requests. Never thought it would turn out to be a Mozilla product, let alone Firefox. Glad to see the bug request is Assigned. A fix can't come soon enough.
[Tracking Requested - why for this release]:

If this will prevent bug 1136772, I think it would be a good candidate to ride a dot release.
https://hg.mozilla.org/mozilla-central/rev/7d4d1f4e9b72
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 33

4 years ago
Comment on attachment 8574660 [details] [diff] [review]
honor browser.casting.enabled pref for casting on desktop,

Approval Request Comment
[Feature/regressing bug #]: casting
[User impact if declined]: can't prevent Firefox from using simpleservicediscovery and ssdp/upnp protocols
[Describe test coverage new/current, TreeHerder]: no :-(
[Risks and why]: low-medium: sticks everything behind a pref, keeps the pref true for now (we potentially want to flip it to false on 37, at least, AIUI)
[String/UUID change made/needed]: no
Attachment #8574660 - Flags: approval-mozilla-beta?
Attachment #8574660 - Flags: approval-mozilla-aurora?
(In reply to Gijs Kruitbosch (unavailable until Friday March 13th) from comment #33)
> [Feature/regressing bug #]: casting
Do you have a bug ref? Or, in what release did we first ship casting?

> [Describe test coverage new/current, TreeHerder]: no :-(
> [Risks and why]: low-medium: sticks everything behind a pref, keeps the pref
> true for now (we potentially want to flip it to false on 37, at least, AIUI)

This change only recently landed on m-c. Given the risk, I would prefer to see this fix verified in Nightly before shipping it to the Beta audience. Can you ensure that this is tested in tomorrow's Nightly build? If the testing is successful, we can take this fix in Beta 6, which gtb on Mon, Mar 16.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #34)
> > [Feature/regressing bug #]: casting
> Do you have a bug ref? Or, in what release did we first ship casting?

bug 1054959 and bug 1088758.
Thanks for the references Gavin. 36 should be unaffected.
This very much affects 36 (comment 2 and the user alarm over bug 1136772) which is why Felipe wondered if we could get this out in a 36 dot release (comment 31)
Flags: needinfo?(lmandel)
(Assignee)

Updated

4 years ago
Flags: needinfo?(gijskruitbosch+bugs)
You're right. I missed the comment and keyed off of the wrong bug. I have nomed for tracking 36 to add this to our list of potential bugs for a point release.
Flags: needinfo?(lmandel)
Comment on attachment 8574660 [details] [diff] [review]
honor browser.casting.enabled pref for casting on desktop,

Since this is on central and we might want to respin for it, let's get it onto Aurora/Beta now to get more users with the fix.
Attachment #8574660 - Flags: approval-mozilla-beta?
Attachment #8574660 - Flags: approval-mozilla-beta+
Attachment #8574660 - Flags: approval-mozilla-aurora?
Attachment #8574660 - Flags: approval-mozilla-aurora+
Note that this patch on its own doesn't fix anything - we would need to pair it with bug 1142521.
This fix seems to go hand-in-hand with the one pushed for Bug 1142521, but I'm not quite sure about the extent of what needs to be confirmed here by manual QA. 

At this point, Fx 37.0b6 (20150316202753) shows 'browser.casting.enabled' false by default and video/tab casting is indeed unavailable. Is there anything else I should be looking at?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 44

4 years ago
(In reply to Andrei Vaida, QA [:avaida] from comment #43)
> This fix seems to go hand-in-hand with the one pushed for Bug 1142521, but
> I'm not quite sure about the extent of what needs to be confirmed here by
> manual QA. 
> 
> At this point, Fx 37.0b6 (20150316202753) shows 'browser.casting.enabled'
> false by default and video/tab casting is indeed unavailable. Is there
> anything else I should be looking at?

Does toggling it to true (and restarting) make it work again? Are there any relevant errors if you try to access casting functionality anyway while it is disabled?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #44)

> Does toggling it to true (and restarting) make it work again? Are there any
> relevant errors if you try to access casting functionality anyway while it
> is disabled?

I tried to verify this issue using Firefox 39.0a1 (2015-03-22) and Firefox 37 Beta 7 (20150319212106) on a desktop with Windows 7 x64 and on a Surface Pro with Windows 8.1 x64 and this is what I have noticed:

 - There are no relevant errors if you try to access casting functionality while it is disabled.

 - After I set the pref 'browser.casting.enabled' to true, the 'Send Video to Device' option from the contextmenu is still grayed out even if I have a Roku device connected to the same wifi network.
I can successfully cast to Roku using Firefox installed on an Android device connected to the same wifi network.

  Should I file a separate bug for the issue mentioned?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 46

4 years ago
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #45)
>  - After I set the pref 'browser.casting.enabled' to true, the 'Send Video
> to Device' option from the contextmenu is still grayed out even if I have a
> Roku device connected to the same wifi network.

Did you restart after toggling the pref before trying to use the Roku? Does this reproduce on trunk/aurora as well?

And yes, this should be a different bug.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #46)

> Did you restart after toggling the pref before trying to use the Roku? Does
> this reproduce on trunk/aurora as well?

I confirm that I restart the browser after I changed the pref to true.

Today I tried to verify this issue on two different networks,but the 'Send Video to Device' option from the contextmenu is still grayed out on Nightly 39.0a1 (2015-03-23) and Aurora 38.0a2 (2015-03-24).

To cast a video from an Android device to Roku it is needed a Firefox application. There is no need for such an application on desktop as well?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 48

4 years ago
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #47)
> (In reply to :Gijs Kruitbosch from comment #46)
> 
> > Did you restart after toggling the pref before trying to use the Roku? Does
> > this reproduce on trunk/aurora as well?
> 
> I confirm that I restart the browser after I changed the pref to true.
> 
> Today I tried to verify this issue on two different networks,but the 'Send
> Video to Device' option from the contextmenu is still grayed out on Nightly
> 39.0a1 (2015-03-23) and Aurora 38.0a2 (2015-03-24).
> 
> To cast a video from an Android device to Roku it is needed a Firefox
> application. There is no need for such an application on desktop as well?

Please file a different bug and ask questions of blassey and/or rbarker. See also bug 1054959 comment 56 and later.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch (away 26-30/3) from comment #48)

> Please file a different bug and ask questions of blassey and/or rbarker. See
> also bug 1054959 comment 56 and later.

Closing this bug as the remaining issue is tracked in the followup bug: Bug 1147873.

Thanks Gijs for your guidance!
You need to log in before you can comment on or make changes to this bug.