Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
redanaheim opened this issue May 27, 2024 · 9 comments
Closed

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

redanaheim opened this issue May 27, 2024 · 9 comments

Comments

@redanaheim
Copy link
Contributor

redanaheim commented May 27, 2024

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
Captura desde 2024-05-27 12-44-48
Captura desde 2024-05-27 12-44-15
Captura desde 2024-05-27 12-43-41

@gustavo-iniguez-goya
Copy link
Collaborator

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

redanaheim commented May 29, 2024

@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
Copy link
Contributor Author

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
Copy link
Collaborator

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 b930510 , but it has not been ported to v1.6.x.

So you have 2 options:

  • Edit the rule from the GUI and click Save (suggested). That will recreate the rule and should work as expected.
  • Or edit the rule manually, and create the "list" field correctly:
  "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
Copy link
Contributor Author

redanaheim commented May 30, 2024

@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
Copy link
Contributor Author

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
Copy link
Collaborator

gustavo-iniguez-goya commented May 30, 2024

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):

  • Rules exported from the GUI can only be imported from the GUI, i.e.: if you copy the exported rules to your /etc/opensnitchd/rules/ they won't load.
    The "created" field is exported as timestamp.
    When importing the rules protobuf.Parse() accepts the timestamp, we transform it to a datetime format, save it to the db and send it back to the daemon.
    The daemon receives the rule, compiles the Operators, the rule is loaded in memory and optionally written to disk.
    The created field is not parsed at this point, and when written to disk, json.Marshall() accepts the timestamp.
    However when the daemon loads a rule it uses json.Unmarshall() to parse the rule, and for time.Time fields, they must comply with the RFC3339 format https://pkg.go.dev/time#Time.UnmarshalJSON

    Summary: Unmarshall() does not accept timestamps, while Marshall() accepts datetimes or timestamps: https://go.dev/play/p/GYPXYV_UV6Y

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.

gustavo-iniguez-goya added a commit that referenced this issue May 30, 2024
We were not formatting the "created" date field properly.
More info: #1140 (comment)
@evilsocket
Copy link
Owner

very dramatic title, much wow.

gustavo-iniguez-goya added a commit that referenced this issue Jun 13, 2024
We were not formatting the "created" date field properly.
More info: #1140 (comment)

(cherry picked from commit b096e66)
gustavo-iniguez-goya added a commit that referenced this issue Jun 18, 2024
When exporting rules, use rfc3339 format for the Created field.
We were exporting as timestamp, which caused issues when importing them.

Related:
 58aa979
 issue #1140
gustavo-iniguez-goya added a commit that referenced this issue Jun 19, 2024
When exporting rules from the GUI, the Created field was exported as
timestamp. Importing rules worked fine, because json.Marshall() accepts
the timestamp format.

However, when the daemon was loading a rule with the Created field as
timestamp, since the field was defined as time.Time, it expected a RFC3339
string (https://pkg.go.dev/time#Time.UnmarshalJSON)
so it failed to parse the timestamp and the rule was not loaded.

Now the field is defined as string, it's always saved as RFC3339, and if
we fail to parse these fields we'll use a temporary date instead of
failing loading the rule.

More info:
#1140 (comment)

(cherry picked from commit 58aa979)
gustavo-iniguez-goya added a commit that referenced this issue Jun 19, 2024
When exporting rules, use rfc3339 format for the Created field.
We were exporting it as timestamp, which caused issues when
importing the rules.

Related:
 58aa979
 issue #1140
(cherry picked from commit 552aed5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants