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

pkg/sysctl: Sanitize parameter names #14533

Merged
merged 3 commits into from
Apr 6, 2021

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented Jan 6, 2021

This PR avoids a warning issued by CodeQL where it identified "Uncontrolled data used in path expression". Refs #14514.

@twpayne twpayne added the release-note/misc This PR makes changes that have no direct user impact. label Jan 6, 2021
@twpayne twpayne requested review from brb, a team and joestringer January 6, 2021 13:12
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Jan 6, 2021
@twpayne twpayne force-pushed the pr/twpayne/sanitize-sysctl-params branch from de4e8af to 2f1a769 Compare January 6, 2021 13:34
pkg/sysctl/sysctl.go Outdated Show resolved Hide resolved
cilium-health/launch/endpoint.go Outdated Show resolved Hide resolved
@twpayne twpayne force-pushed the pr/twpayne/sanitize-sysctl-params branch from 2f1a769 to e94dbcf Compare January 7, 2021 13:49
@twpayne
Copy link
Contributor Author

twpayne commented Jan 12, 2021

test-me-please

@twpayne
Copy link
Contributor Author

twpayne commented Jan 12, 2021

retest-4.9

1 similar comment
@twpayne
Copy link
Contributor Author

twpayne commented Jan 12, 2021

retest-4.9

@twpayne twpayne force-pushed the pr/twpayne/sanitize-sysctl-params branch from e94dbcf to 1c8f0bd Compare January 12, 2021 15:25
@twpayne
Copy link
Contributor Author

twpayne commented Jan 12, 2021

test-me-please

@twpayne
Copy link
Contributor Author

twpayne commented Jan 12, 2021

test-gke

@twpayne twpayne added the dont-merge/preview-only Only for preview or testing, don't merge it. label Jan 13, 2021
@twpayne twpayne force-pushed the pr/twpayne/sanitize-sysctl-params branch from bf87d77 to 9ac0c6a Compare January 13, 2021 12:06
@twpayne twpayne requested a review from a team as a code owner January 13, 2021 12:06
@twpayne twpayne requested a review from tklauser January 13, 2021 12:06
@twpayne twpayne removed the dont-merge/preview-only Only for preview or testing, don't merge it. label Jan 13, 2021
@twpayne twpayne force-pushed the pr/twpayne/sanitize-sysctl-params branch from 9ac0c6a to 29b6ede Compare January 13, 2021 13:29
@twpayne
Copy link
Contributor Author

twpayne commented Jan 13, 2021

test-me-please

@twpayne
Copy link
Contributor Author

twpayne commented Jan 13, 2021

The Travis CI failure is #11990.

@twpayne
Copy link
Contributor Author

twpayne commented Jan 13, 2021

retest-gke

pkg/sysctl/sysctl.go Outdated Show resolved Hide resolved
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

No remarks apart from what André pointed out.

@twpayne twpayne force-pushed the pr/twpayne/sanitize-sysctl-params branch from c5f0f00 to 72a7f13 Compare March 26, 2021 12:19
@aanm
Copy link
Member

aanm commented Mar 29, 2021

test-me-please

@nathanjsweet
Copy link
Member

#15459 on gke.

@nathanjsweet
Copy link
Member

@brb What do you make of the wireguard flake in net-next?

@brb
Copy link
Member

brb commented Mar 30, 2021

@nathanjsweet Thanks for pinging. Opened #15507 which might resolve it.

@twpayne twpayne force-pushed the pr/twpayne/sanitize-sysctl-params branch from 72a7f13 to 2092798 Compare March 30, 2021 16:07
@twpayne
Copy link
Contributor Author

twpayne commented Mar 30, 2021

test-me-please

@aanm
Copy link
Member

aanm commented Mar 30, 2021

@twpayne sorry, can you rebase against master and trigger the tests just to be safe?

@twpayne twpayne force-pushed the pr/twpayne/sanitize-sysctl-params branch from 2092798 to e209d3e Compare March 31, 2021 00:34
@twpayne
Copy link
Contributor Author

twpayne commented Mar 31, 2021

test-me-please

@twpayne
Copy link
Contributor Author

twpayne commented Mar 31, 2021

Cilium-PR-K8s-1.20-kernel-4.9 failure seems to be known flake #13011.

Cilium-PR-K8s-1.13-net-next failure seems to be known flake #12511.

@twpayne
Copy link
Contributor Author

twpayne commented Mar 31, 2021

test-1.13-netnext

@twpayne
Copy link
Contributor Author

twpayne commented Mar 31, 2021

test-1.20-4.9

This avoids a security warning raised by CodeQL. In theory, before this
PR, carefully formed parameter names could read arbitrary files in the
filesystem, e.g.
    sysctl.Read("../../etc/passwd")
In practice, this is was likely unexploitable as '.'s were replaced with
'/'s, making path traversal tricky.

The updated code verifies that parameter names are valid.

Signed-off-by: Tom Payne <tom@isovalent.com>
Signed-off-by: Tom Payne <tom@isovalent.com>
Signed-off-by: Tom Payne <tom@isovalent.com>
@twpayne twpayne force-pushed the pr/twpayne/sanitize-sysctl-params branch from e209d3e to 283b9d9 Compare March 31, 2021 12:47
@twpayne
Copy link
Contributor Author

twpayne commented Mar 31, 2021

test-me-please

1 similar comment
@twpayne
Copy link
Contributor Author

twpayne commented Mar 31, 2021

test-me-please

@pchaigno
Copy link
Member

pchaigno commented Apr 6, 2021

@pchaigno
Copy link
Member

pchaigno commented Apr 6, 2021

Provisioning issue:

test-runtime

@pchaigno
Copy link
Member

pchaigno commented Apr 6, 2021

k8s-1.16-kernel-netnext failed with known flake #15455. k8s-1.19-kernel-4.19 failed with known flake #15337 and isn't required anyway. Reviews are in. Marking and merging.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 6, 2021
@pchaigno pchaigno merged commit 127a5d8 into master Apr 6, 2021
1.10.0 automation moved this from In progress to Done Apr 6, 2021
@pchaigno pchaigno deleted the pr/twpayne/sanitize-sysctl-params branch April 6, 2021 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants