Add Windows Firewall whitelist entry

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 wontfix, firefox35 fixed, firefox36 fixed, firefox37 fixed, firefox-esr31 wontfix)

Details

Attachments

(5 attachments, 7 obsolete attachments)

168.14 KB, image/png
Details
4.08 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
6.17 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
5.37 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
42.46 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
+++ 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
This would also be useful for WiFi device scanning in WebIDE, which also triggers a Windows Firewall popup today (see bug 1031045).
Blocks: 1031045
Posted patch patch rev1 (obsolete) — Splinter Review
Attachment #8535443 - Flags: review?(netzen)
Posted patch plugin compiled with VC6 (obsolete) — Splinter Review
Attachment #8535444 - Flags: review?(netzen)
Posted file Modified liteFirewall source (obsolete) —
I'll add this to other-licenses/nsis/Contrib if all else goes well.
(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/
> rstrong@mozilla.com-f8961dc6cb35/

These installer builds appear to work well here on Windows 8.1, no more firewall prompts.
Kamil, could you test installing this build on WinXP SP2 system?

https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/rstrong@mozilla.com-f8961dc6cb35/

It should add an inbound TCP and UDP entry for the install for the Public network and if present the Private network.
Flags: needinfo?(kamiljoz)
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.
Attachment #8535437 - Flags: review?(netzen)
Attachment #8535444 - Flags: review?(netzen)
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.
Attachment #8535443 - Flags: review?(netzen)
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.
Posted image 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!
Flags: needinfo?(kamiljoz)
Posted patch Client patchSplinter Review
Attachment #8535437 - Attachment is obsolete: true
Attachment #8535443 - Attachment is obsolete: true
Attachment #8535444 - Attachment is obsolete: true
Attachment #8535447 - Attachment is obsolete: true
Attachment #8539108 - Flags: review?(netzen)
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.
Attachment #8539112 - Flags: review?(netzen)
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.
Attachment #8539112 - Flags: review?(netzen) → review+
Attachment #8539108 - Flags: review?(netzen) → review+
Attachment #8539109 - Flags: review?(netzen) → review+
Made changes in comment #19. Carrying forward r+
Attachment #8539112 - Attachment is obsolete: true
Attachment #8541788 - Flags: review+
Note: tested this works with a clean WinXP SP2 system.
https://hg.mozilla.org/mozilla-central/rev/cd3e066846f1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Attachment #8539108 - Flags: approval-mozilla-beta?
Attachment #8539108 - Flags: approval-mozilla-aurora?
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.
Brad and Karen can provide more info as to why this is needed
Flags: needinfo?(krudnitski)
Flags: needinfo?(blassey.bugs)
This prevents getting a firewall error when doing an SDP search
Flags: needinfo?(blassey.bugs)
Attachment #8539108 - Flags: approval-mozilla-beta?
Attachment #8539108 - Flags: approval-mozilla-beta+
Attachment #8539108 - Flags: approval-mozilla-aurora?
Attachment #8539108 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.