From 116c4d6a4d97886fb8cfc0a2f65e3e8113d62543 Mon Sep 17 00:00:00 2001 From: Ori Braunshtein Date: Tue, 17 Sep 2024 10:16:00 +0300 Subject: [PATCH] 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 --- internal/controller/api_to_config.go | 21 +++++++++++++-------- internal/controller/api_to_config_test.go | 12 ++++++------ internal/controller/merge_test.go | 12 ++++++------ internal/frr/config.go | 6 +++--- internal/frr/frr_test.go | 6 +++--- internal/safeconvert/convertuint32.go | 21 +++++++++++++++++++++ internal/safeconvert/convertuint32_arm.go | 18 ++++++++++++++++++ 7 files changed, 70 insertions(+), 26 deletions(-) create mode 100644 internal/safeconvert/convertuint32.go create mode 100644 internal/safeconvert/convertuint32_arm.go diff --git a/internal/controller/api_to_config.go b/internal/controller/api_to_config.go index becf6a45..1d7f992b 100644 --- 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 index d0afcf01..a81dfd36 100644 --- 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 index a546eae5..034e279c 100644 --- a/internal/controller/merge_test.go +++ b/internal/controller/merge_test.go @@ -1309,9 +1309,9 @@ func TestMergeNeighbors(t *testing.T) { Name: "65040@192.0.1.20", 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: "65040@192.0.1.20", 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 index 21f80206..816f7d77 100644 --- 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 index 2ce7af65..fc69bba2 100644 --- 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 new file mode 100644 index 00000000..c0b28173 --- /dev/null +++ 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 new file mode 100644 index 00000000..de86254e --- /dev/null +++ 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 +}