new connections are blocked when popup is running

[themighty1]

From my experience, all the packets in the nfq are processed sequentially. Until a verdict is passed for a packet, the next one will not be processed.
So, opensnitchd's workers are not concurrent. If one of them blocks, then the whole queue will be blocked.
Just something to keep in mind when making future design decisions.

[gustavo-iniguez-goya]

all the packets in the nfq are processed sequentially

That's the real bottleneck. We're only using one NFQUEUE queue, so all the output packets pass through a single point. Obviously workers concurrency helps a bit, but we should have more queues as explained here:
https://home.regit.org/netfilter-en/using-nfqueue-and-libnetfilter_queue/

iptables -t mangle -I OUTPUT -j NFQUEUE --queue-balance 0:3

and suggested by the libnetfilter_queue docs:
https://netfilter.org/projects/libnetfilter_queue/doxygen/html/

see –queue-balance option in NFQUEUE target for multi-threaded apps (it requires Linux kernel >= 2.6.31).

[themighty1]

Thank you for the links.

My initial impression was that there was a mistaken assumption that the workers process the queue in parallel. I got this impression because right now when a new popup comes up asking to make a decision,
no new connections can go through until the decision on the popup is made, e.g. I cannot use the browser even.

Maybe we should move that connection which is awaiting the user decision onto a different queue, so that normal internet activity is not hindered?

[gustavo-iniguez-goya]

right now when a new popup comes up asking to make a decision, no new connections can go through until the decision on the popup is made,

Yes, that behaviour annoys me quite a bit, because with some VPN clients it interrupts the connection, usually disconnecting it.

Maybe we should move that connection which is awaiting the user decision onto a different queue, so that normal internet activity is not hindered?

I think it would be easier to modify the protocol, and make the query asynchronous:

ui.proto:
rpc AskRule (Connection) returns (Rule) {}
to
rpc AskRule (stream Connection) returns (Rule) {}

I made an attempt to change it a few weeks ago, but I haven't had time to look into it again.

[themighty1]

Correct me if Im wrong, but even if you make it async, you still have to do something with the queued packet in order not to hold up the queue.
I dont see how making the query async solves this problem.

[gustavo-iniguez-goya]

Yes, that's it. It's a different problem, the bottleneck is still the single NFQUEUE queue.

But making this call asynchronous would not stop queue processing, so we wouldn't need to move the connection to a different queue as you said before.

[themighty1]

Oh, let me see if I got it right this time.
You are implying that the packet which needs the decision from the user will have to be NF_DROPped at the time when the async call to the gui is made?

(nfq works in such a manner that unless we pass a verdict on the packet, the next packet will not get processed by nfq)

I'm wondering when we invoke the gui in an async manner and the onPacket() function runs to its end without returning any verdict on the packet, what will happen to the packet? I guess some netfilter exception will be raised because it requires a verdict to be passed on each packet.

[gustavo-iniguez-goya]

Yeah, sorry, it's a bit complicated topic, probably I'm not explaining it well:

You are implying that the packet which needs the decision from the user will have to be NF_DROPped at the time when the async call to the gui is made?

Yes, or the Default Action configured.

I'm wondering when we invoke the gui in an async manner and the onPacket() function runs to its end without returning any verdict on the packet, what will happen to the packet? I guess some netfilter exception will be raised because it requires a verdict to be passed on each packet.

We would process the packet applying the Default Action configured, but only to that packet (Default Action: once). If the user allows the packet and the app retries the connection, then it would apply the rule defined by the user.

If you hold up the connection until the connection is allowed or denied, you'll probably end up triggering the app timeout for that connection.

Right now the connection flow is as follow:

