Check for overflows when converting ints to uints

As was done in MetalLB, we fix the G115 gosec errors by converting ints to uints more carefully. Signed-off-by: Ori Braunshtein <[email protected]>
metallb · Sep 17, 2024 · 7b2f899 · 7b2f899
1 parent 3bfd588
commit 7b2f899
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 26 deletions.
diff --git a/internal/controller/api_to_config.go b/internal/controller/api_to_config.go
@@ -15,6 +15,7 @@ import (
 	"github.com/metallb/frr-k8s/internal/community"
 	"github.com/metallb/frr-k8s/internal/frr"
 	"github.com/metallb/frr-k8s/internal/ipfamily"
+	"github.com/metallb/frr-k8s/internal/safeconvert"
 	corev1 "k8s.io/api/core/v1"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -185,7 +186,7 @@ func neighborToFRR(n v1beta1.Neighbor, prefixesInRouter []string, alwaysBlock []
 	}
 
 	if n.ConnectTime != nil {
-		res.ConnectTime = ptr.To(uint64(n.ConnectTime.Duration / time.Second))
+		res.ConnectTime = ptr.To(int64(n.ConnectTime.Duration / time.Second))
 	}
 
 	res.Password, err = passwordForNeighbor(n, passwordSecrets)
@@ -373,7 +374,11 @@ func filterForSelector(selector v1beta1.PrefixSelector) (frr.IncomingFilter, err
 		return frr.IncomingFilter{}, fmt.Errorf("failed to parse prefix %s: %w", selector.Prefix, err)
 	}
 	maskLen, _ := cidr.Mask.Size()
-	err = validateSelectorLengths(maskLen, selector.LE, selector.GE)
+	maskLenUint, err := safeconvert.IntToUInt32(maskLen)
+	if err != nil {
+		return frr.IncomingFilter{}, fmt.Errorf("failed to convert maskLen from CIDR %s to uint32: %w", cidr, err)
+	}
+	err = validateSelectorLengths(maskLenUint, selector.LE, selector.GE)
 	if err != nil {
 		return frr.IncomingFilter{}, err
 	}
@@ -390,17 +395,17 @@ func filterForSelector(selector v1beta1.PrefixSelector) (frr.IncomingFilter, err
 
 // validateSelectorLengths checks the lengths respect the following
 // condition: mask length <= ge <= le
-func validateSelectorLengths(mask int, le, ge uint32) error {
+func validateSelectorLengths(mask, le, ge uint32) error {
 	if ge == 0 && le == 0 {
 		return nil
 	}
 	if le > 0 && ge > le {
 		return fmt.Errorf("invalid selector lengths: ge %d is bigger than le %d", ge, le)
 	}
-	if le > 0 && uint32(mask) > le {
+	if le > 0 && mask > le {
 		return fmt.Errorf("invalid selector lengths: cidr mask %d is bigger than le %d", mask, le)
 	}
-	if ge > 0 && uint32(mask) > ge {
+	if ge > 0 && mask > ge {
 		return fmt.Errorf("invalid selector lengths: cidr mask %d is bigger than ge %d", mask, ge)
 	}
 	return nil
@@ -604,7 +609,7 @@ func alwaysBlockToFRR(cidrs []net.IPNet) []frr.IncomingFilter {
 	return res
 }
 
-func parseTimers(ht, ka *v1.Duration) (*uint64, *uint64, error) {
+func parseTimers(ht, ka *v1.Duration) (*int64, *int64, error) {
 	if ht == nil && ka != nil || ht != nil && ka == nil {
 		return nil, nil, fmt.Errorf("one of KeepaliveTime/HoldTime specified, both must be set or none")
 	}
@@ -625,8 +630,8 @@ func parseTimers(ht, ka *v1.Duration) (*uint64, *uint64, error) {
 		return nil, nil, fmt.Errorf("invalid keepaliveTime %q, must be lower than holdTime %q", ka, ht)
 	}
 
-	htSeconds := uint64(holdTime / time.Second)
-	kaSeconds := uint64(keepaliveTime / time.Second)
+	htSeconds := int64(holdTime / time.Second)
+	kaSeconds := int64(keepaliveTime / time.Second)
 
 	return &htSeconds, &kaSeconds, nil
 }

diff --git a/internal/controller/api_to_config_test.go b/internal/controller/api_to_config_test.go
@@ -83,9 +83,9 @@ func TestConversion(t *testing.T) {
 								Port:            ptr.To[uint16](179),
 								SrcAddr:         "192.1.1.1",
 								Addr:            "192.0.2.2",
-								KeepaliveTime:   ptr.To[uint64](20),
-								HoldTime:        ptr.To[uint64](40),
-								ConnectTime:     ptr.To(uint64(2)),
+								KeepaliveTime:   ptr.To[int64](20),
+								HoldTime:        ptr.To[int64](40),
+								ConnectTime:     ptr.To(int64(2)),
 								DisableMP:       true,
 								GracefulRestart: true,
 							},
@@ -144,9 +144,9 @@ func TestConversion(t *testing.T) {
 								ASN:           "65002",
 								Port:          ptr.To[uint16](179),
 								Addr:          "192.0.2.2",
-								KeepaliveTime: ptr.To[uint64](20),
-								HoldTime:      ptr.To[uint64](40),
-								ConnectTime:   ptr.To(uint64(2)),
+								KeepaliveTime: ptr.To[int64](20),
+								HoldTime:      ptr.To[int64](40),
+								ConnectTime:   ptr.To(int64(2)),
 								DisableMP:     true,
 							},
 						},

diff --git a/internal/controller/merge_test.go b/internal/controller/merge_test.go
@@ -1309,9 +1309,9 @@ func TestMergeNeighbors(t *testing.T) {
 					Name:          "[email protected]",
 					ASN:           "65040",
 					Addr:          "192.0.1.20",
-					HoldTime:      ptr.To(uint64(180)),
-					KeepaliveTime: ptr.To(uint64(60)),
-					ConnectTime:   ptr.To(uint64(60)),
+					HoldTime:      ptr.To(int64(180)),
+					KeepaliveTime: ptr.To(int64(60)),
+					ConnectTime:   ptr.To(int64(60)),
 				},
 			},
 			expected: []*frr.NeighborConfig{
@@ -1341,9 +1341,9 @@ func TestMergeNeighbors(t *testing.T) {
 					Name:          "[email protected]",
 					ASN:           "65040",
 					Addr:          "192.0.1.20",
-					HoldTime:      ptr.To(uint64(180)),
-					KeepaliveTime: ptr.To(uint64(60)),
-					ConnectTime:   ptr.To(uint64(60)),
+					HoldTime:      ptr.To(int64(180)),
+					KeepaliveTime: ptr.To(int64(60)),
+					ConnectTime:   ptr.To(int64(60)),
 				},
 			},
 			toMerge: []*frr.NeighborConfig{

diff --git a/internal/frr/config.go b/internal/frr/config.go
@@ -69,9 +69,9 @@ type NeighborConfig struct {
 	SrcAddr         string
 	Addr            string
 	Port            *uint16
-	HoldTime        *uint64
-	KeepaliveTime   *uint64
-	ConnectTime     *uint64
+	HoldTime        *int64
+	KeepaliveTime   *int64
+	ConnectTime     *int64
 	Password        string
 	BFDProfile      string
 	GracefulRestart bool

diff --git a/internal/frr/frr_test.go b/internal/frr/frr_test.go
@@ -154,9 +154,9 @@ func TestTwoRoutersTwoNeighbors(t *testing.T) {
 						IPFamily:      ipfamily.IPv4,
 						ASN:           "65001",
 						Addr:          "192.168.1.2",
-						HoldTime:      ptr.To[uint64](80),
-						KeepaliveTime: ptr.To[uint64](40),
-						ConnectTime:   ptr.To(uint64(10)),
+						HoldTime:      ptr.To[int64](80),
+						KeepaliveTime: ptr.To[int64](40),
+						ConnectTime:   ptr.To(int64(10)),
 						Outgoing: AllowedOut{
 							PrefixesV4: []OutgoingFilter{
 								{

diff --git a/internal/safeconvert/convertuint32.go b/internal/safeconvert/convertuint32.go
@@ -0,0 +1,21 @@
+//go:build !arm
+// +build !arm
+
+// SPDX-License-Identifier:Apache-2.0
+
+package safeconvert
+
+import (
+	"fmt"
+	"math"
+)
+
+func IntToUInt32(toConvert int) (uint32, error) {
+	if toConvert < 0 {
+		return 0, fmt.Errorf("trying to convert negative value to uint32: %d", toConvert)
+	}
+	if toConvert > math.MaxUint32 {
+		return 0, fmt.Errorf("trying to convert value to uint32: %d, would overflow", toConvert)
+	}
+	return uint32(toConvert), nil
+}
diff --git a/internal/safeconvert/convertuint32_arm.go b/internal/safeconvert/convertuint32_arm.go
@@ -0,0 +1,18 @@
+//go:build arm
+// +build arm
+
+// SPDX-License-Identifier:Apache-2.0
+
+package safeconvert
+
+import (
+	"fmt"
+)
+
+func IntToUInt32(toConvert int) (uint32, error) {
+	if toConvert < 0 {
+		return 0, fmt.Errorf("trying to convert negative value to uint32: %d", toConvert)
+	}
+	// No need to check for upper limit on arm as maxInt is lower than maxuint32
+	return uint32(toConvert), nil
+}