e2e test for CVE-2021-29923

[aojea]

The e2e test checks that the component implementing Kubernetes Services
interprets ClusterIPs with leading zeros as decimal, otherwise the
cluster will be exposed to CVE-2021-29923.

This also verifies the validation for Services.

The other objects that are susceptible to be vulnerable are Endpoints and EndpointSlices, but they commonly are auto generated, that makes it harder to test, so this test is a necessary but not sufficient condition.

/kind cleanup

added an e2e test to verify that the cluster is not vulnerable to CVE-2021-29923 when using Services with IPs with leading zeros, note that this test is a necessary but not sufficient condition, all the components in the clusters that consume IPs addresses from the APIs MUST interpret them as decimal or discard them.

Ref #100895

[k8s-ci-robot]

@aojea: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[aojea]

/sig network
/sig api-machinery
/assign @sttts @thockin @liggitt
/hold
I want to run it downstream too to verify I'm not doing wrong assumptions

[aojea]

nc -v -t -w 2 funny-ip 80
nc: getaddrinfo: Name does not resolve
command terminated with exit code 1

😅 I have to confirm if is coredns that is discarding the IP with leading zeros @chrisohaver

[aojea]

Indeed coredns doesn't consider the ips valid ...
https://github.com/coredns/coredns/blob/5b9b079dabc7f71463cea3f0c6a92f338935039d/plugin/kubernetes/ns.go#L50

this is going to be a chaos, hopefully nobody is using these addresses 🙃

-01-14T12:32:42.138527975Z stdout F [INFO] plugin/reload: Running configuration MD5 = db32ca3650231d74073ff4cf814959a7
2022-01-14T12:32:42.138546162Z stdout F CoreDNS-1.8.6
2022-01-14T12:32:42.138552199Z stdout F linux/amd64, go1.17.1, 13a9191

[chrisohaver]

nc -v -t -w 2 funny-ip 80
nc: getaddrinfo: Name does not resolve
command terminated with exit code 1

😅 I have to confirm if is coredns that is discarding the IP with leading zeros @chrisohaver

Any CoreDnS release compiled with Go 1.17 will treat IPv4 with leading zeros as invalid. We started compiling releases with Go 1.17 in 1.8.5.

From Go 1.17 Docs: https://golang.org/doc/go1.17#minor_library_changes

The ParseIP and ParseCIDR functions now reject IPv4 addresses which contain decimal components with leading zeros. These components were always interpreted as decimal, but some operating systems treat them as octal. This mismatch could hypothetically lead to security issues if a Go application was used to validate IP addresses which were then used in their original form with non-Go applications which interpreted components as octal. Generally, it is advisable to always re-encode values after validation, which avoids this class of parser misalignment issues.

[aojea]

Any CoreDnS release compiled with Go 1.17 will treat IPv4 with leading zeros as invalid. We started compiling releases with Go 1.17 in 1.8.5.

yeah, thanks for confirming, the good side is that users will be forced to update the IPs with leading zeros, despite the data being valid in kubernetes, the users will not be able to have a fully working cluster because DNS will not work ... if we add the rest of the networking ecosystem Ingresses, CNI plugins, ... that most probably are doing the same, we can think in removing the custom parsers after a few releases

[aojea]

I've modified the test to fail only if it can't connect to the IP in decimal format but it can against the IP interpreted in octal format, that is just what the CVE describes

[aojea]

/retest
this is working now

[aojea]

k8s.io/kubernetes/pkg/controller/statefulset: TestStatefulSetControlRollingUpdateWithPartition/SetDeleteOnly/StatefulSetAutoDeletePVCEnabled expand_more 1s
k8s.io/kubernetes/pkg/controller/statefulset: TestStatefulSetControlRollingUpdateWithPartition expand_more

/test pull-kubernetes-unit

[aojea]

passing in openshift too
https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_kubernetes/1117/pull-ci-openshift-kubernetes-master-k8s-e2e-gcp/1483354245670375424

/hold cancel

[fedebongio]

/remove-sig api-machinery

[thockin]

/lgtm
/approve

Are we going to let this go forever? It seems so brittle.

We know that rejecting these would likely break stored data.

We know that canonicalizing these could cause apply-loop problems.

We know that not doing anything is brittle.

Deciding not to decide is terrible. What if started canonicalizing IP strings. In general we don't want to do this, but I think the risk justifies it here. If we were to do this, would we also canonicalize IPv6 addresses?

[thockin]

nit: s/binary/octal

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, thockin

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:

• test/e2e/network/OWNERS [aojea,thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[aojea]

Are we going to let this go forever? It seems so brittle

I vote to wait until golang 1.16 is deprecated, check that no other project used the sloppy parsers and start a one year warning that the ips with zeros will be invalid.

The data can be valid, but will be practicallly unusable , at minimum DNS will now work #107552 (comment)

[thockin]

How does golang 1.16 really relate to this? It doesn't change existing saved data or clients (not all of which are Go, of course).

…

On Tue, Feb 8, 2022 at 11:19 AM Antonio Ojea ***@***.***> wrote: Are we going to let this go forever? It seems so brittle I vote to wait until golang 1.16 is deprecated, check that no other project used the sloppy parsers and start a one year warning that the ips with zeros will be invalid. The data can be valid, but will be practicallly unusable , at minimum DNS will now work #107552 (comment) <#107552 (comment)> — Reply to this email directly, view it on GitHub <#107552 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVBP35ZTXB6VIGR7DOTU2FUDPANCNFSM5L6UZQRA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were assigned.Message ID: ***@***.***>

[aojea]

How does golang 1.16 really relate to this? It doesn't change existing
saved data or clients (not all of which are Go, of course).

I was looking what APIs were using the parsers, and I found these:

kubernetes/test/integration/apiserver/cve_2021_29923_test.go

Lines 56 to 64 in df53ae8

	gvr("", "v1", "nodes"): `{"kind": "Node", "apiVersion": "v1", "metadata": {"name": "node1"}, "spec": {"unschedulable": true}, "status": {"addresses":[{"address":"172.18.0.012","type":"InternalIP"}]}}`,
	gvr("", "v1", "pods"): `{"kind": "Pod", "apiVersion": "v1", "metadata": {"name": "pod1", "namespace": "test-cve-2021-29923"}, "spec": {"containers": [{"image": "` + "image" + `", "name": "container7", "resources": {"limits": {"cpu": "1M"}, "requests": {"cpu": "1M"}}}]}, "status": {"podIP":"10.244.0.05","podIPs":[{"ip":"10.244.0.05"}]}}`,
	gvr("", "v1", "services"): `{"kind": "Service", "apiVersion": "v1", "metadata": {"name": "service1", "namespace": "test-cve-2021-29923"}, "spec": {"clusterIP": "10.0.0.011", "externalIP": "192.168.0.012", "externalName": "service1name", "ports": [{"port": 10000, "targetPort": 11000}], "selector": {"test": "data"}}}`,
	gvr("", "v1", "endpoints"): `{"kind": "Endpoints", "apiVersion": "v1", "metadata": {"name": "ep1name", "namespace": "test-cve-2021-29923"}, "subsets": [{"addresses": [{"hostname": "bar-001", "ip": "192.168.3.011"}], "ports": [{"port": 8000}]}]}`,
	// k8s.io/kubernetes/pkg/apis/discovery/v1
	gvr("discovery.k8s.io", "v1", "endpointslices"): `{"kind": "EndpointSlice", "apiVersion": "discovery.k8s.io/v1", "metadata": {"name": "slicev1", "namespace": "test-cve-2021-29923"}, "addressType": "IPv4", "protocol": "TCP", "ports": [], "endpoints": [{"addresses": ["10.244.0.011"], "conditions": {"ready": true, "serving": true, "terminating": false}, "nodeName": "control-plane"}]}`,
	// k8s.io/kubernetes/pkg/apis/networking/v1
	gvr("networking.k8s.io", "v1", "ingresses"): `{"kind": "Ingress", "apiVersion": "networking.k8s.io/v1", "metadata": {"name": "ingress3", "namespace": "test-cve-2021-29923"}, "spec": {"defaultBackend": {"service":{"name":"service", "port":{"number": 5000}}}}, "status":{"loadBalancer":{"ingress": [{"ip":"10.0.0.013"}]}}}`,
	gvr("networking.k8s.io", "v1", "networkpolicies"): `{"kind": "NetworkPolicy", "apiVersion": "networking.k8s.io/v1", "metadata": {"name": "np2", "namespace": "test-cve-2021-29923"}, "spec": {"egress":[{"ports":[{"port":5978,"protocol":"TCP"}],"to":[{"ipBlock":{"cidr":"10.0.012.0/24"}}]}],"ingress":[{"from":[{"ipBlock":{"cidr":"172.017.0.0/16","except":["172.17.001.0/24"]}},{"podSelector":{"matchLabels":{"role":"frontend"}}}],"ports":[{"port":6379,"protocol":"TCP"}]}],"podSelector":{"matchLabels":{"role":"db"}},"policyTypes":["Ingress","Egress"]}}`,

The data is not going to change, but people using that data will have to migrate it or DNS will not work, per example,

#107552 (comment)

and my observation is that all projects are upgrading to 1.17 choosing the new parsers and not to drop the IPs with leading zeros, so is just not DNS, is most of the networking features.

gardener/gardener#4822
antrea-io/antrea#3189
ovn-org/ovn-kubernetes#2784
cilium/cilium#17190
projectcalico/calico#5285

Actually, it seems that only openshift and kubernetes upstream are using the sloppy parsers

https://grep.app/search?q=ParseIPSloppy&case=true

I think that being strict about data compatibility is right, but in this case, is really unlikely that people is using IPs with leading zeros, but in case there is someone, it is more unlikely that those IPs keep working after go 1.16 is deprecated.

I've also made a tool to detect this IPs on etcd dumps, https://github.com/aojea/funny-ip-etcd-detector

I think that an exception can be made for this, with a proper announcement and having a reasonable period to minimize the risk, so we can move to the stdlib parsers.

[k8s-triage-robot]

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

• The PR does have any do-not-merge/* labels
• The PR does not have the needs-ok-to-test label
• The PR is mergeable (does not have a needs-rebase label)
• The PR is approved (has cncf-cla: yes, lgtm, approved labels)
• The PR is failing tests required for merge

You can:

• Review the full test history for this PR
• Prevent this bot from retesting with /lgtm cancel or /hold
• Help make our tests less flaky by following our Flaky Tests Guide

/retest

[thockin]

The data is not going to change, but people using that data will have to migrate it or DNS will not work, per example,

So we just let cordns take the blame for a breaking change?

#107552 (comment) <#107552 (comment)> and my observation is that all projects are upgrading to 1.17 choosing the new parsers and not to drop the IPs with leading zeros, so is just not DNS, is most of the networking features. gardener/gardener#4822 <gardener/gardener#4822> antrea-io/antrea#3189 <antrea-io/antrea#3189> ovn-org/ovn-kubernetes#2784 <ovn-org/ovn-kubernetes#2784> cilium/cilium#17190 <cilium/cilium#17190> projectcalico/calico#5285 <projectcalico/calico#5285> Actually, it seems that only openshift and kubernetes upstream are using the sloppy parsers https://grep.app/search?q=ParseIPSloppy&case=true

I'm torn. It's wrong but so is breaking users.

I think that being strict about data compatibility is right, but in this case, is really unlikely that people is using IPs with leading zeros, but in case there is someone, it is more unlikely that those IPs keep working after go 1.16 is deprecated.

...because we are the only ones being careful about compat...

I think that an exception can be made for this, with a proper announcement and having a reasonable period to minimize the risk, so we can move to the stdlib parsers.

I don't think we should break saved data. 1) Maybe we can stop allowing creates with broken IPs. Probably. But we will need warnings first. 2) Maybe we can stop allowing updates to broken IPs. Not sure, but perhaps. 3) Maybe we throw a condition or something when we read a broken IP. 4) Maybe canonicalize on read. It could cause apply-loops but it won't break existing data Maybe we should do (3) - log these, add a metric, set a condition - for a release or 2. Then implement (1) and (2) in addition. Then finally implement (4). It would require keeping the sloppy parsing logic at least in the read-path.

[aojea]

...because we are the only ones being careful about compat...

I don't think that we are the only that care about compat, golang decided to break this compat for a good reason, and is to avoid exposing users to this problem of parsers misalignment, and so other projects are doing.

I don't think we should break saved data.

I always think that the compat promise is a contract and as any contract in negotiable, and in this case I can see that with enough notice is a reasonable break with a very minimum risk.

My point is that I argue this saved data is valid, since as we can see, you are rolling a dice, depending on the consumer of the data, the IP can mean one thing or the other, and we are breaking for a good reasoning, avoiding users to be exposed to a very subtle security issue.

The big problem here is not the api machinery, if we know "these are the fields" we can do any of the solutions you propose, but there is no "IP" field or an easy way to track what objects and fields are affected, you can have IPs in any string field, and we can end with some IPs allow zeros and others not 🤷

[thockin]

Just put yourself in the place of a cluster user whose stuff works on Monday, and stops working on Tuesday. You'd be in a panic. Now you find out "Kubernetes changed some policy" and the only place it was documented was in the cluster's release notes (which, as a user, I probably don't read). How unhappy are you?

if we know "these are the fields" we can do any of the solutions you propose

You're not wrong, but I still think we should try, at least for all the things we know about. How about something like:

1. Any new builtin APIs should use ParseIP/ParseCIDR (not sloppy) and we let validation fail.
2. For the places we know of (service, ingress, netpol, pod, node, ...) we set a condition like "AmbiguousAddressSyntax", log them, maybe a metric.
3. Allow updates to otherwise immutable fields as long as the IP adress is changing from a malformed value to a correct value.
4. Ship that in 1.24.
5. Warn that in 1.25 or 1.26 such IPs will be tolerated if they exist, but rejected for new usage.
6. Implement (5)
7. Warn that in N+1 or N+2 we will fix such IPs upon read, effectively purging them from k8s
8. Implement (7) and ship
9. Maybe put these updates in a repair loop, ship that, then eventually remove sloppy parsing entirely

[aojea]

Sounds like a good plan 👍 ... I'm in