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

Port parsing can overflow (TOB-K8S-015: Overflows when using strconv.Atoi and downcasting the result) #81121

Closed
cji opened this issue Aug 8, 2019 · 26 comments

Comments

@cji
Copy link
Member

@cji cji commented Aug 8, 2019

This issue was reported in the Kubernetes Security Audit Report

Description
The strconv.Atoi function parses an int - a machine dependent integer type, which, for 64-bit targets will be int64. There are places throughout the codebase where the result returned from strconv.Atoi is later converted to a smaller type: int16 or int32. This may overflow with a certain input. An example of the issue has been included in Figure 1.

v, err := strconv.Atoi(options.DiskMBpsReadWrite)
diskMBpsReadWrite = int32(v)

Figure 13.1: pkg/cloudprovider/providers/azure/azure_managedDiskController.go:105

Additionally, there are many code paths that parse ports, and do so differently and in a manner lacking checks for a proper port range. An example of this has been identified within kubectl when handling port values.

Kubectl has the ability to expose particular Pod ports through the use of kubectl expose. This command uses the function updatePodPorts, which uses strconv.Atoi to parse a string into an integer, then downcasts it to an int32 (Figure 2).

// updatePodContainers updates PodSpec.Containers.Ports with passed parameters.
func updatePodPorts(params map[string]string, podSpec *v1.PodSpec) (err error) {
    port := -1
    hostPort := -1
    if len(params["port"]) > 0 {
        port, err = strconv.Atoi(params["port"]) // <-- this should parse port as strconv.ParseUint(params["port"], 10, 16)
        if err != nil {
            return err
        }
    }
       // (...)
    // Don't include the port if it was not specified.
    if len(params["port"]) > 0 {
        podSpec.Containers[0].Ports = []v1.ContainerPort{
            {
                ContainerPort: int32(port), // <-- this should later just be uint16(port)
            },
        }

Figure 13.2: Relevant snippet of the updatePodPorts function.

This error has been operationalized into a crash within kubectl when overflowing provided ports. Starting with a standard deployment with no services, we can observe the expected behavior (Figure 3).

root@k8s-1:~# cat nginx.yml
apiVersion: apps/v1 # for versions before 1.9.0 use apps/v1beta2
kind: Deployment
metadata:
  name: nginx-deployment
spec:
  selector:
    matchLabels:
      app: nginx
  replicas: 1 # tells deployment to run 2 pods matching the template
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - name: nginx
        image: nginx:1.7.9
        ports:
        - containerPort: 80
        
root@k8s-1:~# kubectl create -f nginx.yml
deployment.apps/nginx-deployment created

root@k8s-1:~# kubectl get pods
NAME                                READY   STATUS    RESTARTS   AGE
nginx-deployment-76bf4969df-nskjh   1/1     Running   0          2m14s

root@k8s-1:~# kubectl get services
NAME         TYPE        CLUSTER-IP   EXTERNAL-IP   PORT(S)   AGE
kubernetes   ClusterIP   10.233.0.1   <none>        443/TCP   30m

Figure 13.3: The deployment spec with service and Pod status.

To trigger the overflow, we can now update the deployment through the kubectl expose command with an overflown port, overflowing from 4294967377 to 81 (Figure 4).

root@k8s-1:/home/vagrant# kubectl expose deployment nginx-deployment --port 4294967377 --target-port 80
service/nginx-deployment exposed

Figure 13.4: Overflowing the port parameter.

We are now able to observe this overflown port when listing the services with kubectl get services (Figure 5). We are also able to access the service on the overflown port (Figure 6).

root@k8s-1:/home/vagrant# kubectl get services
NAME               TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)   AGE
kubernetes         ClusterIP   10.233.0.1      <none>        443/TCP   42m
nginx-deployment   ClusterIP   10.233.25.138   <none>        81/TCP    2s

Figure 13.5: The overflown port got exposed.

root@k8s-1:/home/vagrant# curl 10.233.25.138:81
<!DOCTYPE html>
<html>
<head>
<title>Welcome to nginx!</title>
...

Figure 13.6: The result of curling the overflown service port.

Furthering this issue, we are able to also overflow the target port. After deleting the service, we can attempt to overflow the target port as well, which will result in a panic in kubectl (Figure 7 and 8).

root@k8s-1:/home/vagrant# kubectl delete service nginx-deployment
service "nginx-deployment" deleted
root@k8s-1:/home/vagrant# kubectl get services
NAME         TYPE        CLUSTER-IP   EXTERNAL-IP   PORT(S)   AGE
kubernetes   ClusterIP   10.233.0.1   <none>        443/TCP   45m

Figure 13.7: The deletion of the deployment.

root@k8s-1:/home/vagrant# kubectl expose deployment nginx-deployment --port 4294967377 --target-port 4294967376
E0402 09:25:31.888983    3625 intstr.go:61] value: 4294967376 overflows int32
goroutine 1 [running]:
runtime/debug.Stack(0xc000e54eb8, 0xc4f1e9b8, 0xa3ce32e2a3d43b34)
	/usr/local/go/src/runtime/debug/stack.go:24 +0xa7
