1090535 - Add Windows Firewall whitelist entry

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[Robert Strong (they/them - no direct email)]

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

Steps to reproduce:
1. Make sure rules for Firefox/Nightly/Aurora are not present in Windows Firewall.
   (Once Windows Firewall dialog pops up, the rule will be automatically added. So make sure rules are not present everytime.)
2. Launch Firefox.

Actual result:
Windows Firewall dialog pops up.

Expected result:
No dialog should be displayed.

Works https://hg.mozilla.org/releases/mozilla-aurora/rev/9d90f44e6585
Fails https://hg.mozilla.org/releases/mozilla-aurora/rev/ed68a14922fe

Suspected: Bug 1054959 - Add 'Send Video To Device' to the context menu for sending videos from desktop to a second screen

See Bug #1086278 for more info

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This would also be useful for WiFi device scanning in WebIDE, which also triggers a Windows Firewall popup today (see bug 1031045).

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: Changes to the liteFirewall plugin

http://nsis.sourceforge.net/LiteFirewall_Plugin

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: plugin compiled with VC6

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: Modified liteFirewall source

I'll add this to other-licenses/nsis/Contrib if all else goes well.

Comment 6

[Robert Strong (they/them - no direct email)]

Pushed to try so there are builds to test
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f8961dc6cb3

Comment 7

[Robert Strong (they/them - no direct email)]

oops... bad url
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f8961dc6cb35

Comment 8

[Robert Strong (they/them - no direct email)]

builds
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f8961dc6cb35/

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #8)
> builds
> https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/
> [e-mail]-f8961dc6cb35/

These installer builds appear to work well here on Windows 8.1, no more firewall prompts.

Comment 10

[Robert Strong (they/them - no direct email)]

Kamil, could you test installing this build on WinXP SP2 system?

https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f8961dc6cb35/

It should add an inbound TCP and UDP entry for the install for the Public network and if present the Private network.

Comment 11

[Brian R. Bondy [:bbondy]]

Comment on attachment 8535437 [details] [diff] [review]
Changes to the liteFirewall plugin

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

::: 2011-07-liteFirewall3/liteFirewall/src/liteFirewall.cpp
@@ +140,5 @@
> +
> +	// Add the Public profile if not already present
> +	if (!(CurrentProfilesBitMask & NET_FW_PROFILE2_PUBLIC))
> +	{
> +		CurrentProfilesBitMask |= NET_FW_PROFILE2_PUBLIC;

It makes me a little uneasy that we're adding this for everyone automatically and to the public profile. But if you are sure this is OK I won't stop you :)

Also it's a bit strange to me to have only PUBLIC given that most people will probably (maybe) select public. I think it would be better if you want to do this on a different profile to just specify NET_FW_PROFILE2_ALL?

@@ +174,5 @@
> +		goto Cleanup;
> +	}
> +
> +	// Release the INetFwRule object
> +	if (pFwRule != NULL)

This is probably copied from their pattern, but I sure hope it's not NULL since we were using it above :)

@@ +196,5 @@
>  
>  	// Populate the Firewall Rule object
>  	pFwRule->put_Name(bstrRuleName);
> +/* Start Mozilla modification */
> +//	pFwRule->put_Protocol(NET_FW_IP_PROTOCOL_ANY);

I don't really understand why this is necessary, I think the only options available to change are UDP and TCP so I'm not sure what adding them separately gets us.  I think it would be more simple to have less changes to the code. If you strongly disagree for whatever reason though just let me know.

Comment 12

[Brian R. Bondy [:bbondy]]

Comment on attachment 8535443 [details] [diff] [review]
patch rev1

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

Looks good so far, but please also call it from shared.nsh for existing installs who update.

Comment 13

[Brian R. Bondy [:bbondy]]

Not sure if this is a security concern or not but thought I'd mention it.
1. malware cannot access network under current profile so they just start firefox to download their new malware update or to send up personal info.
2. Firefox does it because it is whitelisted without the user's consent.

