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

bpf: fix missed linux_binprm_type in selector_arg_offset function #2623

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

anfedotoff
Copy link
Contributor

It's seems to me, that case linux_binprm_type: in selector_arg_offset function is missing. security_bprm_check.yaml from examples doesn't work.

@anfedotoff anfedotoff requested a review from a team as a code owner July 2, 2024 16:28
@anfedotoff anfedotoff requested a review from kevsecurity July 2, 2024 16:28
Copy link

netlify bot commented Jul 2, 2024

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit adff394
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/6685126cbe908a000879af4d
😎 Deploy Preview https://deploy-preview-2623--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@kevsecurity kevsecurity added kind/bug Something isn't working release-note/misc This PR makes changes that have no direct user impact. labels Jul 2, 2024
@anfedotoff anfedotoff force-pushed the fix-linux_bprm-filter branch from 76445c1 to 6818e16 Compare July 2, 2024 16:46
Copy link
Contributor

@kevsecurity kevsecurity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kkourt
Copy link
Contributor

kkourt commented Jul 2, 2024

There is a test failure, which seems related:

FAIL: TestLinuxBinprmExtractPath (1.12s)

Copy link
Contributor

@kevsecurity kevsecurity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to work out why the ARM test is failing.

@anfedotoff
Copy link
Contributor Author

Yes, I'll look at failing tests.

@kevsecurity
Copy link
Contributor

Yes, I'll look at failing tests.

It might be that the tests themselves are broken.

@mtardy
Copy link
Member

mtardy commented Jul 2, 2024

Yep it seems test itself is broken:

MatchArgs: []v1alpha1.ArgSelector{
{
Operator: "In",

It should be "Equal", "In" does not exist for matchArgs.

It bypasses validation since it doesn't pass YAML unmarshalling.

I just tried changing the exec command and the test passes all the time, it needs a fix! I'm writing something.

@mtardy
Copy link
Member

mtardy commented Jul 2, 2024

@anfedotoff I have a patch for the test that actually fails without your patch and succeeds with your patch (which should have been what happened previously), is it okay if I push to your branch?

@mtardy
Copy link
Member

mtardy commented Jul 2, 2024

@anfedotoff I have a patch for the test that actually fails without your patch and succeeds with your patch (which should have been what happened previously), is it okay if I push to your branch?

I created #2624 so that I can run the tests from there. We can merge like this with your commit authored to you. Or I can merge the fix of the test and then we fix your patch, however you prefer.

@anfedotoff
Copy link
Contributor Author

@anfedotoff I have a patch for the test that actually fails without your patch and succeeds with your patch (which should have been what happened previously), is it okay if I push to your branch?

Thanks, yes, sure!

@anfedotoff
Copy link
Contributor Author

@anfedotoff I have a patch for the test that actually fails without your patch and succeeds with your patch (which should have been what happened previously), is it okay if I push to your branch?

I created #2624 so that I can run the tests from there. We can merge like this with your commit authored to you. Or I can merge the fix of the test and then we fix your patch, however you prefer.

I think, the easiest way is to merge your PR with my commit and your test fix.

@mtardy
Copy link
Member

mtardy commented Jul 2, 2024

@anfedotoff I have a patch for the test that actually fails without your patch and succeeds with your patch (which should have been what happened previously), is it okay if I push to your branch?

I created #2624 so that I can run the tests from there. We can merge like this with your commit authored to you. Or I can merge the fix of the test and then we fix your patch, however you prefer.

I think, the easiest way is to merge your PR with my commit and your test fix.

Ok we can do that, then given your patch, my test failed on 4.19 since it doesn't have large progs. If it's expected we should just skip this test!

@anfedotoff
Copy link
Contributor Author

@anfedotoff I have a patch for the test that actually fails without your patch and succeeds with your patch (which should have been what happened previously), is it okay if I push to your branch?

I created #2624 so that I can run the tests from there. We can merge like this with your commit authored to you. Or I can merge the fix of the test and then we fix your patch, however you prefer.

I think, the easiest way is to merge your PR with my commit and your test fix.

Ok we can do that, then given your patch, my test failed on 4.19 since it doesn't have large progs. If it's expected we should just skip this test!

Yes, I think it is expected. We use large progs to copy args of this type, if I'm not mistaken.

@kevsecurity
Copy link
Contributor

Hmm, why's it failing some tests on bpf-next?

anfedotoff and others added 3 commits July 3, 2024 11:57
The test was broken in two ways:
- First it was using the "In" operator with MatchArgs, which does not
  exist. "Equal" should be used. It can be confusing because
  MatchBinaries uses "In". It bypasses validation since it provided the
  Go object without using the YAML unmarshalling.
- Secondly, it was only checking that the event existed without checking
  that the filtering actually happened. We can check the "lack" of event
  in addition to the presence of it thanks to the way this test is
  written, using the perfring.RunTestEvents.

This test should have detected the issue fixed by the following patch.

Signed-off-by: Mahe Tardy <[email protected]>
@anfedotoff anfedotoff force-pushed the fix-linux_bprm-filter branch from 0bd2cb8 to adff394 Compare July 3, 2024 08:57
@anfedotoff
Copy link
Contributor Author

Hmm, why's it failing some tests on bpf-next?

It seems to me, that changes it the PR don't influence on this test. I rebased this branch on current main, let's look at the tests.

Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think the fix is complete now :)

@mtardy mtardy requested a review from kevsecurity July 3, 2024 10:20
@kevsecurity kevsecurity merged commit 857205e into cilium:main Jul 3, 2024
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants