Merge pull request #460 from cloudfoundry/g115-fix

Address integer overflows across guardian

gardener/gardener.go

@@ -538,7 +538,8 @@ func (g *Gardener) checkMaxContainers(handles []string) error {
 		return nil
 	}
 
-	if len(handles) >= int(g.MaxContainers) {
+	// #nosec G115 - length will never be negative
+	if uint64(len(handles)) >= g.MaxContainers {
 		return errors.New("max containers reached")
 	}
 

gardener/volume_provider.go

@@ -68,9 +68,10 @@ func (v *VolumeProvider) Create(log lager.Logger, spec garden.ContainerSpec) (sp
 	} else {
 		var err error
 		baseConfig, err = v.VolumeCreator.Create(log.Session("volume-creator"), spec.Handle, RootfsSpec{
-			RootFS:     rootFSURL,
-			Username:   spec.Image.Username,
-			Password:   spec.Image.Password,
+			RootFS:   rootFSURL,
+			Username: spec.Image.Username,
+			Password: spec.Image.Password,
+			// #nosec G115 - itnore int overflow for filesystem attrs as it would require 9 exabytes to cause an issue
 			QuotaSize:  int64(spec.Limits.Disk.ByteHard),
 			QuotaScope: spec.Limits.Disk.Scope,
 			Namespaced: !spec.Privileged,

guardiancmd/command.go

@@ -599,6 +599,7 @@ func (cmd *CommonCommand) wireContainerizer(
 
 	processDepot := execrunner.NewProcessDirDepot(depot)
 
+	// #nosec G115 - the uid/gidmappings lists are capped at maxint32 by idmapper, and should never be negative
 	execRunner := factory.WireExecRunner(runcRoot, uint32(uidMappings.Map(0)), uint32(gidMappings.Map(0)), bundleSaver, depot, processDepot)
 	wireExecerFunc := func(pidGetter runrunc.PidGetter) *runrunc.Execer {
 		return runrunc.NewExecer(depot, processBuilder, factory.WireMkdirer(), userLookupper, execRunner, pidGetter)
@@ -723,8 +724,9 @@ func (cmd *CommonCommand) idMappings() (idmapper.MappingList, idmapper.MappingLi
 	uidMappings := idmapper.MappingList{
 		{
 			ContainerID: 0,
-			HostID:      uint32(containerRootUID),
-			Size:        1,
+			// #nosec G115 - containerRootUID uses idmapper's logic which caps the Uid at MaxInt32, and should not be negative.
+			HostID: uint32(containerRootUID),
+			Size:   1,
 		},
 		{
 			ContainerID: 1,
@@ -735,8 +737,9 @@ func (cmd *CommonCommand) idMappings() (idmapper.MappingList, idmapper.MappingLi
 	gidMappings := idmapper.MappingList{
 		{
 			ContainerID: 0,
-			HostID:      uint32(containerRootGID),
-			Size:        1,
+			// #nosec G115 - containerRootGID uses idmapper's logic which caps the Gid at MaxInt32, and should not be negative.
+			HostID: uint32(containerRootGID),
+			Size:   1,
 		},
 		{
 			ContainerID: 1,
@@ -749,10 +752,12 @@ func (cmd *CommonCommand) idMappings() (idmapper.MappingList, idmapper.MappingLi
 
 func (cmd *CommonCommand) calculateDefaultMappingLengths(containerRootUID, containerRootGID int) {
 	if cmd.Containers.UIDMapLength == 0 {
+		// #nosec G115 - containerRootUID uses idmapper's logic which caps the Uid at MaxInt32, and should not be negative.
 		cmd.Containers.UIDMapLength = uint32(containerRootUID) - cmd.Containers.UIDMapStart
 	}
 
 	if cmd.Containers.GIDMapLength == 0 {
+		// #nosec G115 - containerRootUID uses idmapper's logic which caps the Uid at MaxInt32, and should not be negative.
 		cmd.Containers.GIDMapLength = uint32(containerRootGID) - cmd.Containers.GIDMapStart
 	}
 }

guardiancmd/kernel_min_version_checker.go

@@ -47,27 +47,28 @@ func kernelVersionFromReleaseString(release string) (uint64, error) {
 		return 0, fmt.Errorf("malformed version: %s", release)
 	}
 
-	maj, err := strconv.ParseInt(parts[1], 10, 16)
+	maj, err := strconv.ParseUint(parts[1], 10, 16)
 	if err != nil {
 		return 0, err
 	}
 
-	var min int64
+	var min uint64
 	if parts[2] != "" {
-		min, err = strconv.ParseInt(parts[2], 10, 16)
+		min, err = strconv.ParseUint(parts[2], 10, 16)
 		if err != nil {
 			return 0, err
 		}
 	}
 
-	var patch int64
+	var patch uint64
 	if parts[3] != "" {
-		patch, err = strconv.ParseInt(parts[3], 10, 16)
+		patch, err = strconv.ParseUint(parts[3], 10, 16)
 		if err != nil {
 			return 0, err
 		}
 	}
 
+	// #nosec G115 - strconv.ParseUint validates all of these above
 	return kernelVersion(uint16(maj), uint16(min), uint16(patch)), nil
 }
 

kawasaki/configure/container.go

@@ -148,5 +148,6 @@ func exitStatus(err error) (uint32, error) {
 		return 2, exitErr
 	}
 
+	// #nosec G115 - waitstatus is a uint32 that could return -1 if  the program hadn't exited, but we've nsured that prior to calling exitStatus() and by checking ExitError()
 	return uint32(status.ExitStatus()), nil
 }

kawasaki/networker.go

@@ -177,6 +177,7 @@ func (n *Networker) Network(log lager.Logger, containerSpec garden.ContainerSpec
 
 // Capacity returns the number of subnets this network can host
 func (n *Networker) Capacity() uint64 {
+	// #nosec G115 - we would never have a negative pool of subnets
 	return uint64(n.subnetPool.Capacity())
 }
 

rundmc/bundlerules/limits.go

@@ -1,6 +1,8 @@
 package bundlerules
 
 import (
+	"math"
+
 	spec "code.cloudfoundry.org/guardian/gardener/container-spec"
 	"code.cloudfoundry.org/guardian/rundmc/goci"
 	"github.com/opencontainers/runtime-spec/specs-go"
@@ -18,7 +20,13 @@ type Limits struct {
 }
 
 func (l Limits) Apply(bndl goci.Bndl, spec spec.DesiredContainerSpec) (goci.Bndl, error) {
-	limit := int64(spec.Limits.Memory.LimitInBytes)
+	var limit int64
+	if spec.Limits.Memory.LimitInBytes > math.MaxInt64 {
+		limit = math.MaxInt64
+	} else {
+		// #nosec G115 - any values over maxint64 are capped above, so no overflow
+		limit = int64(spec.Limits.Memory.LimitInBytes)
+	}
 
 	var swapLimit *int64
 	if !l.DisableSwapLimit {
@@ -28,9 +36,9 @@ func (l Limits) Apply(bndl goci.Bndl, spec spec.DesiredContainerSpec) (goci.Bndl
 	bndl = bndl.WithMemoryLimit(specs.LinuxMemory{Limit: &limit, Swap: swapLimit})
 
 	//lint:ignore SA1019 - we still specify this to make the deprecated logic work until we get rid of the code in garden
-	shares := uint64(spec.Limits.CPU.LimitInShares)
+	shares := spec.Limits.CPU.LimitInShares
 	if spec.Limits.CPU.Weight > 0 {
-		shares = uint64(spec.Limits.CPU.Weight)
+		shares = spec.Limits.CPU.Weight
 	}
 	cpuSpec := specs.LinuxCPU{Shares: &shares}
 	if l.CpuQuotaPerShare > 0 && shares > 0 {
@@ -46,11 +54,21 @@ func (l Limits) Apply(bndl goci.Bndl, spec spec.DesiredContainerSpec) (goci.Bndl
 
 	bndl = bndl.WithBlockIO(specs.LinuxBlockIO{Weight: &l.BlockIOWeight})
 
-	pids := int64(spec.Limits.Pid.Max)
+	var pids int64
+	if spec.Limits.Pid.Max > math.MaxInt64 {
+		pids = math.MaxInt64
+	} else {
+		// #nosec G115 - any values over maxint64 are capped above, so no overflow
+		pids = int64(spec.Limits.Pid.Max)
+	}
 	return bndl.WithPidLimit(specs.LinuxPids{Limit: pids}), nil
 }
 
 func int64PtrVal(n uint64) *int64 {
+	if n > math.MaxInt64 {
+		n = math.MaxInt64
+	}
+	// #nosec G115 - any values over maxint64 are capped above, so no overflow
 	unsignedVal := int64(n)
 	return &unsignedVal
 }

rundmc/bundlerules/mount_options_linux.go

@@ -22,6 +22,7 @@ func UnprivilegedMountFlagsGetter(path string) ([]string, error) {
 
 	var flags []string
 	for mask, flag := range unprivilegedFlags {
+		// #nosec G115 - all the flags we care about above are positive ints, so we don't need to worry about overflow here
 		if uint64(statfs.Flags)&mask == mask {
 			flags = append(flags, flag)
 		}

rundmc/bundlerules/windows.go

@@ -1,6 +1,8 @@
 package bundlerules
 
 import (
+	"math"
+
 	spec "code.cloudfoundry.org/guardian/gardener/container-spec"
 	"code.cloudfoundry.org/guardian/rundmc/goci"
 	"github.com/opencontainers/runtime-spec/specs-go"
@@ -19,11 +21,19 @@ func (w Windows) Apply(bndl goci.Bndl, spec spec.DesiredContainerSpec) (goci.Bnd
 	bndl = bndl.WithWindowsMemoryLimit(specs.WindowsMemoryResources{Limit: &limit})
 
 	//lint:ignore SA1019 - we still specify this to make the deprecated logic work until we get rid of the code in garden
-	shares := uint16(spec.Limits.CPU.LimitInShares)
+	shares := spec.Limits.CPU.LimitInShares
 	if spec.Limits.CPU.Weight > 0 {
-		shares = uint16(spec.Limits.CPU.Weight)
+		shares = spec.Limits.CPU.Weight
+	}
+
+	var sharesUint uint16
+	if shares > math.MaxUint16 {
+		sharesUint = math.MaxUint16
+	} else {
+		sharesUint = uint16(shares)
 	}
-	bndl = bndl.WithWindowsCPUShares(specs.WindowsCPUResources{Shares: &shares})
+
+	bndl = bndl.WithWindowsCPUShares(specs.WindowsCPUResources{Shares: &sharesUint})
 
 	return bndl, nil
 }

rundmc/containerizer.go

@@ -351,6 +351,7 @@ func (c *Containerizer) Info(log lager.Logger, handle string) (spec.ActualContai
 	var cpuShares, limitInBytes uint64
 	if bundle.Resources() != nil {
 		cpuShares = *bundle.Resources().CPU.Shares
+		// #nosec G115 - limits should never be negative
 		limitInBytes = uint64(*bundle.Resources().Memory.Limit)
 	} else {
 		log.Debug("bundle-resources-is-nil", lager.Data{"bundle": bundle})

rundmc/execrunner/dadoo/winsize.go

@@ -26,7 +26,7 @@ func SetWinSize(f *os.File, ws garden.WindowSize) error {
 		uintptr(unsafe.Pointer(&struct {
 			Rows uint16
 			Cols uint16
-		}{Rows: uint16(ws.Rows), Cols: uint16(ws.Columns)})),
+		}{Rows: ws.Rows, Cols: ws.Columns})),
 		0, 0, 0,
 	)
 

rundmc/preparerootfs/prepare_linux.go

@@ -38,7 +38,7 @@ func prepare() {
 	var rootfsPath = flag.String("rootfsPath", "", "rootfs path to chroot into")
 	var uid = flag.Int("uid", 0, "uid to create directories as")
 	var gid = flag.Int("gid", 0, "gid to create directories as")
-	var perm = flag.Int("perm", 0755, "Mode to create the directory with")
+	var perm = flag.Uint("perm", 0755, "Mode to create the directory with")
 	var recreate = flag.Bool("recreate", false, "whether to delete the directory before (re-)creating it")
 
 	flag.Parse()
@@ -62,6 +62,7 @@ func prepare() {
 			rmdir(path)
 		}
 
+		// #nosec G115 - filemodes shouldn't be above 32bit anyway
 		mkdir(path, *uid, *gid, os.FileMode(*perm))
 	}
 }

rundmc/processes/builder.go

@@ -38,7 +38,9 @@ func (p *ProcBuilder) BuildProcess(bndl goci.Bndl, spec garden.ProcessSpec, user
 		ConsoleSize: console(spec),
 		Env:         p.envDeterminer.EnvFor(bndl, spec, user.Uid),
 		User: specs.User{
-			UID:            uint32(user.Uid),
+			// #nosec G115 - uids should be positive and 32bit on linux, but libcontainer uses an int for them
+			UID: uint32(user.Uid),
+			// #nosec G115 - gids should be positive and 32bit on linux, but libcontainer uses an int for them
 			GID:            uint32(user.Gid),
 			AdditionalGids: additionalGIDs,
 			Username:       spec.User,
@@ -96,6 +98,7 @@ func intersect(l1 []string, l2 []string) (result []string) {
 func toUint32Slice(slice []int) []uint32 {
 	result := []uint32{}
 	for _, i := range slice {
+		// #nosec G115 - uids should be positive and 32bit on linux, but libcontainer uses an int for them
 		result = append(result, uint32(i))
 	}
 	return result

rundmc/runcontainerd/runcontainerd.go

@@ -129,6 +129,7 @@ func (r *RunContainerd) Create(log lager.Logger, id string, bundle goci.Bndl, pi
 	containerRootUID := idmapper.MappingList(bundle.Spec.Linux.UIDMappings).Map(0)
 	containerRootGID := idmapper.MappingList(bundle.Spec.Linux.GIDMappings).Map(0)
 
+	// #nosec G115 - the uid/gidmappings lists are capped at maxint32 by idmapper, and should never be negative
 	err := r.containerManager.Create(log, id, &bundle.Spec, uint32(containerRootUID), uint32(containerRootGID), func() (io.Reader, io.Writer, io.Writer) { return pio.Stdin, pio.Stdout, pio.Stderr })
 	if err != nil {
 		return err