Comment 14

[Kamil Jozwiak [:kjozwiak]]

Attachment: WinXPFirewall.png

OS's Used:

- Windows XP Pro x86 SP2
- Windows XP Pro x86 SP3
- Windows XP Pro x64 SP3

I went through the following with each of the OS's that's listed above:

- downloaded the x86 build from the link in comment # 10
- installed fx without any issues (never received the Win firewall prompt)
- launched the build several times without any problems (never received the Win firewall prompt)
- ensured "Nightly" appeared under the "Exceptions" tab in the Win Firewall settings
- ensured that "Any computer (including those on the Internet)" has been selected under the "Change Scope" tab
- ran "netstat -ano" in the command prompt and opened Nighty (ensured that active connections appeared)
- restarted the machine and ensured that the "Nightly" entry is still being listed under the "Exceptions" tab

Robert, I couldn't find where Windows XP lists the actual inbound TCP/UDP ports that are currently opened for Nightly. After doing some research, it seems that opened ports should be listed under the "Exceptions" tab in the Windows Firewall settings but I didn't see any listed for Nightly. Only the "Nightly" entry is appearing in the list as mentioned in the test cases above. I posted a screenshot of what's being listed/available in the Windows Firewall settings.

Let me know if I missed something or if there's anything else that you need!

Comment 15

[Robert Strong (they/them - no direct email)]

Attachment: Client patch

Comment 16

[Robert Strong (they/them - no direct email)]

Attachment: plugin compiled with VC6

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: Changes to the liteFirewall plugin

I made it so it always uses the private network and that it doesn't try to add the exception when one already exists with the same name to prevent multiple entries along with some other code cleanup.

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: Modified liteFirewall source

Comment 19

[Brian R. Bondy [:bbondy]]

Comment on attachment 8539112 [details] [diff] [review]
Changes to the liteFirewall plugin

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

::: 2011-07-liteFirewall3/liteFirewall/src/liteFirewall.cpp
@@ +185,4 @@
>  	}
>  
> +	// Release the INetFwRule object
> +	if (pFwRule != NULL)

I'm fine with leaving it to avoid having to retest anything.

nit:
Per last time, if this `if` is needed I think it should appear above where we explicitly use pFwRule->
If not then the if check here is not needed.

Comment 20

[Robert Strong (they/them - no direct email)]

Attachment: Changes to the liteFirewall plugin

Made changes in comment #19. Carrying forward r+

Comment 21

[Robert Strong (they/them - no direct email)]

Attachment: plugin compiled with VC6

Comment 22

[Robert Strong (they/them - no direct email)]

Attachment: Source files with changes

Comment 23

[Robert Strong (they/them - no direct email)]

Note: tested this works with a clean WinXP SP2 system.

Comment 24

[Robert Strong (they/them - no direct email)]

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/cd3e066846f1

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd3e066846f1

Comment 26

[Robert Strong (they/them - no direct email)]

Comment on attachment 8539108 [details] [diff] [review]
Client patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1054959
[User impact if declined]: The user will see a Firewall prompt and they may choose to not accept it without realizing they will be disabling the feature in bug 1054959.
[Describe test coverage new/current, TBPL]: Manual testing using WinXP, Vista, Win7, and Win8 using the installer and it has been on Nightly for several days and is also invoked after an update.
[Risks and why]: The API is fairly simple and we are adding the same entries numerous other applications add so I think the risks are minimal.
[String/UUID change made/needed]: None

Comment 27

[Robert Strong (they/them - no direct email)]

Note: the binary plugin is the only other patch required for this to work so I only requested approval for the client code change patch.

Comment 28

[Robert Strong (they/them - no direct email)]

Brad and Karen can provide more info as to why this is needed

Comment 29

[Brad Lassey [:blassey] (use needinfo?)]

This prevents getting a firewall error when doing an SDP search

Comment 30

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/2c12736f741f

Pushed to mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/a349c33ba810