k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/intstr.FromInt(0x100000050, 0xa, 0x100000050, 0x0, 0x0)
...
service/nginx-deployment exposed

Figure 13.8: The panic in kubectl when overflowing the target port.

Despite the panic from kubectl (visible in Figure 8), the service is still exposed (Figure 9) and accessible (Figure 10).

root@k8s-1:/home/vagrant# kubectl get services
NAME               TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)   AGE
kubernetes         ClusterIP   10.233.0.1      <none>        443/TCP   46m
nginx-deployment   ClusterIP   10.233.59.190   <none>        81/TCP    35s

Figure 13.9: The service is exposed despite the kubectl panic and overflow.

root@k8s-1:/home/vagrant# curl 10.233.59.190:81
<!DOCTYPE html>
<html>
<head>
<title>Welcome to nginx!</title>
...

Figure 13.10: The service is also accessible after overflow.

Exploit Scenario
A value is parsed from a configuration file with Atoi, resulting in an integer. It is then downcasted to a lower precision value, resulting in a potential overflow or underflow which is not raised as an error or panic.

Recommendation
Short term, when parsing strings into fixed-width integer types, use strconv.ParseInt or strconv.ParseUint with appropriate bitSize argument instead of strconv.Atoi.

Long term, ensure the validity of data and types. Parse and validate values with common functions. For example the ParsePort (cmd/kubeadm/app/util/endpoint.go:117) utility function parses and validates TCP port values, but it is not well used across the codebase.

Anything else we need to know?:

See #81146 for current status of all issues created from these findings.

The vendor gave this issue an ID of TOB-K8S-015 and it was finding 13 of the report.

The vendor considers this issue Medium Severity.

To view the original finding, begin on page 42 of the Kubernetes Security Review Report

Environment:

  • Kubernetes version: 1.13.4
@joelsmith
Copy link
Contributor

@joelsmith joelsmith commented Aug 8, 2019

/sig network
/sig cli
/area security
/kind bug
/remove-kind feature

@liggitt liggitt changed the title Overflows when using strconv.Atoi and downcasting the result TOB-K8S-015: Overflows when using strconv.Atoi and downcasting the result Aug 8, 2019
@athenabot
Copy link

@athenabot athenabot commented Aug 8, 2019

/triage unresolved

Comment /remove-triage unresolved when the issue is assessed and confirmed.

🤖 I am a bot run by vllry. 👩‍🔬

@vllry
Copy link
Contributor

@vllry vllry commented Aug 8, 2019

/assign @thockin

@thockin
Copy link
Member

@thockin thockin commented Aug 8, 2019

Is there any static checker that can find such cases, ideally with a way to decorate audited sites? Something a la use taintin perl?

@thockin
Copy link
Member

@thockin thockin commented Aug 8, 2019

We should maybe move ParsePort() to k/utils/net and then use that everywhere.

@thockin thockin changed the title TOB-K8S-015: Overflows when using strconv.Atoi and downcasting the result Port parsing can overflow (TOB-K8S-015: Overflows when using strconv.Atoi and downcasting the result) Aug 8, 2019
@thockin
Copy link
Member

@thockin thockin commented Aug 8, 2019

I have re-titled this. I started the ball rolling. This is a great bug for someone who isn't super familiar with the codebase and wants to explore a bit.

The only hard part is re-vendoring k/utils and that is not hard.

@vegemitecheesetoast
Copy link

@vegemitecheesetoast vegemitecheesetoast commented Aug 8, 2019

I'd like to work on this. I'm someone who isn't super familiar with the codebase and wants to explore a bit.

@athenabot
Copy link

@athenabot athenabot commented Aug 8, 2019

/triage unresolved

Comment /remove-triage unresolved when the issue is assessed and confirmed.

🤖 I am a bot run by vllry. 👩‍🔬

@loqutus
Copy link
Contributor

@loqutus loqutus commented Aug 10, 2019

/assign

@elieser1101
Copy link

@elieser1101 elieser1101 commented Aug 10, 2019

i would like to work on this, let me know if you still need help!

@thockin
Copy link
Member

@thockin thockin commented Aug 10, 2019

@elieser1101
Copy link

@elieser1101 elieser1101 commented Aug 10, 2019

@thockin got it. So thanks, i am able to start contributing but would like to take some good-first-issues as im not to familiar with the code base!! any recommendation on simple issues would be great!

@vegemitecheesetoast
Copy link

@vegemitecheesetoast vegemitecheesetoast commented Aug 11, 2019

vegemitecheesetoast@a058bf9

This is my commit with just a few files just to see if I'm on the right track. Would greatly appreciate if someone can check this out and steer me in the right direction if need be.

@nicowaisman
Copy link

@nicowaisman nicowaisman commented Aug 12, 2019

I run a little variant analysis of this bug class on kubernetes and all their staging packages and I thought you might find the result useful:

"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/apimachinery/revision-2019-August-10--09-40-29/src/pkg/util/intstr/intstr.go@63:46-63:48"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/csi-translation/revision-2019-August-10--09-41-28/src/plugins/aws_ebs.go@132:31-132:39"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/csi-translation/revision-2019-August-10--09-41-28/src/plugins/gce_pd.go@296:31-296:37"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/kubectl/revision-2019-August-10--09-41-42/src/pkg/cmd/portforward/portforward.go@181:85-181:91"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/kubectl/revision-2019-August-10--09-41-42/src/pkg/cmd/portforward/portforward.go@187:12-187:18"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/kubectl/revision-2019-August-10--09-41-42/src/pkg/cmd/rollingupdate/rolling_updater.go@204:19-204:35"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/kubectl/revision-2019-August-10--09-41-42/src/pkg/generate/versioned/run.go@106:19-106:23"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/kubectl/revision-2019-August-10--09-41-42/src/pkg/generate/versioned/run.go@194:19-194:23"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/kubectl/revision-2019-August-10--09-41-42/src/pkg/generate/versioned/run.go@282:19-282:23"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/kubectl/revision-2019-August-10--09-41-42/src/pkg/generate/versioned/run.go@817:19-817:23"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/kubectl/revision-2019-August-10--09-41-42/src/pkg/generate/versioned/run.go@887:26-887:29"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/kubectl/revision-2019-August-10--09-41-42/src/pkg/generate/versioned/run.go@891:52-891:59"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/kubectl/revision-2019-August-10--09-41-42/src/pkg/generate/versioned/service.go@174:21-174:24"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/kubectl/revision-2019-August-10--09-41-42/src/pkg/generate/versioned/service_basic.go@99:16-99:19"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/kubectl/revision-2019-August-10--09-41-42/src/pkg/generate/versioned/service_basic.go@114:15-114:18"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/legacy-cloud-providers/revision-2019-August-10--09-42-05/src/azure/azure_loadbalancer.go@545:16-545:17"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/staging/legacy-cloud-providers/revision-2019-August-10--09-42-05/src/azure/azure_managedDiskController.go@118:30-118:30"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/kubernetes/revision-2019-August-06--15-29-05/src/pkg/controller/deployment/util/deployment_util.go@394:15-394:22"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/kubernetes/revision-2019-August-06--15-29-05/src/pkg/controller/deployment/util/deployment_util.go@855:9-855:15"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/kubernetes/revision-2019-August-06--15-29-05/src/pkg/proxy/ipvs/proxier.go@1710:20-1710:26"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/kubernetes/revision-2019-August-06--15-29-05/src/pkg/proxy/ipvs/proxier.go@1753:20-1753:26"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/kubernetes/revision-2019-August-06--15-29-05/src/pkg/volume/fc/fc_util.go@171:34-171:36"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/kubernetes/revision-2019-August-06--15-29-05/src/pkg/volume/iscsi/iscsi.go@253:27-253:29"
"call to Atoi","Atoi() used in combination of a int wrapper could cause overflow - ","/home/nico/Semmle/Projects/Kubernetes/kubernetes/revision-2019-August-06--15-29-05/src/pkg/volume/iscsi/iscsi.go@659:26-659:28"

@vegemitecheesetoast
Copy link

@vegemitecheesetoast vegemitecheesetoast commented Aug 13, 2019

thockin@ let me know that we've added a util function for this: kubernetes/utils#107

Step one is to re-vendor this lib and then use that function:

https://github.com/kubernetes/community/blob/9258fa171f569a33b62a07239c10bce137598527/contributors/devel/sig-architecture/vendor.md#adding-or-updating-a-dependency

I'm still trying to work out what re-vendoring is and figure out how to do this. Will report back in a few days when I get some progress.

@nicowaisman
Copy link

@nicowaisman nicowaisman commented Aug 14, 2019

Hey ya!
I found an interesting case in apimachinery
https://github.com/kubernetes/apimachinery/blob/master/pkg/api/resource/suffix.go#L190

		parsed, err := strconv.ParseInt(string(suffix[1:]), 10, 64)
		if err != nil {
			return 0, 0, DecimalExponent, false
		}
		return 10, int32(parsed), DecimalExponent, true

@vegemitecheesetoast
Copy link

@vegemitecheesetoast vegemitecheesetoast commented Sep 9, 2019

My first PR was merged to change the utils for this, now I will work on migrating the logic that uses ParseInt to ParsePort in utils. Hopefully should get to that this weekend.

@entro-pi
Copy link

@entro-pi entro-pi commented Oct 2, 2019

Ping! @vegemitecheesetoast are you still interested in finishing this one up?

I'm an Outreachy Applicant and would love to finish what has been started here.

@bbtong
Copy link

@bbtong bbtong commented Nov 13, 2019

Still open -- anyone actively working on this? Would like to attempt if not. :)

@JakubOboza
Copy link

@JakubOboza JakubOboza commented Dec 19, 2019

If it is ports which should be in range 0 to 65535 most sense to me makes unit16 but strconv.ParseUint returns uint64 which could also be a option.

ParseUint has a limit on bitsize which can be set to 16 to force port be in correct range
https://golang.org/pkg/strconv/#ParseUint

link to playground: https://play.golang.org/p/bWlmTBuwKNt

package main

import (
	"fmt"
	"strconv"
)

func main() {
	tooBigValue    := "66000"
	correctValue   := "30111"
	belowZeroValue := "-13"
	
	_, err := strconv.ParseUint(tooBigValue, 10, 16);
	if err != nil {
		fmt.Println(err)
	}
		
	parsedValue, err := strconv.ParseUint(correctValue, 10, 16);

	fmt.Println(parsedValue)
	
	_, errToBig := strconv.ParseUint(belowZeroValue, 10, 16);
	if errToBig != nil {
		fmt.Println(errToBig)
	}

}

golang "net" package has also unexported function called parsePort
https://golang.org/src/net/port.go

with comment:

// Some system resolvers will return a valid port number when given a number
// over 65536 (see https://golang.org/issues/11715). Alas, the parser
// can't bail early on numbers > 65536. Therefore reasonably large/small
// numbers are parsed in full and rejected if invalid.

Don't know if this applies here but maybe ?

On the other hand in the net package there is a strict >0 < 65535 requirement.
https://golang.org/src/net/lookup.go?s=11027:11124#L327

LookupPort method.

func (r *Resolver) LookupPort(ctx context.Context, network, service string) (port int, err error) {
	port, needsLookup := parsePort(service)
	if needsLookup {
		switch network {
		case "tcp", "tcp4", "tcp6", "udp", "udp4", "udp6":
		case "": // a hint wildcard for Go 1.0 undocumented behavior
			network = "ip"
		default:
			return 0, &AddrError{Err: "unknown network", Addr: network}
		}
		port, err = r.lookupPort(ctx, network, service)
		if err != nil {
			return 0, err
		}
	}
	if 0 > port || port > 65535 {
		return 0, &AddrError{Err: "invalid port", Addr: service}
	}
	return port, nil
}

So imho eg:

strconv.ParseUint(portStr, 10, 16);

should be fine in this case.

@alaymodi
Copy link

@alaymodi alaymodi commented Dec 25, 2019

hi! if there is any more work to be done on this, I would love to help.

@tanjunchen
Copy link
Member

@tanjunchen tanjunchen commented Dec 27, 2019

very interesting . focus on this topic

@fejta-bot
Copy link

@fejta-bot fejta-bot commented Mar 26, 2020

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@cji
Copy link
Member Author

@cji cji commented Mar 26, 2020

/lifecycle frozen

@cmluciano
Copy link
Member

@cmluciano cmluciano commented Aug 28, 2020

I think this should be covered by #89120

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Aug 28, 2020

@cmluciano: Closing this issue.

In response to this:

I think this should be covered by #89120

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.