evilsocket / opensnitch

OpenSnitch is a GNU/Linux interactive application firewall inspired by Little Snitch.
GNU General Public License v3.0
10.96k stars 511 forks source link

OpenSnitch lies about what rule caused packets to be approved #1140

Closed redanaheim closed 5 months ago

redanaheim commented 6 months ago

Describe the bug

The OpenSnitch UI statistics panel is lying about what rule caused connections to be allowed. This also has the catastrophic effect that nothing at all is blocked or prompted.

  OpenSnitch version - 1.6.5.1
  OS: NixOS
  Version: 24.11.20240524 Vicuña
  Window Manager: GNOME shell
  Kernel version: Linux asahimbp 6.8.9-asahi #1-NixOS SMP PREEMPT_DYNAMIC Tue Jan 1 00:00:00 UTC 1980 aarch64 GNU/Linux

Steps to reproduce the behavior:

  1. Enable OpenSnitch using the equivalent of the following configuration:

NixOS module:

{
  pkgs,
  lib,
  config,
  ...
}:
# TODO: Find out what is causing every connection to be accepted on account of the last rule?
let
  settings = {
    Server = {
      Address = "unix:///tmp/osui.sock";
    };
    # Unnecessary as we are not modifying the system firewall
    # FwOptions.ConfigPath = "/etc/opensnitchd/system-fw_alt.json";
    # TODO: Fix this. We should be able to use eBPF. This may require a kernel PR
    # https://github.com/evilsocket/opensnitch/issues/1138
    # ProcMonitorMethod = "ebpf";
    Ebpf.ModulesPath = "${config.boot.kernelPackages.opensnitch-ebpf}/etc/opensnitchd";
    ProcMonitorMethod = "proc";
    Rules.Path = "/var/lib/opensnitch/rules";
    InterceptUnknown = true;
  };
