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 boundary checks #2338

Merged
merged 1 commit into from
Jan 5, 2024
Merged

Conversation

mauriciovasquezbernal
Copy link
Member

  • Add missing check on pkg/runtime/grpc/grpc-runtime.go
  • Use ParseInt() instead of ParseUint() in pkg/gadgets/snapshot/process/tracer/tracer.go

https://github.com/inspektor-gadget/inspektor-gadget/security/code-scanning/12
https://github.com/inspektor-gadget/inspektor-gadget/security/code-scanning/13
https://github.com/inspektor-gadget/inspektor-gadget/security/code-scanning/14

Fixes: 2ad8b61 ("pkg: Add bound checks for integer converted by strconv functions.")

Copy link
Member

@eiffel-fl eiffel-fl left a comment

Choose a reason for hiding this comment

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

Hi!

Glad you understood how to fix these!
These reports are cool but I sometimes find them cryptic.
I just find some nits, otherwise you can merge it.

Best regards.

pkg/gadgets/snapshot/process/tracer/tracer.go Outdated Show resolved Hide resolved
pkg/gadgets/snapshot/process/tracer/tracer.go Outdated Show resolved Hide resolved
@mauriciovasquezbernal mauriciovasquezbernal force-pushed the mauricio/fix-boundary-checks branch 2 times, most recently from 0326600 to ff5d08d Compare January 5, 2024 13:56
@mauriciovasquezbernal
Copy link
Member Author

I'm still not totally convinced with the solution in pkg/runtime/grpc/grpc-runtime.go. I don't think we should be making extra validations there as this is already done in the params library:

TypeUint: ValidateUint(strconv.IntSize),

I'm not totally sure what's the error being reported by codeql, I think about two possibilities:

  1. Lack of error check of strconv.ParseUint in params.AsUint, should we panic if there is an error? (@flyth)
  2. CodeQL has a right point that an uint to int64 conversion without a higher bound check is incorrect, perhaps this parameter could just be a uint16? (I don't think anybody will use a timeout higher than 2^16)

I think for the sake of simplicity on this PR, I'll go for option 2.

- Update timeout parameter to be Uint16 on pkg/runtime/grpc/grpc-runtime.go
- Use ParseInt() instead of ParseUint() in pkg/gadgets/snapshot/process/tracer/tracer.go

https://github.com/inspektor-gadget/inspektor-gadget/security/code-scanning/12
https://github.com/inspektor-gadget/inspektor-gadget/security/code-scanning/13
https://github.com/inspektor-gadget/inspektor-gadget/security/code-scanning/14

Fixes: 2ad8b61 ("pkg: Add bound checks for integer converted by strconv functions.")

Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>
@mauriciovasquezbernal mauriciovasquezbernal merged commit 63e8451 into main Jan 5, 2024
53 checks passed
@mauriciovasquezbernal mauriciovasquezbernal deleted the mauricio/fix-boundary-checks branch January 5, 2024 15:46
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.

None yet

2 participants