Skip to content

Commit 7b2f899

Browse files
oribonfedepaol
oribon
authored and
fedepaol
committed
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]>
1 parent 3bfd588 commit 7b2f899

File tree

7 files changed

+70
-26
lines changed

7 files changed

+70
-26
lines changed

‎internal/controller/api_to_config.go

+13-8
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"github.com/metallb/frr-k8s/internal/community"
1616
"github.com/metallb/frr-k8s/internal/frr"
1717
"github.com/metallb/frr-k8s/internal/ipfamily"
18+
"github.com/metallb/frr-k8s/internal/safeconvert"
1819
corev1 "k8s.io/api/core/v1"
1920
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2021
"k8s.io/apimachinery/pkg/util/sets"
@@ -185,7 +186,7 @@ func neighborToFRR(n v1beta1.Neighbor, prefixesInRouter []string, alwaysBlock []
185186
}
186187

187188
if n.ConnectTime != nil {
188-
res.ConnectTime = ptr.To(uint64(n.ConnectTime.Duration / time.Second))
189+
res.ConnectTime = ptr.To(int64(n.ConnectTime.Duration / time.Second))
189190
}
190191

191192
res.Password, err = passwordForNeighbor(n, passwordSecrets)
@@ -373,7 +374,11 @@ func filterForSelector(selector v1beta1.PrefixSelector) (frr.IncomingFilter, err
373374
return frr.IncomingFilter{}, fmt.Errorf("failed to parse prefix %s: %w", selector.Prefix, err)
374375
}
375376
maskLen, _ := cidr.Mask.Size()
376-
err = validateSelectorLengths(maskLen, selector.LE, selector.GE)
377+
maskLenUint, err := safeconvert.IntToUInt32(maskLen)
378+
if err != nil {
379+
return frr.IncomingFilter{}, fmt.Errorf("failed to convert maskLen from CIDR %s to uint32: %w", cidr, err)
380+
}
381+
err = validateSelectorLengths(maskLenUint, selector.LE, selector.GE)
377382
if err != nil {
378383
return frr.IncomingFilter{}, err
379384
}
@@ -390,17 +395,17 @@ func filterForSelector(selector v1beta1.PrefixSelector) (frr.IncomingFilter, err
390395

391396
// validateSelectorLengths checks the lengths respect the following
392397
// condition: mask length <= ge <= le
393-
func validateSelectorLengths(mask int, le, ge uint32) error {
398+
func validateSelectorLengths(mask, le, ge uint32) error {
394399
if ge == 0 && le == 0 {
395400
return nil
396401
}
397402
if le > 0 && ge > le {
398403
return fmt.Errorf("invalid selector lengths: ge %d is bigger than le %d", ge, le)
399404
}
400-
if le > 0 && uint32(mask) > le {
405+
if le > 0 && mask > le {
401406
return fmt.Errorf("invalid selector lengths: cidr mask %d is bigger than le %d", mask, le)
402407
}
403-
if ge > 0 && uint32(mask) > ge {
408+
if ge > 0 && mask > ge {
404409
return fmt.Errorf("invalid selector lengths: cidr mask %d is bigger than ge %d", mask, ge)
405410
}
406411
return nil
@@ -604,7 +609,7 @@ func alwaysBlockToFRR(cidrs []net.IPNet) []frr.IncomingFilter {
604609
return res
605610
}
606611

607-
func parseTimers(ht, ka *v1.Duration) (*uint64, *uint64, error) {
612+
func parseTimers(ht, ka *v1.Duration) (*int64, *int64, error) {
608613
if ht == nil && ka != nil || ht != nil && ka == nil {
609614
return nil, nil, fmt.Errorf("one of KeepaliveTime/HoldTime specified, both must be set or none")
610615
}
@@ -625,8 +630,8 @@ func parseTimers(ht, ka *v1.Duration) (*uint64, *uint64, error) {
625630
return nil, nil, fmt.Errorf("invalid keepaliveTime %q, must be lower than holdTime %q", ka, ht)
626631
}
627632

628-
htSeconds := uint64(holdTime / time.Second)
629-
kaSeconds := uint64(keepaliveTime / time.Second)
633+
htSeconds := int64(holdTime / time.Second)
634+
kaSeconds := int64(keepaliveTime / time.Second)
630635

631636
return &htSeconds, &kaSeconds, nil
632637
}

‎internal/controller/api_to_config_test.go

+6-6
Original file line numberDiff line numberDiff line change
@@ -83,9 +83,9 @@ func TestConversion(t *testing.T) {
8383
Port: ptr.To[uint16](179),
8484
SrcAddr: "192.1.1.1",
8585
Addr: "192.0.2.2",
86-
KeepaliveTime: ptr.To[uint64](20),
87-
HoldTime: ptr.To[uint64](40),
88-
ConnectTime: ptr.To(uint64(2)),
86+
KeepaliveTime: ptr.To[int64](20),
87+
HoldTime: ptr.To[int64](40),
88+
ConnectTime: ptr.To(int64(2)),
8989
DisableMP: true,
9090
GracefulRestart: true,
9191
},
@@ -144,9 +144,9 @@ func TestConversion(t *testing.T) {
144144
ASN: "65002",
145145
Port: ptr.To[uint16](179),
146146
Addr: "192.0.2.2",
147-
KeepaliveTime: ptr.To[uint64](20),
148-
HoldTime: ptr.To[uint64](40),
149-
ConnectTime: ptr.To(uint64(2)),
147+
KeepaliveTime: ptr.To[int64](20),
148+
HoldTime: ptr.To[int64](40),
149+
ConnectTime: ptr.To(int64(2)),
150150
DisableMP: true,
151151
},
152152
},

‎internal/controller/merge_test.go

+6-6
Original file line numberDiff line numberDiff line change
@@ -1309,9 +1309,9 @@ func TestMergeNeighbors(t *testing.T) {
13091309
Name: "[email protected]",
13101310
ASN: "65040",
13111311
Addr: "192.0.1.20",
1312-
HoldTime: ptr.To(uint64(180)),
1313-
KeepaliveTime: ptr.To(uint64(60)),
1314-
ConnectTime: ptr.To(uint64(60)),
1312+
HoldTime: ptr.To(int64(180)),
1313+
KeepaliveTime: ptr.To(int64(60)),
1314+
ConnectTime: ptr.To(int64(60)),
13151315
},
13161316
},
13171317
expected: []*frr.NeighborConfig{
@@ -1341,9 +1341,9 @@ func TestMergeNeighbors(t *testing.T) {
13411341
Name: "[email protected]",
13421342
ASN: "65040",
13431343
Addr: "192.0.1.20",
1344-
HoldTime: ptr.To(uint64(180)),
1345-
KeepaliveTime: ptr.To(uint64(60)),
1346-
ConnectTime: ptr.To(uint64(60)),
1344+
HoldTime: ptr.To(int64(180)),
1345+
KeepaliveTime: ptr.To(int64(60)),
1346+
ConnectTime: ptr.To(int64(60)),
13471347
},
13481348
},
13491349
toMerge: []*frr.NeighborConfig{

‎internal/frr/config.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,9 @@ type NeighborConfig struct {
6969
SrcAddr string
7070
Addr string
7171
Port *uint16
72-
HoldTime *uint64
73-
KeepaliveTime *uint64
74-
ConnectTime *uint64
72+
HoldTime *int64
73+
KeepaliveTime *int64
74+
ConnectTime *int64
7575
Password string
7676
BFDProfile string
7777
GracefulRestart bool

‎internal/frr/frr_test.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -154,9 +154,9 @@ func TestTwoRoutersTwoNeighbors(t *testing.T) {
154154
IPFamily: ipfamily.IPv4,
155155
ASN: "65001",
156156
Addr: "192.168.1.2",
157-
HoldTime: ptr.To[uint64](80),
158-
KeepaliveTime: ptr.To[uint64](40),
159-
ConnectTime: ptr.To(uint64(10)),
157+
HoldTime: ptr.To[int64](80),
158+
KeepaliveTime: ptr.To[int64](40),
159+
ConnectTime: ptr.To(int64(10)),
160160
Outgoing: AllowedOut{
161161
PrefixesV4: []OutgoingFilter{
162162
{

‎internal/safeconvert/convertuint32.go

+21
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
//go:build !arm
2+
// +build !arm
3+
4+
// SPDX-License-Identifier:Apache-2.0
5+
6+
package safeconvert
7+
8+
import (
9+
"fmt"
10+
"math"
11+
)
12+
13+
func IntToUInt32(toConvert int) (uint32, error) {
14+
if toConvert < 0 {
15+
return 0, fmt.Errorf("trying to convert negative value to uint32: %d", toConvert)
16+
}
17+
if toConvert > math.MaxUint32 {
18+
return 0, fmt.Errorf("trying to convert value to uint32: %d, would overflow", toConvert)
19+
}
20+
return uint32(toConvert), nil
21+
}

‎internal/safeconvert/convertuint32_arm.go

+18
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
//go:build arm
2+
// +build arm
3+
4+
// SPDX-License-Identifier:Apache-2.0
5+
6+
package safeconvert
7+
8+
import (
9+
"fmt"
10+
)
11+
12+
func IntToUInt32(toConvert int) (uint32, error) {
13+
if toConvert < 0 {
14+
return 0, fmt.Errorf("trying to convert negative value to uint32: %d", toConvert)
15+
}
16+
// No need to check for upper limit on arm as maxInt is lower than maxuint32
17+
return uint32(toConvert), nil
18+
}

0 commit comments

Comments
 (0)