Most connections get ignored in the alpha

[UltimateEvil]

The rules are not getting matched properly. First and foremost, there is an error when some of the parameters - namely the service name - is left as null and not defaulted to an empty string. That is an easy fix though.

2023-04-02 00:59:49,897 INFO  [  8] - Out-going connection for 'setup.exe' is blocked by a rule - ignored.
2023-04-02 00:59:49,898 INFO  [  8] - Handle Out-going connection for 'setup.exe', service: - ...
2023-04-02 00:59:52,926 ERROR [  8] - Unable to add the connection to the pool.
System.ArgumentNullException: Value cannot be null. (Parameter 'source')
   at System.Linq.ThrowHelper.ThrowArgumentNullException(ExceptionArgument argument)
   at System.Linq.Enumerable.Any[TSource](IEnumerable`1 source)
   at Wokhan.WindowsFirewallNotifier.Common.Net.WFP.Rules.Rule.Matches(String path, String service, Int32 protocol, String localport, String target, String remoteport, String appPkgId, String LocalUserOwner, Int32 currentProfile) in D:\a\WFN\WFN\Common\Net\WFP\Rules\Rule.cs:line 207
   at Wokhan.WindowsFirewallNotifier.Common.Net.WFP.FirewallHelper.<>c__DisplayClass13_0.<GetMatchingRules>b__0(Rule r) in D:\a\WFN\WFN\Common\Net\WFP\FirewallHelper.cs:line 186
   at System.Linq.Utilities.<>c__DisplayClass1_0`1.<CombinePredicates>b__0(TSource x)
   at System.Linq.Utilities.<>c__DisplayClass1_0`1.<CombinePredicates>b__0(TSource x)
   at System.Linq.Enumerable.WhereArrayIterator`1.MoveNext()
   at System.Linq.Enumerable.Any[TSource](IEnumerable`1 source)
   at Wokhan.WindowsFirewallNotifier.Notifier.App.AddItem(CurrentConn conn) in D:\a\WFN\WFN\Notifier\App.cs:line 247

But even then the rules don't quite work the way they should. When the following logger is added into the block where we have any matching rule:

                    blockingRules.All(r => {//no need to check more, the first is sufficient
                        
                        LogHelper.Debug($"enabled {r.Enabled}");
                        LogHelper.Debug($"{r.Profiles} <--> {FirewallHelper.GetCurrentProfile()} {(((r.Profiles & FirewallHelper.GetCurrentProfile()) != 0) || ((r.Profiles & (int)NET_FW_PROFILE_TYPE2_.NET_FW_PROFILE2_ALL) != 0))}");
                        LogHelper.Debug($"{r.ApplicationName} <-->  {conn.Path}   {true && (String.IsNullOrEmpty(r.ApplicationName) || StringComparer.CurrentCultureIgnoreCase.Compare(r.ApplicationName, conn.Path) == 0)}");
                        LogHelper.Debug($"{r.ServiceName}  <-->  { String.Join(",", conn.ServiceName)}    {true && ((String.IsNullOrEmpty(r.ServiceName) || (r.ServiceName ?? "").Any() && conn.ServiceName == "*" || StringComparer.CurrentCultureIgnoreCase.Equals(r.ServiceName, conn.ServiceName)))}");
                        LogHelper.Debug($"{r.Protocol.ToString()}  <-->  {conn.Protocol} {true && (r.Protocol == Protocol.ANY || conn.RawProtocol == r.Protocol)}");
                        LogHelper.Debug($"{r.RemoteAddresses}  <-->  {conn.TargetIP} {true && Common.Net.WFP.Rules.Rule.CheckRuleAddresses(r.RemoteAddresses, conn.TargetIP ?? "")}");
                        LogHelper.Debug($"{r.RemotePorts}  <-->  {conn.TargetPort} {true && Common.Net.WFP.Rules.Rule.CheckRulePorts(r.RemotePorts, conn.TargetPort ?? "")}");
                        LogHelper.Debug($"{r.LocalPorts}  <-->  {conn.SourcePort} {true && Common.Net.WFP.Rules.Rule.CheckRulePorts(r.LocalPorts, conn.SourcePort)}");
                        LogHelper.Debug($"{r.AppPkgId}  <-->  {conn.CurrentAppPkgId} {true && (String.IsNullOrEmpty(r.AppPkgId) || (r.AppPkgId == conn.CurrentAppPkgId))}");
                        LogHelper.Debug($"{r.LUOwn}  <-->  { conn.CurrentLocalUserOwner} {true && (String.IsNullOrEmpty(r.LUOwn) || (r.LUOwn == conn.CurrentLocalUserOwner))}");

                        return false; });

It is clear unrelated rules pass through. Such as this one. And yet it gets logged anyhow
C:\program files\oracle\virtualbox\virtualbox.exe <--> C:\windows\system32\wermgr.exe False

Writing the condition inside the Matches function out into individual IFs solves the issue, however I do not know why it did not work with the nested ands

[wokhan]

Thanks! From the code, if service (or servicename) is null, it shouldn't fail nor raise any exception (since it's checked first).
What makes you think it could be the root cause for this issue?
I have to stop for now (maybe even for today), I'll try to repro asap, but if you have more information on that one (even if you provided a lot already) it would be really helpful!

[UltimateEvil]

I am not sure we are both refering to the same variable.

The one I am refering to:

internal bool AddItem(CurrentConn conn)

...
FirewallHelper.GetMatchingRules(conn.Path, conn.CurrentAppPkgId, conn.RawProtocol, conn.TargetIP, conn.TargetPort, conn.SourcePort, 

conn.ServiceName , <------ can be null - VS also tells you about this. Changing to conn.ServiceName ?? "" is sufficient

conn.CurrentLocalUserOwner, blockOnly: true, outgoingOnly: true);

It does not actually get checked later in the Matches function. The function does a check like this

(string.IsNullOrEmpty(ServiceName) || service.Any() && ServiceName == "*" || StringComparer.CurrentCultureIgnoreCase.Equals(ServiceName, service))

But since service is null, the .any() check gives the strange error.

Aside from that the issue seems to be much more illusive. Try the following implementation of the match check

GetMatchingRules(...)
....
        IEnumerable<Rule> ret = GetRules().Where(r => {
            bool match = r.Matches(path, service, protocol, localPort, target, targetPort, appPkgId, localUserOwner, currentProfile);
            if (match) {
                if (!r.Matches(path, service, protocol, localPort, target, targetPort, appPkgId, localUserOwner, currentProfile))
                {
                    LogHelper.Debug("HELP");
                    return false;
                }
            }
            else
                LogHelper.Debug($"!!!unmatched {r} {r.Name}");
            return match;
            });
...

What exactly is the Rule object? I am not familiar with C# enough to knwo if the marshalling and all that other stuff ends up injectiong some sort of crazy lazy-loading or something along those lines.

[wokhan]

You're right I wasn't checking the right part and it doesn't make sense, I don't even get why it was working before when "service" itself is null (btw checking if the string is empty with Any() isn't the best way semantically as I even thought it was an array when re-reading that code...).
Checking that maybe tonight or tomorrow night, depending on my personal (read family-related) "constraints" 😁
Thanks

[wokhan]

I forgot to answer the last part of your post: the Rule object is a custom WFN object serving as a base for all firewall rules (either custom or system ones). So it's a plain simple object to store everything related to the rules, and also comes with this method to check if a connection actually matches a rule. There is no lazy loading in that one except for the icon which gets resolved only when called first.

Note: the code you pointed must have been failing since 2020 on a regular basis when a rule was created for a specific service (and it rings a bell, I think there is another issue about this which I never took time to investigate further)...

[wokhan]

Was wrongly automatically closed when committing. Still working on it.