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

fix(filters): int conversion without check #4482

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

geyslan
Copy link
Member

@geyslan geyslan commented Jan 10, 2025

Takes care of some of #4481.

1. Explain what the PR does

4bef591 fix(filters): int conversion without check

This silences some CodeQL "Incorrect conversion between integer types"
warnings.

2. Explain how to test it

3. Other comments

@@ -70,11 +71,16 @@ func attachSuspiciousSyscallSourceProbes(t *Tracee, eventParams []map[string]fil
if err != nil {
return err
}
if !events.Core.IsDefined(events.ID(syscallID)) {
if syscallID < 0 || syscallID > math.MaxInt32 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this kind of check be resolved inside the IsDefined method? Is this the particular code which fixes the CodeQL issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't this kind of check be resolved inside the IsDefined method?

IsDefined only checks for the existence of a events.ID value, it's agnostic to the origin of that value, so I believe it's sane to check it right after the Atoi conversion, since we're truncating the output (int - 64) to events.ID (int32).

Is this the particular code which fixes the CodeQL issue?

Yep. It's an attempt. Let's see if the issue is closed automagically after this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, I would like to ask you if these conversions https://github.com/aquasecurity/tracee/security/code-scanning/2 are ok. I mean, if they are the full size of those values, why the API providing them uses int (or other larger types) instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Related:

https://github.com/aquasecurity/tracee/pull/4484/files/21ff8fd5251120a52a961d6d9eb466f0f22135e9..ce22b0f64fe6a85a09760becd432a90dbced4709

image

If there's difference of size between cgroup HID versions, the API should be explicit and provide appropriate getters.

This silences some CodeQL "Incorrect conversion between integer types"
warnings.
@rscampos
Copy link
Collaborator

@geyslan LGTM

Realized that there are other 2 places also with the same issue (https://github.com/aquasecurity/tracee/security/code-scanning):

tracee/pkg/ebpf/tracee.go

Lines 301 to 303 in ca5ebd4

for mountNS, pid := range mntNSProcs {
t.pidsInMntns.AddBucketItem(uint32(mountNS), uint32(pid))
}

Following the same approach:

    for mountNS, pid := range mntNSProcs {
        if mountNS < 0 || mountNS > math.MaxUint32 {
            return errfmt.Errorf("invalid mnt namespace %d", mountNS)
        }
        if pid < 0 || pid > math.MaxUint32 {
            return errfmt.Errorf("invalid pid %d", pid)
        }

        t.pidsInMntns.AddBucketItem(uint32(mountNS), uint32(pid))
    }

for mountNS, p := range mountNSTimeMap {
mountNSToFirstProcess[mountNS] = int(p.pid)
}

Following the same approach:

for mountNS, p := range mountNSTimeMap {
	if p.pid > math.MaxInt32 {
		return nil, errfmt.Errorf("invalid pid %d", p.pid)
	}
	mountNSToFirstProcess[mountNS] = int(p.pid)
}

@geyslan
Copy link
Member Author

geyslan commented Jan 21, 2025

I didn't touch the others since they deserve a better analysis (API wise).

Copy link
Collaborator

@rscampos rscampos left a comment

Choose a reason for hiding this comment

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

LGTM

@geyslan geyslan merged commit 0e2bc65 into aquasecurity:main Jan 22, 2025
40 of 41 checks passed
@geyslan geyslan mentioned this pull request Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants