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
golang 1.17 fails to parse IPs with leading zeros #104368
Conversation
/kind bug |
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/104368/pull-kubernetes-unit/1428640022386446336 |
/hold cancel |
@aojea: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/skip |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks @aojea ! FWIW I have ruleguard working EXCEPT across modules. It can run it from the k/k root and it finds errors in k/k and in vendored modules, but not in staging. In fact, I have it stored in hack/tools (which is a module) and it works against the root module (presumable because it is a path prefix) but it does not work in staging, still. This is one of those "go modules is dumb" things but also "ruleguard is dumb". I filed some issues with ruleguard and golangci-lint, and until those are resolved the only answers I see are: a) keep using your custom thing |
/triage accepted |
``` +==============================================================================+ | | | /$$$$$$ /$$ | | /$$__ $$ | $$ | | /$$$$$$$ /$$$$$$ | $$ \__//$$$$$$ /$$$$$$ /$$ /$$ | | /$$_____/ |____ $$| $$$$ /$$__ $$|_ $$_/ | $$ | $$ | | | $$$$$$ /$$$$$$$| $$_/ | $$$$$$$$ | $$ | $$ | $$ | | \____ $$ /$$__ $$| $$ | $$_____/ | $$ /$$| $$ | $$ | | /$$$$$$$/| $$$$$$$| $$ | $$$$$$$ | $$$$/| $$$$$$$ | | |_______/ \_______/|__/ \_______/ \___/ \____ $$ | | /$$ | $$ | | | $$$$$$/ | | by pyup.io \______/ | | | +==============================================================================+ | REPORT | | checked 30 packages, using free DB (updated once a month) | +============================+===========+==========================+==========+ | package | installed | affected | ID | +============================+===========+==========================+==========+ | kubernetes | 23.3.0 | >0 | 45114 | +==============================================================================+ | Kubernetes (python client) uses Kubernetes API, which has an unfixed | | vulnerability, CVE-2021-29923: Go before 1.17 does not properly consider | | extraneous zero characters at the beginning of an IP address octet, which | | (in some situations) allows attackers to bypass access control that is based | | on IP addresses, because of unexpected octal interpretation. This affects | | net.ParseIP and net.ParseCIDR. Kubernetes interprets leading zeros on IPv4 | | addresses as decimal to keep backwards compatibility, but users relying on | | parser alignment will be impacted by this CVE. | | kubernetes/kubernetes#104368 | | kubernetes/kubernetes#108074 | +==============================================================================+ ```
``` +==============================================================================+ | | | /$$$$$$ /$$ | | /$$__ $$ | $$ | | /$$$$$$$ /$$$$$$ | $$ \__//$$$$$$ /$$$$$$ /$$ /$$ | | /$$_____/ |____ $$| $$$$ /$$__ $$|_ $$_/ | $$ | $$ | | | $$$$$$ /$$$$$$$| $$_/ | $$$$$$$$ | $$ | $$ | $$ | | \____ $$ /$$__ $$| $$ | $$_____/ | $$ /$$| $$ | $$ | | /$$$$$$$/| $$$$$$$| $$ | $$$$$$$ | $$$$/| $$$$$$$ | | |_______/ \_______/|__/ \_______/ \___/ \____ $$ | | /$$ | $$ | | | $$$$$$/ | | by pyup.io \______/ | | | +==============================================================================+ | REPORT | | checked 5 packages, using free DB (updated once a month) | +============================+===========+==========================+==========+ | package | installed | affected | ID | +============================+===========+==========================+==========+ | kubernetes | 23.3.0 | >0 | 45114 | +==============================================================================+ | Kubernetes (python client) uses Kubernetes API, which has an unfixed | | vulnerability, CVE-2021-29923: Go before 1.17 does not properly consider | | extraneous zero characters at the beginning of an IP address octet, which | | (in some situations) allows attackers to bypass access control that is based | | on IP addresses, because of unexpected octal interpretation. This affects | | net.ParseIP and net.ParseCIDR. Kubernetes interprets leading zeros on IPv4 | | addresses as decimal to keep backwards compatibility, but users relying on | | parser alignment will be impacted by this CVE. | | kubernetes/kubernetes#104368 | | kubernetes/kubernetes#108074 | +==============================================================================+ ```
/kind bug
/kind cleanup
What this PR does / why we need it:
Since golang 1.17, golang/go#30999, "In both net.ParseIP and net.ParseCIDR reject leading zeros in the dot-decimal notation of IPv4 addresses."
This can cause that previous valid data becomes invalid, so we should guarantee that this doesn't happen.
In addition, since this change in the golang stdlib as associated a security CVE-2021-29923, we should check
Which issue(s) this PR fixes:
Fixes #100895
Additional information
I've tried to use the golangci-lint integration, but that depends on the gocritic linter that is the one embedding ruleguard.
This makes this more brittle and more complicated than just installing ruleguard and add our own rules.
It's important to mention that ruleguard rules require to import
github.com/quasilyte/go-ruleguard/dsl
that has a BSD-3 License