new outgoing connection -> iptables NFQUEUE
                                  |
                                queue 0
                                  |
                             opensnitchd
                                  |
                              onPacket()
                                  |
                            conman.Parse()
                                  |
                            acceptOrDeny()
                                  |
                       rule.FindFirstMatch() 
                                  |
                            rule NOT FOUND -> uiClient.Ask()
                                                      |
                                    **(wait user response or pop-up timeout
                                       blocking all traffic for 15s or more)**
                                                      |
                                                 rule received 
                                                 applyAction()
                                      _______ |
                                    /
                                  /
                                  |
                        NF_ACCEPT or NF_DROP
                                  |
                        keep processing packets

Invoking the GUI asynchronously would change the flow like this:

                      rule NOT FOUND -> uiClient -> Ask()
                            |
                      applyDefaultAction() - once
                            |
                  NF_ACCEPT or NF_DROP
                            |
                  keep processing packets

Anyway, this is something that the users asked for long time ago, be able to choose if display a connection pop-up or apply a default action and later edit the rule using the GUI.

[themighty1]

Now it makes sense, thanks.

[gustavo-iniguez-goya]

oh, I've just remembered what happened when I implemented this working method.

If the Default Duration is "once", the daemon sends the request to allow or deny the connection to the GUI. However, as soon as the packet is dropped (if that's the Default Action configured) and a new packet arrives (a connection retry), the daemon send the request again until the app give up.

If I'll look into this again I'll have it into account.

[gustavo-iniguez-goya]

Regarding the bottleneck of a single NFQUEUE queue, I've been playing a bit:

main.go

 func pktQueue(id int, pktQueue <-chan netfilter.Packet) {
      fmt.Println("setting up async queue ", id)
      for true {
          select {
          case <-ctx.Done():
              goto Exit
          default:
              pkt, ok := <-pktQueue
              if !ok {
                  goto Exit
              }
              fmt.Println("pkt.queue: ", id)
              wrkChan <- pkt
          }
      }
  Exit:
      fmt.Println("pktQueue exit: ", id)
  }

in main():

<<< pktChan = queue.Packets()

>>> go pktQueue(queueNum, queue.Packets())
>>> go pktQueue(queueNum+1, queue1.Packets())

          select {
          case <-ctx.Done():
              goto Exit
              /*case pkt, ok := <-pktChan:
                  fmt.Println("packet.queue0")
                  if !ok {
                      goto Exit
                  }
                  wrkChan <- pkt
              case pkt1, ok := <-pktChan1:
                  fmt.Println("packet.queue1")
                  if !ok {
                      goto Exit
                  }
                  wrkChan <- pkt1
              */

start opensnitch and add 2 iptables rules (UDP packets to queue 0 and TCP packets to queue 1):

iptables -t mangle -I OUTPUT -p udp -m conntrack --ctstate NEW,RELATED -j NFQUEUE --queue-num 0
iptables -t mangle -I OUTPUT -p tcp -m conntrack --ctstate NEW,RELATED -j NFQUEUE --queue-num 1

It seems to work, I haven't run it through the race detector though. Each packet type goes to its assigned queue.

However the --queue-balance doesn't seem to work:
iptables -t mangle -I OUTPUT -m conntrack --ctstate NEW,RELATED -j NFQUEUE --queue-balance 0:1 --queue-cpu-fanout

[gustavo-iniguez-goya]

I've pushed a branch with these changes just in case you want to give it a try:
https://github.com/gustavo-iniguez-goya/opensnitch/tree/feature-multiple-nfqueues

./opensnitchd -rules-path /etc/opensnitchd/rules/ --queue-numbers 10

I don't know if it makes any difference. Web navigation seems to be a bit more reactive and with 10 nfqueues I don't see the usual "unknown connections" using transmission.

Regarding the bottleneck of a single NFQUEUE queue, I've been playing a bit:

main.go

 func pktQueue(id int, pktQueue <-chan netfilter.Packet) {
      fmt.Println("setting up async queue ", id)
      for true {
          select {
          case <-ctx.Done():
              goto Exit
          default:
              pkt, ok := <-pktQueue
              if !ok {
                  goto Exit
              }
              fmt.Println("pkt.queue: ", id)
              wrkChan <- pkt
          }
      }
  Exit:
      fmt.Println("pktQueue exit: ", id)
  }

in main():

<<< pktChan = queue.Packets()

>>> go pktQueue(queueNum, queue.Packets())
>>> go pktQueue(queueNum+1, queue1.Packets())

          select {
          case <-ctx.Done():
              goto Exit
              /*case pkt, ok := <-pktChan:
                  fmt.Println("packet.queue0")
                  if !ok {
                      goto Exit
                  }
                  wrkChan <- pkt
              case pkt1, ok := <-pktChan1:
                  fmt.Println("packet.queue1")
                  if !ok {
                      goto Exit
                  }
                  wrkChan <- pkt1
              */

start opensnitch and add 2 iptables rules (UDP packets to queue 0 and TCP packets to queue 1):

iptables -t mangle -I OUTPUT -p udp -m conntrack --ctstate NEW,RELATED -j NFQUEUE --queue-num 0
iptables -t mangle -I OUTPUT -p tcp -m conntrack --ctstate NEW,RELATED -j NFQUEUE --queue-num 1

It seems to work, I haven't run it through the race detector though. Each packet type goes to its assigned queue.

However the --queue-balance doesn't seem to work:
iptables -t mangle -I OUTPUT -m conntrack --ctstate NEW,RELATED -j NFQUEUE --queue-balance 0:1 --queue-cpu-fanout

[themighty1]

Thank you for the branch. It was supposed to fix the unknown connections? I guess you posted it in the wrong issue.
Anyway, I ran it and looking at the debug console, I still see the same occasional unknown conections.

[gustavo-iniguez-goya]

we started talking about workers concurrency, but well, nevermind.

It was supposed to fix the unknown connections?
not specially. I guess that we should get more throughput. Specially if we would analyze traffic with Yara rules for example or any other type of packet inspection.

[themighty1]

@gustavo-iniguez-goya , do you have any code for Invoking the GUI asynchronously as discussed here.
I would like to implement it, unless you already have.
Like you said earlier:

rule NOT FOUND -> uiClient -> Ask()
                          |
                    applyDefaultAction() - once
                          |
                NF_ACCEPT or NF_DROP
                          |
                keep processing packets

If the popup is already running and a new allow/deny request arrives, the GUI should simply ignore it until the action on the currently displayed popup is taken.
Do you agree with this?

[gustavo-iniguez-goya]

do you have any code for Invoking the GUI asynchronously as discussed here.

nope, not anymore.

If the popup is already running and a new allow/deny request arrives, the GUI should simply ignore it until the action on the currently displayed popup is taken. Do you agree with this?

Do you mean discard the request on the GUI side? I think that it should be done on the daemon, otherwise the connections will still be blocked.

Maybe something like:

                         acceptOrDeny()
                                  |
                    alreadyAsking == false -> rule.FindFirstMatch() 
                                  |                 |
                                         rule NOT FOUND -> uiClient.Ask()
                                                                    |
                                                     **(wait user response or pop-up timeout
                                                            blocking all traffic for 15s or more)**
                                                                    |
                                  |                          rule received 
                   alreadyAsking == true                             /
                                 |                                 /
                                   \                             /
                                      \                       /
                                              applyAction()
                                                    |
                                           NF_ACCEPT or NF_DROP
                                                    |
                                       keep processing packets

If uiClient.Ask() is not asynchronous I think it still blocks the main thread. But I don't know for sure, implement it as best as you can and we keep working from there.

[themighty1]

@gustavo-iniguez-goya
Let me try to explain again what I had in mind.
The daemon will have no notion of whether the GUI is in popup mode or not.
The daemon will Ask() the GUI asynchronously and will apply the default action and carry on processing the packets.
The GUI will ignore all incoming allow/deny request if a popup is currently running.

Does this make sense?
Do you agree with this flow?

[gustavo-iniguez-goya]

The only problem I see is that we may end up spawning too many goroutines if there's no control on the daemon.
But let's start with something basic, and see how it works.