wixtoolset / issues

WiX Toolset Issues Tracker
http://wixtoolset.org/
129 stars 36 forks source link

Firewall Extension does not handle port or protocol changes across change and repair #5869

Open dyhims opened 6 years ago

dyhims commented 6 years ago

The wix firewall extension does not work correctly across change/repair, and is likely broken in some upgrade scenarios as well. Port and protocol do not update when new values are passed to the custom action. Essentially I have the issue described here: https://github.com/wixtoolset/issues/issues/5675

Bugs

If this issue is a bug:

3.10.3007.0

VS 2017 15.8.1

0.9.21.62588

4.5.2

Windows 10 (10.0.17134)

If the only change in a firewall exception is the port (for example), it does not update during change or repair. The old port stays open on the system.

Add a firewall exception, to your wix source, e.g.

<Component Id="OpenFirewallPort" Guid="*" KeyPath="yes">
        <fw:FirewallException Id="FwOpenPort"
                              Description="!(loc.FirewallRuleDescription)"
                              Name="InboundOpenException"
                              Port="[MY_PORT]"
                              Scope="any"  />
</Component>
MSI (s) (7C:58) [13:48:43:169]: Executing op: CustomActionSchedule(Action=WixExecFirewallExceptionsInstall,ActionType=3073,Source=BinaryData,Target=ExecFirewallExceptions,CustomActionData=3 SmeInboundOpenException 2147483647 * 0 1 6515 6 Windows Admin Center inbound port exception)
----
ExecFirewallExceptions:  Installing firewall exception2 SmeInboundOpenException on port 6515, protocol 6

See the comment on this issue: https://github.com/wixtoolset/issues/issues/5675#issuecomment-391358971. The code only re-enables a disabled rule, it does not perform other updates.

Expected: The port is updated to match the supplied number when the action runs. Actual: The old parameters for port and protocol remain installed.

chrpai commented 5 years ago

Should it? I would think it's the job of the MSI author to implement a remember me property type pattern. I don't expect other Windows Installer tables to automatically remember properties.

dyhims commented 5 years ago

@chrpai This is not about remembering the old property, this is about honoring the values that are passed in when the component is reinstalled (for example).

Under the current implementation, if do a clean install with MY_PORT=443, I get a firewall rule called "MyRule" for tcp/443. If I then run repair with MY_PORT=5000, the component will say that it is installing with the new port value, but the firewall rule will not be changed. Port 443 will still be open, 5000 will be blocked.

In my own project I ended up supporting the behavior we needed entirely through custom actions.

chrpai commented 5 years ago

Ah, thanks for the clarification. Can you put a little more context around this for me? What if you install MY_PORT=443, manually change the port to 442 and then do a repair with MY_PORT=5000. Does it still go back to 443 or does it stay at 442?

dyhims commented 5 years ago

I believe (but haven't executed the code!) that it will keep 442 open.

The underlying problem is that the firewall API only does lookup by the rule name. I appreciate that there are probably backward compatibility concerns with what I'm asking for here. If I understand the code correctly then it seems like nothing will get installed if there is a name collision, even with a clean install.

mjaepel commented 5 years ago

Scope of firewall rules is also affected. It occurs also if the rule already exists before MSI package is installed.

For example: Some one create manually a firewall rule "Rule A" with scope/remote-Adress 127.0.0.2 and install a MSI Package that should create also a firewall rule "Rule A" but with scope/remote-Adress 127.0.0.1. Then the rule will keep 127.0.0.2. If you rename the existing rule to "Rule B" and start a repair installation of MSI package, than there will be 2 rules: Rule B - 127.0.0.2 Rule A - 127.0.0.1

So it seems that there is a generally issue with overwriting of existing rules by msi packages. Not sure if this an issue with WiX or with Implementation in Windows.

fareed-ali commented 4 years ago

Hi, I'm using wix v 3.11.1.2318. But this issue still persists. Can anyone please update me that how should I proceed if I want the installer to add a new firewall rule and remove the last one in case of upgrade/update.

EpicScizor commented 2 years ago

I have also encountered this behaviour. The problem lies in this section of Firewall.cpp. and the similar sections for the other AddException methods in that file. It only changes the "Enabled" state of the rule, not any of the other properties.

Suggested behaviour would be to also change the rest of the properties of that rule, by using INetFwRule interface methods with put_RemoteAddresses and the like.

A more complicated case is if there are multiple rules with the same name, which is allowed by using the netsh advfirewall command line tool, at least (if I copy-paste the same "add rule" command twice, I end up with two rules with the same name. I am unsure whether or how these rules are uniquely identified by the INetFw API, and what that implies for how WixFirewallExtension should behave.

barnson commented 2 years ago

As you can see from the code, the current behavior is intentional. This issue is open to implement an alternate mode. So far, nobody's implemented it.

EpicScizor commented 2 years ago

This means both repair, modify and upgrade functionality will not work as expected by default (it will not, in fact, fix a misconfigured rule, or create a rule that has the exact settings supplied - it may in fact enable a completely different rule to what we would expect)

I can implement the code to apply new properties - which would simply be adding a few more API calls in the right places to the methods in Firewall.cpp, since the information is there already - but if it requires a new alternate mode of behaviour specified in the FirewallException element by the user, then I do not know WiX's code-base well enough to implement that.

barnson commented 2 years ago

The current behavior allows an admin to tweak the rule and not have the installer remove the tweaks. Again, it was intentional behavior. I'm not opposed to a repair having the (optional) effect of updating or adding a rule.