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 path substitution to enable setting sysctls on vlan interfaces #779

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

mmirecki
Copy link
Contributor

@mmirecki mmirecki commented Nov 4, 2022

This commit changes the order of substituting sysctl path to first handle . to / change, before substituting the interface name. This is needed as vlan interfaces have a . in the name, which should not be changed.

@mmirecki
Copy link
Contributor Author

mmirecki commented Nov 4, 2022

@dcbw @s1061123 Could you please take a look? Thanks

Copy link
Member

@mars1024 mars1024 left a comment

Choose a reason for hiding this comment

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

/lgtm, do you mind adding some tests for this? @mmirecki

@s1061123
Copy link
Contributor

s1061123 commented Nov 7, 2022

/lgtm too. Thanks, @mmirecki !

@mmirecki
Copy link
Contributor Author

mmirecki commented Nov 7, 2022

/lgtm, do you mind adding some tests for this? @mmirecki

To test it properly it would have to be tested with namespaces sysctl's, which is currently not doable with the current test framwork. Currently we test the sysctl's with "net.ipv4.conf.all" which would work here.

@squeed
Copy link
Member

squeed commented Nov 7, 2022

@mmirecki this is a potential directory traversal issue, since we don't actually validate that IFNAME is sane. Can you check to make sure that IFNAME doesn't contain any "/"?

We should also add a check to ensure that the resolved path doesn't escape /proc/sys.

@mmirecki
Copy link
Contributor Author

mmirecki commented Nov 8, 2022

@mmirecki this is a potential directory traversal issue, since we don't actually validate that IFNAME is sane. Can you check to make sure that IFNAME doesn't contain any "/"?

We should also add a check to ensure that the resolved path doesn't escape /proc/sys.

Added validateArgs to validate the interface name does not contain any path separators.

This commit changes the order of substituting sysctl path to first handle
. to / change, before substituting the interface name.
This is needed as vlan interfaces have a . in the name, which should not
be changed.

Signed-off-by: mmirecki <[email protected]>
@mmirecki
Copy link
Contributor Author

mmirecki commented Nov 9, 2022

@squeed could you please look again? Added the requested validation

@dcbw
Copy link
Member

dcbw commented Nov 14, 2022

/lgtm

@dcbw dcbw merged commit 7e9ada5 into containernetworking:main Nov 14, 2022
mmirecki pushed a commit to mmirecki/plugins that referenced this pull request Nov 17, 2022
Fix path substitution to enable setting sysctls on vlan interfaces
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.

5 participants