Deprecate IPs with Leading Zeros ( CVE-2021-29923 )

[aojea]

To summarize the situation:

1. Since golang 1.17 IPv4 addresses with leading zeros are considered invalid. [go1.17] Guard against stdlib ParseIP/ParseCIDR changes in API validation #100895
2. Kubernetes decided to keep compatibility with stored data so it forked previous parsers to allow IPs with leading zeros. golang 1.17 fails to parse IPs with leading zeros #104368
3. The rest of the Kubernetes ecosystem seems to prefer to use default parsers and consider that data invalid e2e test for CVE-2021-29923 #107552 (comment)

We are in a situation where users can have IPs with leading zeros in their API objects, but may not be able to use it because the CNI, DNS, Ingress, ... implementation doesn't consider it valid

The proposal is (quoting thockin)

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

Originally posted by @thockin in #107552 (comment)

[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
/priority important-soon
/assign @thockin @aojea

[aojea]

requesting input from @liggitt about the proposal

I think that this are the only places that use the parsers for validation

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"]}}`,

[liggitt]

not sure of all the types it propagates into, but these are the call sites I'd chase to APIs to understand exposure:

git grep -n Sloppy | egrep -v 'test|config|kubeadm' | egrep 'kube-openapi|validation|/api/|/apis/'
pkg/apis/core/validation/validation.go:3221:			if ip := netutils.ParseIPSloppy(ns); ip == nil {
pkg/apis/core/validation/validation.go:3353:		if ip := netutils.ParseIPSloppy(hostAlias.IP); ip == nil {
pkg/apis/core/validation/validation.go:6100:	ip := netutils.ParseIPSloppy(ipAddress)
pkg/apis/core/validation/validation.go:6440:			if isIP := (netutils.ParseIPSloppy(ingress.IP) != nil); !isIP {
pkg/apis/core/validation/validation.go:6448:			if isIP := (netutils.ParseIPSloppy(ingress.Hostname) != nil); isIP {
pkg/apis/core/validation/validation.go:6478:	_, net, err := netutils.ParseCIDRSloppy(cidr)
pkg/apis/networking/validation/validation.go:328:			if isIP := (netutils.ParseIPSloppy(ih.Host) != nil); isIP {
staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go:350:	if netutils.ParseIPSloppy(value) == nil {
staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go:359:	ip := netutils.ParseIPSloppy(value)
staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go:369:	ip := netutils.ParseIPSloppy(value)
vendor/k8s.io/kube-openapi/pkg/validation/strfmt/default.go:212:	ip := netutils.ParseIPSloppy(s)
vendor/k8s.io/kube-openapi/pkg/validation/strfmt/default.go:218:	_, _, err := netutils.ParseCIDRSloppy(s)

[liggitt]

1. Any new builtin APIs should use ParseIP/ParseCIDR (not sloppy) and we let validation fail.

Yes

2. For the places we know of (service, ingress, netpol, pod, node, ...) we set a condition like "AmbiguousAddressSyntax", log them, maybe a metric.

log/metric (and even warning) seem ok... less sure about writing to the API object

3. Allow updates to otherwise immutable fields as long as the IP adress is changing from a malformed value to a correct value.

Probably ok, though can get pretty messy (pod spec and service immutability on update are particularly messy)

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)

Can use the existing pattern of plumbing validation opts into validation, and setting whether to tolerate based on the presence of leading-zero IPs/CIDRs in the existing object

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

I'm less certain about fix-on-read after the experience in #107847 (comment)

[thockin]

I'm less certain about fix-on-read after the experience in #107847

But that's all patched up now :)

[danwinship]

To summarize the situation:

can you add:

0. Before golang 1.17, IPv4 addresses with leading zeros were parsed as though the zeros weren't there (ie, the numbers were still parsed as decimal, not octal like the C APIs would do)

[danwinship]

Any new builtin APIs should use ParseIP/ParseCIDR (not sloppy) and we let validation fail.

They should use the non-sloppy functions in their validation functions but for now we should probably still require people to use the sloppy functions everywhere else, just for consistency / not needing to think about it. (If the field has been validated to have no leading zeros then it's guaranteed that ParseIP() and ParseIPSloppy() will give the same results later on, so using the sloppy function on "non-sloppy" data is fine.)

Longer term this ties into the whole "migrating to new golang 1.18 IP type/APIs" thing

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[danwinship]

Longer term this ties into the whole "migrating to new golang 1.18 IP type/APIs" thing

https://pkg.go.dev/net/netip

[aojea]

Longer term this ties into the whole "migrating to new golang 1.18 IP type/APIs" thing

https://pkg.go.dev/net/netip

#106161

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[aojea]

I'm working for migrating Services IPs as part of
https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1880-multiple-service-cidrs

[danwinship]

Wondering if maybe we need a KEP here, but...

1. There was clear consensus that we had to do something to keep accepting bad IP values already in etcd, but less consensus on bad values in other places:
   • Do we want to / are we allowed to tighten validation for IP/CIDR fields in all objects? If someone has a controller that tries to create Services with clusterIP values like 172.030.000.010, are we OK with that controller breaking in some future release due to stronger validation? (I think consensus was that normally we would not do this, but since there are security implications, we're probably OK doing it here?)
   • What about for IPs outside of API fields? If someone is launching kube-apiserver with --service-cluster-ip-range 172.030.000.000/16, are we OK with that suddenly resulting in kube-apiserver failing to start after some future upgrade?
2. The CVE / sloppy-parsing-fire-drill was just about IPs with leading 0s, but there are other edge cases:
   • Rejecting IPv6-wrapped IPv4 addresses (eg ::ffff:1.2.3.4), which shoud be treated as IPv4 but may be misinterpreted by some code (potentially leading to the same sorts of "smuggling bad values past sanity checks" problems as with leading 0s).
   • Rejecting CIDR values that have bits set after the mask length, which may be interpreted as IPs rather than subnets by some software (fix documentation and maybe validation of NetworkPolicy CIDRs #94484)
   • Requiring IPv6 addresses to appear in canonical form (eg 1234::abcd, not 1234:0:0:0:0:0:0:ABCD) to prevent reapply loops in controllers (eg Services API: warnings on IP addresses #114505 warns about this). (This is only a problem for IPv6; any IPv4 address that is valid according to the new rules is also canonical.)
   • Some API objects are currently validated with ValidateNonSpecialIP which rejects loopback, link-local, etc, and we may want to do that more broadly?
3. Presumably we want to make sure that CRD validation of IPs/CIDRs gets updated to match apiserver validation. We may have less ability to phase in changes over time here.
4. (Random thing to keep in mind: netip.ParseAddr accepts some strings that net.ParseIP does not (eg fe80::1234%eth1) so we need to be careful to not accidentally change our validation semantics when we switch to that.)

[aojea]

omg

kubectl get service echoserver -o yaml
apiVersion: v1
kind: Service
metadata:
  annotations:
    cloud.google.com/neg: '{"ingress":true}'
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"v1","kind":"Service","metadata":{"annotations":{},"name":"echoserver","namespace":"default"},"spec":{"clusterIP":"::ffff:10.52.0.99","ports":[{"port":80,"protocol":"TCP","targetPort":80}],"selector":{"app":"echoserver"},"type":"ClusterIP"}}
  creationTimestamp: "2023-06-15T13:08:50Z"
  name: echoserver
  namespace: default
  resourceVersion: "15473859"
  uid: 86cadfcb-8fa4-4af1-9e24-aeebbba1a574
spec:
  clusterIP: ::ffff:10.52.0.99
  clusterIPs:
  - ::ffff:10.52.0.99

[thockin]

The question witht his proposal is whether we have the stomach to do step 6 - actually reject ill-formed IPs (in new usage) even when they USED TO work. Will it actually break anyone? It seems unlikely but that's not the bar.

[aojea]

/assign @danwinship