gustavo-iniguez-goya / opensnitch

OpenSnitch is a GNU/Linux application firewall
GNU General Public License v3.0
395 stars 20 forks source link

Precedence Of Rules? #36

Closed metal450 closed 4 years ago

metal450 commented 4 years ago

Question: what is the order/precedence in which rules are applied?

Real world example:

A) I have a rule "always allow to destination IP 127.0.0.1" - I want everything to be able to talk to localhost (as not being able to can break some applications). B) I have another rule to "always block a given command-line" - I want to prevent a give application from talking to the internet.

If the rules are applied A, B, then the app will be able to talk to localhost, but nothing else. If applied B,A, it won't be able to talk to anything.

In particular: how can the order be controlled - as the first scenario is the one I'm trying to achieve ("allow to talk to localhost" should supercede all other rules)?

gustavo-iniguez-goya commented 4 years ago

I don't know very well this part of the code, but the logic seems to be as follow:

So, in your case, a packet sent to localhost will be allowed, but if there's a DENY rule that matches that packet (a blocked app for example), it'll be blocked.

Try adding the rule to allow everything to localhost, and another one blocking eveyrthing from your command with this destination IP: [^:127\.0\.0\.1:]

It blocks packets to everything except 127.0.0.1, but some connections doesn't match. I'll investigate why.

metal450 commented 4 years ago

So based on your understanding, it seems like the precedence can be summarized as simply as: "Deny rules always take precedence." Right? Because if there's any "deny" rule, it'd always be blocked, regardless of if there are "accept" rules before or after.

If that's the case (?), then I might make a request: some way to accomplish the above - the ability to have an "allow" rule supercede all others. Thus with one rule I could i.e. allow everything to communcate to local host, on my LAN, or i.e. to my own webserver - without needing additional "not" exceptional rules in each & every other rule. Does that make sense? :)

gustavo-iniguez-goya commented 4 years ago

Yes, I think it makes a lot of sense, a feature to prioritize some rules.

For now / If you want to get better performance, it'd be better to add a rule directly to iptables: iptables -t mangle -I OUTPUT -d 127.0.0.1 -j ACCEPT

That way you avoid the overhead of iterate all the rules. And just add the rule to block everything from the app.

I'll give it a thought.

metal450 commented 4 years ago