in {
  # Kernel module necessary for OpenSnitch to pick up wireguard connections
  # and assign them to the correct process
  # TODO: Fix this (uncomment this line)
  # boot.extraModulePackages = [config.boot.kernelPackages.opensnitch-ebpf];
  # List of options required to enable: https://github.com/evilsocket/opensnitch/issues/774
  boot.kernelPatches = [
    {
      name = "opensnitch_enable";
      patch = null;
      extraStructuredConfig = with lib.kernel; {
        FTRACE = yes;
        KPROBES = yes;
        HAVE_KPROBES = yes;
        # TODO: Fix this
        # Unfortunately not supported yet on arm64
        # https://github.com/torvalds/linux/blob/master/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
        KPROBES_ON_FTRACE = yes;
        HAVE_KPROBES_ON_FTRACE = yes;
        # end of unsupported options
        KPROBE_EVENTS = yes;
        HAVE_SYSCALL_TRACEPOINTS = yes;
        FTRACE_SYSCALLS = yes;
        UPROBE_EVENTS = yes;
        INET_DIAG = yes;
        INET_TCP_DIAG = yes;
        INET_UDP_DIAG = yes;
        INET_DIAG_DESTROY = yes;
      };
    }
  ];

  services.opensnitch = {
    inherit settings;
    enable = true;
    rules = (import ./rules.nix) {inherit pkgs lib;};
  };

  systemd.services.opensnitchd.serviceConfig.ExecStart = let
    format = pkgs.formats.json {};
    default_settings = builtins.fromJSON (builtins.unsafeDiscardStringContext (builtins.readFile "${pkgs.opensnitch}/etc/opensnitchd/default-config.json"));
    config_file = format.generate "default-config.json" (default_settings // settings);
  in [
    ""
    "${pkgs.opensnitch}/bin/opensnitchd --config-file ${config_file}"
  ];

  # Not necessary because we do not need to modify the system firewall
  # environment.etc."opensnitchd/system-fw_alt.json" = {
  #   text = builtins.toJSON (import ./system_fw.nix {});
  #   mode = "0440";
  # };
}

That is, InterceptUnknown as true, ProcMonitorMethod as "proc", and everything else set to the default. Also, place the following set of rules in /var/lib/opensnitch/rules to replicate the rules generated by Nix: (directly exported from OpenSnitch)

vscodium_openvsx.json vscodium_github.json systemd-timesyncd_all.json systemd-resolved_all.json openvpn_all.json mullvad_all.json git-remote-http_github.json firefox_all.json evolution-data-server_umich-instructure.json evolution-data-server_google-calendar.json dhcpcd_all.json avahi-daemon_all.json

  1. Start the OpenSnitch daemon and UI.

  2. Turn off all VPNs.

  3. ping google.com. Notice that instead of prompting you to allow or deny the connection, OpenSnitch allows it and for some reason attributes it to the last alphabetical rule, vscodium_openvsx.

Expected behavior (optional)

OpenSnitch prompts me to allow or deny the connection or correctly labels it according to a rule that actually applies.

Screenshots

Attached below:

![Captura desde 2024-05-27 12-44-57](https://github.com/evilsocket/opensnitch/assets/54746609/b7f0a0e2-2a2e-4e3b-aac5-bfdac2d62548) ![Captura desde 2024-05-27 12-44-48](https://github.com/evilsocket/opensnitch/assets/54746609/251acfd3-cc26-4d05-a28c-39ce100c0871) ![Captura desde 2024-05-27 12-44-15](https://github.com/evilsocket/opensnitch/assets/54746609/605bab57-7705-42fc-bc17-72f7d3e96559) ![Captura desde 2024-05-27 12-43-41](https://github.com/evilsocket/opensnitch/assets/54746609/38a74c97-4620-42a4-b320-77f4c1877b6e)
gustavo-iniguez-goya commented 6 months ago

Hi @redanaheim ,

Some questions/suggestions:

Could you change the LogLevel to DEBUG, reproduce it and post the logs? It'd be worth testing it also with telnet, curl or wget.
See if ICMP is allowed globally under Rules tab -> System rules. If it is, disable the ICMP rules. Modify the rule systemd-resolved_all.json, and mark [x] To this port: 53, to restrict DNS queries to this port.

By the way, silly question, but the daemon you're running is without modifications, right? compiled from latest sources.

I've tried to reproduce it but it works as expected.

("lying" is maybe a strong word here :) opensnitch is not doing this on purpose, so if anything, it's not reporting the rule correctly, or allowing something that shouldn't.)

redanaheim commented 6 months ago

@gustavo-iniguez-goya

Could you change the LogLevel to DEBUG, reproduce it and post the logs?

Yes. Here's the log file:

log.txt

It'd be worth testing it also with telnet, curl or wget.

The bug occurs with any application whatsoever (you can see some others in the log). I did test it with those, however, and the same thing occurs.

See if ICMP is allowed globally under Rules tab -> System rules. If it is, disable the ICMP rules.

It was before, but I disabled it and the same issue occurs.

Modify the rule systemd-resolved_all.json, and mark [x] To this port: 53, to restrict DNS queries to this port.

Done.

By the way, silly question, but the daemon you're running is without modifications, right? compiled from latest sources.

I realized it was not before, but I rebuilt from latest sources and the issue still appears the exact same way.

I've tried to reproduce it but it works as expected.

I will try to get a minimal NixOS VM going to reproduce the issue.

redanaheim commented 6 months ago

@gustavo-iniguez-goya : I created a minimal vm that reproduces the issue.

https://github.com/redanaheim/opensnitch_nixos_vm

If you want to see for yourself:

  1. Clone
  2. Make sure you have nix installed
  3. Run build.sh. Once it's done, run the executable in result/bin. No root privileges required.
  4. "vm" user password is "vm". Opensnitch-ui automatically starts up and the exact same thing that occurs on my machine occurs.
redanaheim commented 6 months ago

Also, if qemu aarch64 is too slow, you should be able to just find and replace with x86_64 to build the exact same VM for x86_64. Nix is awesome!

gustavo-iniguez-goya commented 6 months ago

thank you @redanaheim , I've reproduced the issue.

Did you create the rule from the GUI or manually?

On the one hand, the vscodium_openvsx.json's "created" field is invalid, instead of a timestamp the expected value is a date, for example: "2024-05-30T01:18:24.270461895+02:00" . The rule fails to load: "Error parsing rule..."

Take a look at this example: https://github.com/evilsocket/opensnitch/wiki/Rules#format

On the other hand, the "data" field containing the plain json is only meant to be used as a temporary value between the daemon and the GUI. When the daemon receives a rule from the GUI, it expands that flat json to a structured json, but when loading a rule it doesn't parse that field.

This behaviour was changed on v1.7.0 here b93051026e6a82ba07a5ac2f072880e69f04c238 , but it has not been ported to v1.6.x.

So you have 2 options:

  "operator": {
    "operand": "list",
    "data": "[{\"type\": \"simple\", \"operand\": \"process.path\", \"data\": \"/nix/store/iy0wwrvrysml9x8m916dnnrlzg0w62ha-vscodium-1.88.1.24104/lib/vscode/codium\", \"sensitive\": false}, {\"type\": \"simple\", \"operand\": \"dest.host\", \"data\": \"open-vsx.org\", \"sensitive\": false}]",
    "type": "list",
    "list": [
      {
        "operand": "process.path",
        "data": "/nix/store/iy0wwrvrysml9x8m916dnnrlzg0w62ha-vscodium-1.88.1.24104/lib/vscode/codium",
        "type": "simple",
        "list": null,
        "sensitive": false
      },
      {
        "operand": "dest.host",
        "data": "open-vsx.org",
        "type": "simple",
        "list": null,
        "sensitive": false
      }
    ],
    "sensitive": false
  },
redanaheim commented 6 months ago

@gustavo-iniguez-goya thanks!

The rules are generated into the rules directory by nix expressions so I can easily modify them to include the additional structured JSON field.

However, I still think the behavior here is issue-worthy. Presumably when a rule fails to parse, it should not behave this way and instead should not use the rule at all, right?

Also, is there any reason why the client is designed to create the data field instead of creating the structured form directly?

If there is, maybe additional documentation explaining how that works is a good idea, because the reason I had Nix generate the data field that way is because (iirc) copying the rule to clipboard yielded that form for list operators, so it's a little confusing that it doesn't work if you copy what the client gives you directly. I could PR that if you wanted.

redanaheim commented 6 months ago

Oh wait, I now see the commit and issue addressing the string thing. Ignore that part of my comment since it's fixed.

gustavo-iniguez-goya commented 5 months ago

Presumably when a rule fails to parse, it should not behave this way and instead should not use the rule at all, right?

Yeah, if the rule fails to load then it's not added to the list of rules. It's not used.

However if the "created" field is formatted correctly, but the rule has no Operators, then the rule is an empty rule. It could be used to log everything that is not explicitely filtered, applying the defined action. A similar behaviour can also be accomplished, by selecting [x] Disable popups from the preferences. see? it's not a bug, it's a feature! ;)

From the GUI you can't create a rule without Operators, but for the daemon is a valid rule. I opted for this behaviour a long time ago on purpose, if I remember correctly, to avoid "empty" rules created by accident.

But there's more. The confusing behaviour is (v1.7.0):

On the other hand, the option "Export to clipboard" writes a generic datetime, which json.Unmarshall() doesn't validate (because the reason explained above).

I'll push a fix for the latter, and I think I'll export the rules always as RFC3339, instead of timestamp.

~I'll review the behaviour on v1.6.x.~ The behaviour on v1.6.x is the same.

evilsocket commented 5 months ago

very dramatic title, much wow.