Skip to content
New issue

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

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

Already on GitHub? # to your account

Add the "Prefix" operator for the "matchBinaries" selector #1278

Closed
2 tasks done
Vampouille opened this issue Jul 26, 2023 · 2 comments · Fixed by #1732
Closed
2 tasks done

Add the "Prefix" operator for the "matchBinaries" selector #1278

Vampouille opened this issue Jul 26, 2023 · 2 comments · Fixed by #1732
Assignees
Labels
kind/enhancement This improves or streamlines existing functionality

Comments

@Vampouille
Copy link
Contributor

Vampouille commented Jul 26, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem?

I would like to forbid network usage for every binaries in a specific folder. For example, binaries in ~/bin-no-network/ should not be able to create socket. This can be done with the following TracingPolicy:

apiVersion: cilium.io/v1alpha1
kind: TracingPolicy
metadata:
  name: "connect"
spec:
  kprobes:
  - call: "tcp_connect"
    syscall: false
    args:
    - index: 0
      type: "sock"
    selectors:
    - matchBinaries:
      - operator: "In"
        values:
          - "/home/jacroute/bin-no-network/curl"
          - "/home/jacroute/bin-no-network/jq"
      matchActions:
      - action: Sigkill

But we need to list every binary in this folder and keep this list up-to-date.

Describe the feature you would like

I can be nice to use the "Prefix" operator for this use-case but only "In" and "NotIn" operators are currently implemented

With this new feature, the TracingPolicy should look like :

apiVersion: cilium.io/v1alpha1
kind: TracingPolicy
metadata:
  name: "connect"
spec:
  kprobes:
  - call: "tcp_connect"
    syscall: false
    args:
    - index: 0
      type: "sock"
    selectors:
    - matchBinaries:
      - operator: "Prefix"
        values:
          - "/home/jacroute/bin-no-network/"
      matchActions:
      - action: Sigkill

Describe your proposed solution

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@Vampouille Vampouille added the kind/enhancement This improves or streamlines existing functionality label Jul 26, 2023
@ashishkurmi
Copy link
Contributor

ashishkurmi commented Aug 17, 2023

I would also request adding support for PostFix operator so that one can include a binary that could be present at multiple locations. For example:

  • /usr/local/bin/binary_to_include
  • /usr/bin/binary_to_include

@mtardy
Copy link
Member

mtardy commented Sep 15, 2023

So I spent some time on this in the light of #1408. I think this is doable against a bit of work. This PR by Anastasios explains how matchBinaries work in details #774 (comment).

But the tl;dr is that we need to retrieve the binary path at the exec stage and we need to transmit that information for later when the kprobe hits. As of now, we don't transmit the whole string of the binary from the execve stage to the kprobe matchBinaries but only an index because it's more efficient. This means that during the kprobe matchBinaries BPF code we only know yes or no if we matched but not really on what (since we don't have the string but only an ID).

Regarding #1408, prefix old implementation needed the string, and was slow because we parsed the whole potential strings byte by byte to see if we had a match. The new implementation uses LPM_TRIE which is optimized for this but needs to be populated at map load time in userspace. We could do that in a new implementation but we would need the value, again, at the kprobe matchBinaries to perform the lookup into the trie.

Here's what I proposed and may implement:

In a new design, we could keep the old implem which is very efficient for In and NotIn operators, but also transmit the complete path of the execve stage in the execve_map (adding it in the execve_map_value struct) from which we already retrieve the binary index. It would leave the old implem untouched except for the copy of the string from execve_msg_heap_map containing the whole msg_execve_event to the execve_map_value persistence map which contains only a subset of that, which will be done at at execve_send (like it's done for other persisted fields, like the binary index at the moment).

Then in the case of Prefix and Postfix, have a trie populated per selector at loading time containing the prefixes, retrieve the path, similarly as we do currently by retrieving the execve->binary information after getting via event_find_curr, it would add something like a execve->binary_path read (only in the case of Prefix and Postfix).

execve = event_find_curr(&ppid, &walker);
			if (!execve)
				return 0;

			trace_printk("%d", execve->process);
			bin_key = execve->binary;

We could still check if the pointer is NULL or not for In and NotIn and in the case Prefix and Postfix, lookup against the new trie containing the prefix with the value retrieved from the execve->binary_path.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
kind/enhancement This improves or streamlines existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants