bpf: Fix Prefix operator for matchBinaries

[anfedotoff]

It seems to me, that Prefix operator for matchBinaries selector is not applied if executable path length is greater than 256 bytes.

[anfedotoff]

I discovered this issue while working on #2689. @mtardy , @kevsecurity , please, have a look.

[mtardy]

I discovered this issue while working on #2689. @mtardy , @kevsecurity , please, have a look.

Hey I haven't looked in details but a good way to present this would be:

1. A failing test
2. Fix the failure to show the test now succeed

[anfedotoff]

I discovered this issue while working on #2689. @mtardy , @kevsecurity , please, have a look.

Hey I haven't looked in details but a good way to present this would be:

1. A failing test
2. Fix the failure to show the test now succeed

I have Tetragon running and it is armed with this policy:

apiVersion: cilium.io/v1alpha1
kind: TracingPolicy
metadata:
  name: "lsm"
spec:
  lsmhooks:
  - hook: "file_open"
    args:
      - index: 0
        type: "file"
    selectors:
    - matchBinaries:
      - operator: "Prefix"
        values:
        - "/home/anfedotoff/go/src/github.com/cilium/tetragon/contrib/tester-progs"

if I run this binary, the LSM is not triggered, but it seems to me it does.

/home/anfedotoff/go/src/github.com/cilium/tetragon/contrib/tester-progs/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb/ccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc/ddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd/direct-write-tester /tmp/test-test

[netlify]

✅ Deploy Preview for tetragon ready!

Name	Link
🔨 Latest commit	f26e8f5
🔍 Latest deploy log	https://app.netlify.com/sites/tetragon/deploys/669fd6df8cc82a00087c71d1
😎 Deploy Preview	https://deploy-preview-2718--tetragon.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[anfedotoff]

@mtardy, Hey! I added a test. It is failed on some VMs. I think, it because it isn't allowed to run binaries from /tmp

kprobe_test.go:3923: failed to run direct-write-tester: exit status 255

But overall it shows the problem, I think. Maybe you have an idea where to create a large path to test binary?

[mtardy]

@mtardy, Hey! I added a test. It is failed on some VMs. I think, it because it isn't allowed to run binaries from /tmp

kprobe_test.go:3923: failed to run direct-write-tester: exit status 255

But overall it shows the problem, I think. Maybe you have an idea where to create a large path to test binary?

yes, do you remember this #2698?

tetragon/pkg/sensors/tracing/enforcer_test.go

Lines 300 to 304 in b506aa1

	// We can't use t.TempDir as it writes into /tmp by default.
	// The direct-write-tester.c program opens and writes using the O_DIRECT
	// flag that is unsupported and return EINVAL on tmpfs, while it works on a
	// disk based fs. Recently, the base image used by vmtests started to switch
	// /tmp from the disk to tmpfs which made that test fail.

[mtardy]

Maybe you have an idea where to create a large path to test binary?

I once created this thing https://github.com/mtardy/pathgen that could be adapted or copy-pasted in our test base, as the comment below, you could use /var/tmp or just stop using direct write tester.

If you can avoid using the direct write tester I would as it uses a not very user-friendly Linux interface (breaks on tmpfs).

[anfedotoff]

Maybe you have an idea where to create a large path to test binary?

I once created this thing https://github.com/mtardy/pathgen that could be adapted or copy-pasted in our test base, as the comment below, you could use /var/tmp or just stop using direct write tester.

If you can avoid using the direct write tester I would as it uses a not very user-friendly Linux interface (breaks on tmpfs).

Ahh, thanks! I think I'll try to look at some other executable from contrib/tester-progs.

[anfedotoff]

Tests now are fine, @mtardy, if you have time, please have a look). This fix is also is needed to #2689, I suppose.

[mtardy]

Thanks for the test, it seems to highlight the issue nicely, it's a great start! I think this is something I missed because I somehow forgot that the dentry will be walked from end to start and thus this will break the Prefix feature! Thanks!

could you explain more about your patch however? It looks to me that you rewrote the existing <5.4 behavior implem (with the heap, instead of p->args) as a fallback from the read_exe failure. See my comments for more details.

[mtardy]

so I'm not sure it's really appropriate as a fix:

• The current implementation is indeed limited to 256 bytes on exe read, but this could be increased (maybe that's what you want to actually do). This is indeed bad that it's failing open for something as Prefix.
• Here you read execve_heap that contains the binary only because of the way we read args at tracepoint/sys_execve, I don't think there are any guarantees that this heap will actually contain this btw since it's just a heap, we can use it for anything. This rollback to the old situation where we retrieved the filename from that args (which is still in place for <5.4) that could lead to read a relative path or a symlink (we switched to exe for that reason, reading the syscall args wasn't reliable). See:

  tetragon/bpf/process/bpf_execve_event.c

  Lines 248 to 254 in 92a17c2

  	#ifdef __LARGE_BPF_PROG
  	// Reading the absolute path of the process exe for matchBinaries.
  	// Historically we used the filename, a potentially relative path (maybe to
  	// a symlink) coming from the execve tracepoint. For kernels not supporting
  	// large BPF prog, we still use the filename.
  	read_exe((struct task_struct *)get_current_task(), &event->exe);
  	#endif

I agree that there is a limitation on the current implementation which is pretty explicit (with BINARY_PATH_MAX_LEN), but falling back to the way we do things on <5.4 kernels will not help as it's also flawed and can be escaped with relative paths symlinks.

[mtardy]

Thinking a bit more about this, it's indeed not great that we never check for the error at read from #1926 and that fallback to the args' filename could be an idea but it looks flawed. Indeed, we have unit tests on the function that uses read_exe and the error should be filled if the path is too big to enter the buffer of 256 (it seems that the limit is 255) so we should be aware that we could not match properly.

Because as your test highlights, an errored dentry will not work since it's reading from the end to the start, and thus the beginning of the path containing the prefix will not be here. Switching back to the old implem (as it seems you do even though you read from the execve_heap instead of p->args) could work until you don't use any relative path or symlink.

So I see two things:

• support the error more explicitly and return an error to the user so that he knows we might have skipped an event because of path len limitation
• fix the issue by increasing BINARY_PATH_MAX_LEN to 4096.

[anfedotoff]

Thanks for detailed review! Yes you are right, relative paths and symlinks might be a problem. It is good to find a better solution that I proposed. Let me explain what I've done.

IIUC, we read the first argument here.

tetragon/bpf/process/bpf_execve_event.c

Lines 59 to 69 in 1a7b15a

	/* skip first argument - binary path */
	heap = map_lookup_elem(&execve_heap, &zero);
	if (!heap)
	return 0;
	/* poor man's strlen */
	off = probe_read_str(&heap->maxpath, 4096, (char *)start_stack);
	if (off < 0)
	return 0;
	start_stack += off;

Here we think that p->args contains the first argument, but it seems to me that p->args contains arguments starting from the second one. So that's why I changed the logic for old kernels.

tetragon/bpf/process/bpf_execve_event.c

Line 338 in 1a7b15a

// reuse p->args first string that contains the filename, this can't be

[anfedotoff]

So I see two things:
support the error more explicitly and return an error to the user so that he knows we might have skipped an event because of path len limitation
fix the issue by increasing BINARY_PATH_MAX_LEN to 4096.

I think it's a good idea to increase BINARY_PATH_MAX_LEN to 4096. If so we don't need file_copy_reverse here. And also, we don't need to have extra variables to hold postfix path.

Do you see any limitations that can stop us to increase the BINARY_PATH_MAX_LEN?
IIUC, limitations with prefix/postfix matching we can avoid by copying exact amount of data (STRING_PREFIX_MAX_LENGTH / STRING_POSTFIX_MAX_LENGTH).

[anfedotoff]

We can have problems here, if __LARGE_MAP_KEYS is not defined, I think:

tetragon/bpf/process/types/basic.h

Lines 1581 to 1587 in 1a7b15a

	case op_filter_in:
	case op_filter_notin:
	path_map = map_lookup_elem(&tg_mb_paths, &selidx);
	if (!path_map)
	return 0;
	found_key = map_lookup_elem(path_map, current->bin.path);
	break;

.

But we can have a check for this case.

[mtardy]

Here we think that p->args contains the first argument, but it seems to me that p->args contains arguments starting from the second one. So that's why I changed the logic for old kernels.

It's not the case, event->process.args is read in function read_path and the first string separated by \0 is the first args, thus the (relative/symlink/etc) binary given to sys_execve:

tetragon/bpf/process/bpf_execve_event.c

Lines 102 to 111 in 8e07d6c

	read_path(void *ctx, struct msg_execve_event *event, void *filename)
	{
	struct msg_process *p = &event->process;
	__u32 size = 0;
	__u32 flags = 0;
	char *earg;
	earg = (void *)p + offsetof(struct msg_process, args);
	size = probe_read_str(earg, MAXARGLENGTH - 1, filename);

[mtardy]

read_args, reads the args (hear the args of the execve binary not the args of the syscall), and thus use probe_read_str to remove the first part of the args, but does not touch it.

[anfedotoff]

Hmm... I see, thanks making it clear!

[mtardy]

Do you see any limitations that can stop us to increase the BINARY_PATH_MAX_LEN?

It all boils down to make prepend_name support more than 256 bytes. I'm looking into it at the moment as it's the only function I've written BPF unit tests so that might be helpful here.

[mtardy]

Thinking a bit more about this, it's indeed not great that we never check for the error at read from #1926 and that fallback to the args' filename could be an idea but it looks flawed. Indeed, we have unit tests on the function that uses read_exe and the error should be filled if the path is too big to enter the buffer of 256 (it seems that the limit is 255) so we should be aware that we could not match properly.

Because as your test highlights, an errored dentry will not work since it's reading from the end to the start, and thus the beginning of the path containing the prefix will not be here. Switching back to the old implem (as it seems you do even though you read from the execve_heap instead of p->args) could work until you don't use any relative path or symlink.

So I see two things:

• support the error more explicitly and return an error to the user so that he knows we might have skipped an event because of path len limitation
• fix the issue by increasing BINARY_PATH_MAX_LEN to 4096.

[anfedotoff]

The Fix: #2764