Perfect, thanks! Will that survive reboots & be permanent? (& I can therefore get rid of the 'allow all' rule I'd added, which would still successfully apply to other apps that only ever try to access localhost)?

gustavo-iniguez-goya commented 4 years ago

No, that iptables rule is temporal. You can use ufw, iptables-persistent or a similar software to save and restore rules after restart. Then yes, if you add that rule you can get rid of the allow-all-to-localhost rule.

metal450 commented 4 years ago

Great, thanks - will give it a go (if it becomes an issue again before there's a possibility to have a "high priority rule" :)

gustavo-iniguez-goya commented 4 years ago

hey @metal450 , check out this branch if you have time https://github.com/gustavo-iniguez-goya/opensnitch/tree/priority-rules, I added the option to prioritize rules, which bypasses connections interception. You could simply add under "allow" -> "-d 127.0.0.1" (more details here)

It's not exactly what you requested, but as there're others scenarios where it might be useful, maybe it'll work as well in this case.

metal450 commented 4 years ago

Cool! Unfortunately, I'm a little slammed at the moment (& have honestly only installed from debs thus far, not from source), so I think I'll need to hold off if or until it's in the official build...then I'd love to restructure my rules to use this & get rid of a bit of the duplication. But I don't really think I can be of much help as a tester at THIS particular moment. Thanks for the ping tho! :)

gustavo-iniguez-goya commented 4 years ago

don't worry! I'll generate a deb of that branch in a few hours, just in case you want to test it.

metal450 commented 4 years ago

Cool, that'll make it more doable :)

gustavo-iniguez-goya commented 4 years ago

Copy the fw.json file to /etc/opensnitchd/, it has some examples.

You can check if the rules have been loaded with iptables -t mangle -L, a new opensnitch-priority-rules chain should appear.

Whenever you modify the fw.json file, the rules be reloaded.

fw.json.gz

opensnitch_1.0.0priorules-1_amd64.deb.gz

gustavo-iniguez-goya commented 4 years ago

I don't know very well this part of the code, but the logic seems to be as follow: iterate over all the rules.

Some clarification regarding this request.

The order of rules is not guaranteed to be the same between iterations, so naming the rules alphabetically (000-allow-xxx, 001-deny-nnn) does not assure that the rules will be checked in that order. I've got a solution for this, so the Deny rules can be prioritize.

On the other hand, if there's an Allow rule that matches a connection, we keep iterating until a Deny rule matches and breaks the loop. In this case, we should have a flag to indicate that the Allow rule is prioritary and that the connection must be allowed without checking the rest of the rules. TODO.

Both cases would help to improve the performance of rules checking.

Dretch commented 3 years ago

I have not been able to get the priority rules to work as I expect them to.

I am looking to do something like:

It just doesn't seem to work.

Perhaps this is because I am not familiar with Go, but the priority rule matching code does not make sense to me.

Consider: https://github.com/gustavo-iniguez-goya/opensnitch/blob/main/daemon/rule/loader.go#L296

This (I think!) says if there was previous rule that matched, and this rule is an allow rule with the priority flag set, then (regardless of whether this rule matches!), return the previous rule as a match. I don't understand how this could implement the desired behaviour, so I am wondering if there is a bug here.

gustavo-iniguez-goya commented 3 years ago

Hi @Dretch ! what's the name of your rule? Try it naming the priotiry rule: 000-allow-firefox, [x] Priority rule

Right now (>= 1.2.0) the behaviour is as follow:

Dretch commented 3 years ago

Hi @gustavo-iniguez-goya , sorry for the late reply.

My test rules are indeed named such that the allow rule comes first. The behaviour you documented makes sense, but I was not able to reproduce it myself, and the code in this method (to my understanding) does not actually implement that algorithm: https://github.com/gustavo-iniguez-goya/opensnitch/blob/main/daemon/rule/loader.go#L282

gustavo-iniguez-goya commented 3 years ago

Can you post an example of a rule that is not prioritized?

Dretch commented 3 years ago

Here is an example.

The allow rule comes first and has precedence = true. The deny rule comes second and has precedence = false. The first rule should be applied, but actually the second rule is applied.

000-allow-firefox.json

{
  "created": "2020-12-04T20:48:01.187041533Z",
  "updated": "2020-12-04T20:48:01.187101518Z",
  "name": "000-allow-firefox",
  "enabled": true,
  "precedence": true,
  "action": "allow",
  "duration": "always",
  "operator": {
    "type": "simple",
    "operand": "process.path",
    "sensitive": false,
    "data": "/usr/lib/firefox/firefox",
    "list": []
  }
}

001-deny-firefox.json

{
  "created": "2020-12-04T20:45:15.041604408Z",
  "updated": "2020-12-04T20:45:15.041684698Z",
  "name": "001-deny-firefox",
  "enabled": true,
  "precedence": false,
  "action": "deny",
  "duration": "always",
  "operator": {
    "type": "simple",
    "operand": "process.path",
    "sensitive": false,
    "data": "/usr/lib/firefox/firefox",
    "list": []
  }
}

In practice the allow rule has some conditions attached, but these are not necessary to demonstrate the behaviour.

gustavo-iniguez-goya commented 3 years ago

Sorry @Dretch , I can't reproduce this behaviour. With both rules only the allow rule is applied: image

image

The allow rule has matched several connections: image

while the deny rule is empty: image

I really have no idea why is not working for you. :/

Dretch commented 3 years ago

@gustavo-iniguez-goya of course I am seeing the opposite to you: all my connections go into my deny rule :smile:

The only explanation I have is that you have additional rules in your configuration, and in particular the rule that follows 000-allow-chrome for you is another allow rule with the precedence flag set. This obviously shouldn't make a difference, but according to the code here then it will do: https://github.com/gustavo-iniguez-goya/opensnitch/blob/main/daemon/rule/loader.go#L282 (and I have also just verified it locally for me).

gustavo-iniguez-goya commented 3 years ago

Well, finally I've managed to reproduce this behaviour.

2 rules only: 000-allow-chrome, 001-deny-chrome

I had configured the pop-ups Default Action in the GUI to Allow (daemon default action Deny). Both rules 000-allow and 001-deny were behaving correctly. However, after changing the pop-ups Default Action to Deny, the rule 001-deny started taking precedence.

FindFirstMatch() start-----------
rule:  000-allow-chrome Action: allow Priority: true
match?  <nil>
 ! MATCH !  000-allow-chrome
rule:  001-deny-chrome Action: deny Priority: false
match?  000-allow-chrome: if(process.path is '/opt/google/chrome/chrome'){ allow always }
 ! MATCH !  001-deny-chrome
 >> DENY  001-deny-chrome
FindFirstMatch() end------------

Anyway, that code snippet has always looked a bit odd to me; replacing it by this seems to work as expected:

  func (l *Loader) FindFirstMatch(con *conman.Connection) (match *Rule) {
      l.RLock()
      defer l.RUnlock()

      for _, ruleIdx := range l.rulesKeys {
          rule, _ := l.rules[ruleIdx]

          match  = rule
          if rule.Match(con) {
              if rule.Action == Deny || rule.Precedence == true {
                  return rule
              }
          }
      }

      return match
}

I'll test it for some time, and will start unit tests once and for all... O:)

themighty1 commented 3 years ago

@gustavo-iniguez-goya , good job. Talking about unit tests. I agree we need to have some tests at least to check if the rules are being conformed to.