Fix CodeQL coode scanning alerts (microsoft#1972)

Fix CodeQL alerts for unchecked downcasts from `int`s are to
`(u)int32` (or `uintptr`) without checking for overflow.
This can (potentially) cause incorrect behavior due to the value
wrapping around to an unexpected value.

Alerts:
https://github.com/microsoft/hcsshim/security/code-scanning?query=branch%3Amain+rule%3Ago%2Fincorrect-integer-conversion

Issue description:
https://cwe.mitre.org/data/definitions/190.html
https://cwe.mitre.org/data/definitions/681.html

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

guest/bridge/bridge.go

@@ -11,6 +11,7 @@ import (
  "encoding/json"
  "fmt"
  "io"
+ "math"
  "os"
  "strconv"
  "sync"
@@ -443,21 +444,23 @@ func setErrorForResponseBase(response *prot.MessageResponseBase, errForResponse
  errorMessage := errForResponse.Error()
  stackString := ""
  fileName := ""
- lineNumber := -1
+ // We use -1 as a sentinel if no line number found (or it cannot be parsed),
+ // but that will ultimately end up as [math.MaxUint32], so set it to that explicitly.
+ // (Still keep using -1 for backwards compatibility ...)
+ lineNumber := uint32(math.MaxUint32)
  functionName := ""
  if stack := gcserr.BaseStackTrace(errForResponse); stack != nil {
  bottomFrame := stack[0]
  stackString = fmt.Sprintf("%+v", stack)
  fileName = fmt.Sprintf("%s", bottomFrame)
  lineNumberStr := fmt.Sprintf("%d", bottomFrame)
- var err error
- lineNumber, err = strconv.Atoi(lineNumberStr)
- if err != nil {
+ if n, err := strconv.ParseUint(lineNumberStr, 10, 32); err == nil {
+  lineNumber = uint32(n)
+ } else {
  logrus.WithFields(logrus.Fields{
  "line-number": lineNumberStr,
  logrus.ErrorKey: err,
  }).Error("opengcs::bridge::setErrorForResponseBase - failed to parse line number, using -1 instead")
- lineNumber = -1
  }
  functionName = fmt.Sprintf("%n", bottomFrame)
  }
@@ -474,7 +477,7 @@ func setErrorForResponseBase(response *prot.MessageResponseBase, errForResponse
  StackTrace: stackString,
  ModuleName: "gcs",
  FileName: fileName,
- Line: uint32(lineNumber),
+ Line: lineNumber,
  FunctionName: functionName,
  }
  response.ErrorRecords = append(response.ErrorRecords, newRecord)

guest/runtime/hcsv2/spec.go

@@ -6,6 +6,7 @@ package hcsv2
 import (
  "context"
  "fmt"
+ "math"
  "path/filepath"
  "strconv"
  "strings"
@@ -92,6 +93,18 @@ func setCoreRLimit(spec *oci.Spec, value string) error {
 // Valid values are: user, uid, user:group, uid:gid, uid:group, user:gid.
 // If uid is provided instead of the username then that value is not checked against the
 // /etc/passwd file to verify if the user with given uid actually exists.
+//
+// Since UID and GID are parsed as ints, but will ultimately end up as uint32 in the [OCI spec],
+// an error is returned if the the IDs are not within the uint32 bounds ([0, math.MathUint32]).
+// This avoid unexpected results if the ID is first parsed as an int and then
+// overflows around when downcast (eg, [math.MaxUint32] + 1 will become 0).
+// Notes:
+// - Per the [Go spec], we have no indication of overflow when converting between integer types.
+// - "man 5 passwd" and "man 5 group" (as well as [user.ParsePasswdFileFilter] and [user.ParseGroupFilter))
+// do not specify any limits on the UID and GID range.
+//
+// [OCI spec]: https://pkg.go.dev/github.com/opencontainers/runtime-spec/specs-go#User
+// [Go spec]: https://go.dev/ref/spec#Conversions
 func setUserStr(spec *oci.Spec, userstr string) error {
  setProcess(spec)
 
@@ -103,32 +116,48 @@ func setUserStr(spec *oci.Spec, userstr string) error {
  // evaluate username to uid/gid
  return setUsername(spec, userstr)
  }
- return setUserID(spec, int(v))
+ if outOfUint32Bounds(v) {
+ return errors.Errorf("UID (%d) exceeds uint32 bounds", v)
+ }
+ return setUserID(spec, uint32(v))
  case 2:
  var (
  username, groupname string
- uid, gid int
+ uid, gid uint32
  )
+
  v, err := strconv.Atoi(parts[0])
  if err != nil {
  username = parts[0]
  } else {
- uid = int(v)
+ if outOfUint32Bounds(v) {
+ return errors.Errorf("UID (%d) exceeds uint32 bounds", v)
+ }
+ uid = uint32(v)
  }
+
  v, err = strconv.Atoi(parts[1])
  if err != nil {
  groupname = parts[1]
  } else {
- gid = int(v)
+ if outOfUint32Bounds(v) {
+ return errors.Errorf("GID (%d) for user %q exceeds uint32 bounds", v, parts[0])
+ }
+ gid = uint32(v)
  }
+
  if username != "" {
  u, err := getUser(spec, func(u user.User) bool {
  return u.Name == username
  })
  if err != nil {
  return errors.Wrapf(err, "failed to find user by username: %s", username)
  }
- uid = u.Uid
+
+ if outOfUint32Bounds(u.Uid) {
+ return errors.Errorf("UID (%d) for username %q exceeds uint32 bounds", u.Uid, username)
+ }
+ uid = uint32(u.Uid)
  }
  if groupname != "" {
  g, err := getGroup(spec, func(g user.Group) bool {
@@ -137,9 +166,14 @@ func setUserStr(spec *oci.Spec, userstr string) error {
  if err != nil {
  return errors.Wrapf(err, "failed to find group by groupname: %s", groupname)
  }
- gid = g.Gid
+
+ if outOfUint32Bounds(g.Gid) {
+ return errors.Errorf("GID (%d) for groupname %q exceeds uint32 bounds", g.Gid, groupname)
+ }
+ gid = uint32(g.Gid)
  }
- spec.Process.User.UID, spec.Process.User.GID = uint32(uid), uint32(gid)
+
+ spec.Process.User.UID, spec.Process.User.GID = uid, gid
  return nil
  default:
  return fmt.Errorf("invalid userstr: '%s'", userstr)
@@ -153,19 +187,29 @@ func setUsername(spec *oci.Spec, username string) error {
  if err != nil {
  return errors.Wrapf(err, "failed to find user by username: %s", username)
  }
+ if outOfUint32Bounds(u.Uid) {
+ return errors.Errorf("UID (%d) for username %q exceeds uint32 bounds", u.Uid, username)
+ }
+ if outOfUint32Bounds(u.Gid) {
+ return errors.Errorf("GID (%d) for username %q exceeds uint32 bounds", u.Gid, username)
+ }
  spec.Process.User.UID, spec.Process.User.GID = uint32(u.Uid), uint32(u.Gid)
  return nil
 }
 
-func setUserID(spec *oci.Spec, uid int) error {
+func setUserID(spec *oci.Spec, uid uint32) error {
  u, err := getUser(spec, func(u user.User) bool {
- return u.Uid == uid
+ return u.Uid == int(uid)
  })
  if err != nil {
- spec.Process.User.UID, spec.Process.User.GID = uint32(uid), 0
+ spec.Process.User.UID, spec.Process.User.GID = uid, 0
  return nil
  }
- spec.Process.User.UID, spec.Process.User.GID = uint32(u.Uid), uint32(u.Gid)
+
+ if outOfUint32Bounds(u.Gid) {
+ return errors.Errorf("GID (%d) for UID %d exceeds uint32 bounds", u.Gid, uid)
+ }
+ spec.Process.User.UID, spec.Process.User.GID = uid, uint32(u.Gid)
  return nil
 }
 
@@ -277,3 +321,5 @@ func devShmMountWithSize(sizeString string) (*oci.Mount, error) {
  Options: []string{"nosuid", "noexec", "nodev", "mode=1777", sizeKB},
  }, nil
 }
+
+func outOfUint32Bounds(v int) bool { return v < 0 || v > math.MaxUint32 }

hcsoci/hcsdoc_wcow.go

@@ -245,7 +245,16 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter
  // Memory Resources
  memoryMaxInMB := oci.ParseAnnotationsMemory(ctx, coi.Spec, annotations.ContainerMemorySizeInMB, 0)
  if memoryMaxInMB > 0 {
- v1.MemoryMaximumInMB = int64(memoryMaxInMB)
+ if memoryMaxInMB > math.MaxInt64 {
+ v1.MemoryMaximumInMB = math.MaxInt64
+ log.G(ctx).WithFields(logrus.Fields{
+ "annotation": annotations.ContainerMemorySizeInMB,
+ "oldMemoryMaxInMB": memoryMaxInMB,
+ "newMemoryMaxInMB": v1.MemoryMaximumInMB,
+ }).Debug("container memory size limit exceeds maximum value allowed in v1 HCS schema")
+ } else {
+ v1.MemoryMaximumInMB = int64(memoryMaxInMB)
+ }
  v2Container.Memory = &hcsschema.Memory{
  SizeInMB: memoryMaxInMB,
  }
@@ -479,7 +488,7 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter
 }
 
 // parseAssignedDevices parses assigned devices for the container definition
-// this is currently supported for v2 argon and xenon only
+// this is currently supported for v2 argon and xenon only.
 func parseAssignedDevices(ctx context.Context, coi *createOptionsInternal, v2 *hcsschema.Container) error {
  if !coi.isV2Argon() && !coi.isV2Xenon() {
  return nil
@@ -513,7 +522,7 @@ func parseDumpCount(annots map[string]string) (int32, error) {
  return 10, nil
  }
 
- dumpCount, err := strconv.Atoi(dmpCountStr)
+ dumpCount, err := strconv.ParseInt(dmpCountStr, 10, 32)
  if err != nil {
  return -1, err
  }
@@ -526,7 +535,7 @@ func parseDumpCount(annots map[string]string) (int32, error) {
 // parseDumpType parses the passed in string representation of the local user mode process dump type to the
 // corresponding value the registry expects to be set.
 //
-// See DumpType at https://docs.microsoft.com/en-us/windows/win32/wer/collecting-user-mode-dumps for the mappings
+// See DumpType at https://docs.microsoft.com/en-us/windows/win32/wer/collecting-user-mode-dumps for the mappings.
 func parseDumpType(annots map[string]string) (int32, error) {
  dmpTypeStr := annots[annotations.WCOWProcessDumpType]
  switch dmpTypeStr {

jobobject/limits.go

@@ -143,6 +143,13 @@ func (job *JobObject) SetCPUAffinity(affinityBitMask uint64) error {
  return err
  }
  info.BasicLimitInformation.LimitFlags |= uint32(windows.JOB_OBJECT_LIMIT_AFFINITY)
+
+ // We really, really shouldn't be running on 32 bit, but just in case (and to satisfy CodeQL) ...
+ const maxUintptr = ^uintptr(0)
+ if affinityBitMask > uint64(maxUintptr) {
+ return fmt.Errorf("affinity bitmask (%d) exceeds max allowable value (%d)", affinityBitMask, maxUintptr)
+ }
+
  info.BasicLimitInformation.Affinity = uintptr(affinityBitMask)
  return job.setExtendedInformation(info)
 }

logfields/fields.go

@@ -46,6 +46,7 @@ const (
 
  ExpectedType = "expected-type"
  Bool = "bool"
+ Int32 = "int32"
  Uint32 = "uint32"
  Uint64 = "uint64"
 

oci/annotations.go

@@ -221,6 +221,20 @@ func ParseAnnotationsNullableBool(ctx context.Context, a map[string]string, key
  return nil
 }
 
+// ParseAnnotationsInt32 searches `a` for `key` and if found verifies that the
+// value is a 32-bit signed integer. If `key` is not found returns `def`.
+func ParseAnnotationsInt32(ctx context.Context, a map[string]string, key string, def int32) int32 {
+ if v, ok := a[key]; ok {
+ countu, err := strconv.ParseInt(v, 10, 32)
+ if err == nil {
+ v := int32(countu)
+ return v
+ }
+ logAnnotationParseError(ctx, key, v, logfields.Int32, err)
+ }
+ return def
+}
+
 // ParseAnnotationsUint32 searches `a` for `key` and if found verifies that the
 // value is a 32 bit unsigned integer. If `key` is not found returns `def`.
 func ParseAnnotationsUint32(ctx context.Context, a map[string]string, key string, def uint32) uint32 {

oci/uvm.go

@@ -21,8 +21,8 @@ import (
 // not found searches `s` for the Windows CPU section. If neither are found
 // returns `def`.
 func ParseAnnotationsCPUCount(ctx context.Context, s *specs.Spec, annotation string, def int32) int32 {
- if m := ParseAnnotationsUint64(ctx, s.Annotations, annotation, 0); m != 0 {
- return int32(m)
+ if m := ParseAnnotationsInt32(ctx, s.Annotations, annotation, 0); m != 0 {
+ return m
  }
  if s.Windows != nil &&
  s.Windows.Resources != nil &&
@@ -38,8 +38,8 @@ func ParseAnnotationsCPUCount(ctx context.Context, s *specs.Spec, annotation str
 // not found searches `s` for the Windows CPU section. If neither are found
 // returns `def`.
 func ParseAnnotationsCPULimit(ctx context.Context, s *specs.Spec, annotation string, def int32) int32 {
- if m := ParseAnnotationsUint64(ctx, s.Annotations, annotation, 0); m != 0 {
- return int32(m)
+ if m := ParseAnnotationsInt32(ctx, s.Annotations, annotation, 0); m != 0 {
+ return m
  }
  if s.Windows != nil &&
  s.Windows.Resources != nil &&
@@ -55,8 +55,8 @@ func ParseAnnotationsCPULimit(ctx context.Context, s *specs.Spec, annotation str
 // not found searches `s` for the Windows CPU section. If neither are found
 // returns `def`.
 func ParseAnnotationsCPUWeight(ctx context.Context, s *specs.Spec, annotation string, def int32) int32 {
- if m := ParseAnnotationsUint64(ctx, s.Annotations, annotation, 0); m != 0 {
- return int32(m)
+ if m := ParseAnnotationsInt32(ctx, s.Annotations, annotation, 0); m != 0 {
+ return m
  }
  if s.Windows != nil &&
  s.Windows.Resources != nil &&
@@ -72,8 +72,8 @@ func ParseAnnotationsCPUWeight(ctx context.Context, s *specs.Spec, annotation st
 // annotation. If not found searches `s` for the Windows Storage section. If
 // neither are found returns `def`.
 func ParseAnnotationsStorageIops(ctx context.Context, s *specs.Spec, annotation string, def int32) int32 {
- if m := ParseAnnotationsUint64(ctx, s.Annotations, annotation, 0); m != 0 {
- return int32(m)
+ if m := ParseAnnotationsInt32(ctx, s.Annotations, annotation, 0); m != 0 {
+ return m
  }
  if s.Windows != nil &&
  s.Windows.Resources != nil &&
@@ -89,8 +89,8 @@ func ParseAnnotationsStorageIops(ctx context.Context, s *specs.Spec, annotation
 // If not found searches `s` for the Windows Storage section. If neither are
 // found returns `def`.
 func ParseAnnotationsStorageBps(ctx context.Context, s *specs.Spec, annotation string, def int32) int32 {
- if m := ParseAnnotationsUint64(ctx, s.Annotations, annotation, 0); m != 0 {
- return int32(m)
+ if m := ParseAnnotationsInt32(ctx, s.Annotations, annotation, 0); m != 0 {
+ return m
  }
  if s.Windows != nil &&
  s.Windows.Resources != nil &&