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.

Sign up for GitHub

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

Jump to bottom

golang 1.17 fails to parse IPs with leading zeros #104368

Merged
merged 5 commits into from Aug 20, 2021
Merged

golang 1.17 fails to parse IPs with leading zeros #104368

merged 5 commits into from Aug 20, 2021

Conversation

aojea
Copy link
Member

@aojea aojea commented Aug 14, 2021 •
edited

/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

While you triage those callsites, it would be good to also check if they are affected by the kind of issues that are motivating the change: if you are validating the inputs with Go and then passing the inputs to the OS or to non-Go applications, the two might disagree on the inputs validity or meaning. This can cause issues, which we would love to hear about at security@golang.org or on the issue, to assess the security risk.

Which issue(s) this PR fixes:

Fixes #100895

Since golang 1.17 both net.ParseIP and net.ParseCIDR rejects leading zeros in the dot-decimal notation of IPv4 addresses.
Kubernetes will keep allowing leading zeros on IPv4 address to not break the compatibility.
IMPORTANT: Kubernetes interprets leading zeros on IPv4 addresses as decimal, users must not rely on parser alignment to not being impacted by the associated security advisory:
CVE-2021-29923 golang standard library "net" - Improper Input Validation of octal literals in golang 1.16.2 and below standard library "net" results in indeterminate SSRF & RFI vulnerabilities.
Reference: https://nvd.nist.gov/vuln/detail/CVE-2021-29923

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

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. release-note Denotes a PR that will be considered when it comes time to generate release notes. area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 14, 2021
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 15, 2021
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 15, 2021
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/dependency Issues or PRs related to dependency changes and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 15, 2021
@pacoxu
Copy link
Member

pacoxu commented Aug 16, 2021

/kind bug
/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Aug 16, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2021
@aojea
Copy link
Member Author

aojea commented Aug 20, 2021 

=== RUN   TestVolumeUnmountAndDetachControllerDisabled
W0820 09:05:54.402158   62913 mutation_detector.go:53] Mutation detector is enabled, this will result in memory leakage.
I0820 09:05:54.402758   62913 volume_manager.go:291] "Starting Kubelet Volume Manager"
I0820 09:05:54.402955   62913 desired_state_of_world_populator.go:146] "Desired state populator starts to run"
E0820 09:05:54.405877   62913 reflector.go:138] k8s.io/client-go/informers/factory.go:134: Failed to watch *v1.CSIDriver: unhandled watch: testing.WatchActionImpl{ActionImpl:testing.ActionImpl{Namespace:"", Verb:"watch", Resource:schema.GroupVersionResource{Group:"storage.k8s.io", Version:"v1", Resource:"csidrivers"}, Subresource:""}, WatchRestrictions:testing.WatchRestrictions{Labels:labels.internalSelector(nil), Fields:fields.andTerm{}, ResourceVersion:""}}
I0820 09:05:54.604805   62913 reconciler.go:244] "operationExecutor.AttachVolume started for volume \"vol1\" (UniqueName: \"fake/fake-device\") pod \"foo\" (UID: \"12345678\") " pod="test/foo"
I0820 09:05:54.604984   62913 reconciler.go:157] "Reconciler: start to sync state"
I0820 09:05:54.605064   62913 operation_generator.go:369] AttachVolume.Attach succeeded for volume "vol1" (UniqueName: "fake/fake-device") from node "127.0.0.1" 
I0820 09:05:54.706173   62913 operation_generator.go:587] "MountVolume.WaitForAttach entering for volume \"vol1\" (UniqueName: \"fake/fake-device\") pod \"foo\" (UID: \"12345678\") DevicePath \"/dev/vdb-test\"" pod="test/foo"
I0820 09:05:54.706461   62913 operation_generator.go:597] "MountVolume.WaitForAttach succeeded for volume \"vol1\" (UniqueName: \"fake/fake-device\") pod \"foo\" (UID: \"12345678\") DevicePath \"/dev/sdb\"" pod="test/foo"
I0820 09:05:54.706715   62913 operation_generator.go:630] "MountVolume.MountDevice succeeded for volume \"vol1\" (UniqueName: \"fake/fake-device\") pod \"foo\" (UID: \"12345678\") device mount path \"\"" pod="test/foo"
==================
WARNING: DATA RACE
Write at 0x00c0004d6a68 by goroutine 20:
  k8s.io/kubernetes/pkg/kubelet.TestVolumeUnmountAndDetachControllerDisabled()
      /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/kubelet/kubelet_volumes_test.go:319 +0xc73
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1193 +0x202

Previous read at 0x00c0004d6a68 by goroutine 104:
  k8s.io/kubernetes/pkg/kubelet.(*fakePodWorkers).ShouldPodRuntimeBeRemoved()
      /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernete

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/104368/pull-kubernetes-unit/1428640022386446336
/test pull-kubernetes-unit

@aojea
Copy link
Member Author

aojea commented Aug 20, 2021

/hold
#104368 (comment)

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2021
@k8s-ci-robot
Copy link
Contributor

@aojea: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
check-dependency-stats 0bc360821ecaea72cc295acb4541b71226b6e389 link /test check-dependency-stats
pull-kubernetes-node-kubelet-serial-containerd 2c73d78 link /test pull-kubernetes-node-kubelet-serial-containerd
pull-kubernetes-node-kubelet-serial 2c73d78 link /test pull-kubernetes-node-kubelet-serial
pull-kubernetes-e2e-aks-engine-windows-containerd 2c73d78 link /test pull-kubernetes-e2e-aks-engine-windows-containerd

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.

@liggitt
Copy link
Member

liggitt commented Aug 20, 2021

/skip
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2021
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2021
@k8s-ci-robot k8s-ci-robot merged commit b0bc8ad into kubernetes:master Aug 20, 2021
SIG Node CI/Test Board automation moved this from Archive-it to Done Aug 20, 2021
SIG Auth Old automation moved this from Needs Triage to Closed / Done Aug 20, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Aug 20, 2021
@thockin
Copy link
Member

thockin commented Aug 21, 2021

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
b) symlink EVERYTHING from staging into vendor (which seems to be true today) and just rely on that

@caesarxuchao
Copy link
Member

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 24, 2021
This was referenced Aug 26, 2021
Closed
Merged
@openshift-ci-robot openshift-ci-robot mentioned this pull request Sep 20, 2021
Closed
@spiffxp spiffxp mentioned this pull request Sep 27, 2021
Closed
@phoracek phoracek mentioned this pull request Oct 1, 2021
Closed
@ialidzhikov ialidzhikov mentioned this pull request Oct 13, 2021
Merged
2 tasks
@aojea aojea mentioned this pull request Jan 26, 2022
Merged
@aojea aojea mentioned this pull request Feb 11, 2022
Open
sbrunner added a commit to camptocamp/operator-shared-config-manager that referenced this pull request Apr 6, 2022
@sbrunner
Ignore CVE
43c8dfc 
```
  +==============================================================================+
  |                                                                              |
  |                               /$$$$$$            /$$                         |
  |                              /$$__  $$          | $$                         |
  |           /$$$$$$$  /$$$$$$ | $$  \__//$$$$$$  /$$$$$$   /$$   /$$           |
  |          /$$_____/ |____  $$| $$$$   /$$__  $$|_  $$_/  | $$  | $$           |
  |         |  $$$$$$   /$$$$$$$| $$_/  | $$$$$$$$  | $$    | $$  | $$           |
  |          \____  $$ /$$__  $$| $$    | $$_____/  | $$ /$$| $$  | $$           |
  |          /$$$$$$$/|  $$$$$$$| $$    |  $$$$$$$  |  $$$$/|  $$$$$$$           |
  |         |_______/  \_______/|__/     \_______/   \___/   \____  $$           |
  |                                                          /$$  | $$           |
  |                                                         |  $$$$$$/           |
  |  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                       |
  +==============================================================================+
```
sbrunner added a commit to camptocamp/operator-shared-config-manager that referenced this pull request Apr 7, 2022
@sbrunner
Ignore CVE 45114
77f3662 
```
  +==============================================================================+
  |                                                                              |
  |                               /$$$$$$            /$$                         |
  |                              /$$__  $$          | $$                         |
  |           /$$$$$$$  /$$$$$$ | $$  \__//$$$$$$  /$$$$$$   /$$   /$$           |
  |          /$$_____/ |____  $$| $$$$   /$$__  $$|_  $$_/  | $$  | $$           |
  |         |  $$$$$$   /$$$$$$$| $$_/  | $$$$$$$$  | $$    | $$  | $$           |
  |          \____  $$ /$$__  $$| $$    | $$_____/  | $$ /$$| $$  | $$           |
  |          /$$$$$$$/|  $$$$$$$| $$    |  $$$$$$$  |  $$$$/|  $$$$$$$           |
  |         |_______/  \_______/|__/     \_______/   \___/   \____  $$           |
  |                                                          /$$  | $$           |
  |                                                         |  $$$$$$/           |
  |  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                       |
  +==============================================================================+
```
@aojea aojea mentioned this pull request Nov 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@liggitt liggitt liggitt left review comments

@logicalhan logicalhan Awaiting requested review from logicalhan

@mkumatag mkumatag Awaiting requested review from mkumatag

Assignees

@dims dims

@spiffxp spiffxp

@BenTheElder BenTheElder

@liggitt liggitt

@thockin thockin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/code-generation area/dependency Issues or PRs related to dependency changes area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/ipvs area/kubeadm area/kubectl area/kubelet area/network-policy Issues or PRs related to Network Policy subproject area/provider/gcp Issues or PRs related to gcp provider area/release-eng Issues or PRs related to the Release Engineering subproject area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Auth
Archived in project
SIG Node CI/Test Board
Done
SIG Node CI/Test Board
Archived in project
SIG Auth Old
Closed / Done
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

[go1.17] Guard against stdlib ParseIP/ParseCIDR changes in API validation