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

test(test): Adding fuzzer for ContainerPolicy #1797

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

prady0t
Copy link

@prady0t prady0t commented Jul 7, 2024

Purpose of PR?:

This is a fuzzer for ContainerPolicy

func (p *PolicyServer) ContainerPolicy(c context.Context, data *pb.Policy) (*pb.Response, error) {

For seed corpus I referred to this policy and removed Metadata.Name from it.
With this we are testing if invalid policy results in PolicyStatus_Invalid or not.
To run the fuzz test,

cd KubeArmor/policy
go test -fuzz=Fuzz

Addresses #1367

Does this PR introduce a breaking change?
NO

@prady0t
Copy link
Author

prady0t commented Jul 10, 2024

Already found an issue with this. I ran the fuzzer for a while and It failed for this input :

{
    "0000": "00000000000000",
    "oBjeCt": {
        "metAdAtA": {
            "nAme": "0"
        }
    }
}

Which is totally understandable. To fix this we can also have a condition in fuzzer to check for status as Applied if we have policyEvent.Object.Metadata.Name != "" . Here's the log :

--- FAIL: FuzzContainerPolicy (0.00s)
    --- FAIL: FuzzContainerPolicy/69798181ccc6962d (0.00s)
        ContainerPoilcy_fuzz_test.go:58: Unexpected status: Applied, [123 32 32 32 32 34 48 48 48 48 34 58 32 34 48 48 48 48 48 48 48 48 48 48 48 48 48 48 34 44 32 32 32 32 34 111 66 106 101 67 116 34 58 32 123 32 32 32 32 32 34 109 101 116 65 100 65 116 65 34 58 32 123 32 32 32 32 32 32 34 110 65 109 101 34 58 32 34 48 34 125 125 125]
FAIL
exit status 1
FAIL    github.com/kubearmor/KubeArmor/KubeArmor/policy 0.011s

Marking this as a draft for now.

@prady0t
Copy link
Author

prady0t commented Jul 26, 2024

I've added a check for pb.PolicyStatus_Applied as well for JSON inputs with Object.Metadata.Name != "". With this, our fuzzer should run until it finds an edge case where input results in Unexpected Status. We could have also added a condition to check if we get a pb.PolicyStatus_Applied status when Object.Metadata.Name != "" inside the fuzzer itself, instead of just the AND condition. But that would require to unmarshal the data and would be kind of meta (testing json.Unmarshal with json.Unmarshal).

If you think it's testing something useful, we can go ahead and merge it. Once we have this inside KubeArmor codebase, we can try writing config files so that we can integrate it with oss-fuzz. Running it locally won't give much fruitful results.
In the cilium fuzz testing report (mentioned in the upstream issue comments), we can see fuzzers run for 1000s of hours :

Screenshot 2024-07-26 at 10 59 30 PM

Running our fuzzers with oss fuzz will help us improve it even better. We may also find some crucial bugs. This could be a starting point for how we can add more fuzzer.

@daemon1024 @DelusionalOptimist Do let me know what you guys think of this.

@prady0t prady0t marked this pull request as ready for review July 26, 2024 18:50
Copy link
Contributor

@nyrahul nyrahul left a comment

Choose a reason for hiding this comment

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

Thank you for the PR

KubeArmor/policy/ContainerPoilcy_fuzz_test.go Outdated Show resolved Hide resolved
KubeArmor/policy/ContainerPoilcy_fuzz_test.go Outdated Show resolved Hide resolved
@nyrahul
Copy link
Contributor

nyrahul commented Aug 7, 2024

I think this PR and the resultant image could be uses as input for OSS-Fuzz. How long did you test it on your local system? @prady0t

@prady0t
Copy link
Author

prady0t commented Aug 8, 2024

I think this PR and the resultant image could be uses as input for OSS-Fuzz. How long did you test it on your local system? @prady0t

For about an hour. Do you want me to run it overnight? It might help us find any early bug or any problems with the fuzzer.

Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
@nyrahul
Copy link
Contributor

nyrahul commented Aug 10, 2024

Running it in current state won't result in anything since the primary function is stubbed out.

@nyrahul
Copy link
Contributor

nyrahul commented Aug 10, 2024

The primary updates I made using the last commit is:

  • Using the real ParseAndUpdateContainerSecurityPolicy to fuzz test. Removed the stubbed UpdateContainerPolicy.
  • Fixing the fuzz test initial data

This is already showing up some issues during fuzz ... We need to check if those are genuine issues or need stubbing.

@prady0t
Copy link
Author

prady0t commented Aug 10, 2024

Thanks a Lot for these changes! This makes much more sense now. Let me try to run these locally

@prady0t
Copy link
Author

prady0t commented Aug 22, 2024

Sorry for the late reply. Fuzzer is failing for this input :

[]byte("{\"oBjeCt\":{\"metAdAtA\":{\"nAme\":\"0\"},\"speC\":{\"seleCtor\":{\"mAtChLABels\":{\"\":\"\"}}}}}")

@nyrahul did you find similar results locally?

JSON format :

{
  "oBjeCt": {
    "metAdAtA": {
      "nAme": "0"
    },
    "speC": {
      "seleCtor": {
        "mAtChLABels": {
          "": ""
        }
      }
    }
  }
}

@prady0t
Copy link
Author

prady0t commented Sep 29, 2024

The fuzzer is now running. Would like your reviews @nyrahul @daemon1024 @DelusionalOptimist

Comment on lines 14 to 15
initialData := &pb.Policy{
Policy: []byte(`
Copy link
Member

Choose a reason for hiding this comment

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

Let's use varied kinds of policy including file, network as seed values. https://github.com/kubearmor/KubeArmor/tree/main/tests/k8s_env/ksp/multiubuntu

Comment on lines +182 to +186
if(endpointIdx != -1){
dm.EndPoints[endpointIdx] = newPoint
dm.Logger.UpdateSecurityPolicies("DELETED", newPoint)
dm.RuntimeEnforcer.UpdateSecurityPolicies(newPoint)
// delete endpoint if no containers or policies
Copy link
Member

Choose a reason for hiding this comment

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

We already found a bug and fixed it, Nice 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants