From 7a0da0680e527cea6b4c051eb93fcaf994b44c92 Mon Sep 17 00:00:00 2001 From: Don Browne Date: Wed, 21 Aug 2024 12:31:17 +0100 Subject: [PATCH] Fix integer overflow lint errors (#4227) Fixed through a mixture of type changes, and ignoring the linter in places where I was confident that we would never hit an overflow in practice. --- cmd/cli/app/provider/provider_enroll.go | 4 ++-- cmd/server/app/history_purge_test.go | 2 +- database/query/eval_history.sql | 2 +- internal/crypto/nonce.go | 11 +++++++++-- internal/db/embedded/testdb.go | 6 ++++-- internal/db/entity_execution_lock.sql_test.go | 4 ++-- internal/db/eval_history.sql.go | 4 ++-- internal/engine/ingester/diff/diff.go | 1 + internal/history/service.go | 2 +- internal/util/cli/styles.go | 9 ++++++++- internal/util/helpers.go | 6 ++++++ internal/util/rand/random.go | 6 ++++-- 12 files changed, 41 insertions(+), 16 deletions(-) diff --git a/cmd/cli/app/provider/provider_enroll.go b/cmd/cli/app/provider/provider_enroll.go index bd9a42cac5..88efc98fee 100644 --- a/cmd/cli/app/provider/provider_enroll.go +++ b/cmd/cli/app/provider/provider_enroll.go @@ -215,7 +215,7 @@ func enrollUsingOAuth2Flow( resp, err := oauthClient.GetAuthorizationURL(ctx, &minderv1.GetAuthorizationURLRequest{ Context: &minderv1.Context{Provider: &providerName, Project: &project}, Cli: true, - Port: int32(port), + Port: port, Owner: &owner, Config: providerConfig, ProviderClass: providerClass, @@ -240,7 +240,7 @@ func enrollUsingOAuth2Flow( done := make(chan bool) - go callBackServer(oAuthCallbackCtx, cmd, project, port, done, oauthClient, openTime, resp.GetState()) + go callBackServer(oAuthCallbackCtx, cmd, project, int(port), done, oauthClient, openTime, resp.GetState()) success := <-done diff --git a/cmd/server/app/history_purge_test.go b/cmd/server/app/history_purge_test.go index b8d70a6a46..d0990adde3 100644 --- a/cmd/server/app/history_purge_test.go +++ b/cmd/server/app/history_purge_test.go @@ -47,7 +47,7 @@ func TestRecordSize(t *testing.T) { }, ) - require.Equal(t, 96, int(size)) + require.Equal(t, uintptr(96), size) } func TestPurgeLoop(t *testing.T) { diff --git a/database/query/eval_history.sql b/database/query/eval_history.sql index 945e0b2c94..9c99822e36 100644 --- a/database/query/eval_history.sql +++ b/database/query/eval_history.sql @@ -213,7 +213,7 @@ SELECT s.id::uuid AS evaluation_id, ORDER BY CASE WHEN sqlc.narg(next)::timestamp without time zone IS NULL THEN s.evaluation_time END ASC, CASE WHEN sqlc.narg(prev)::timestamp without time zone IS NULL THEN s.evaluation_time END DESC - LIMIT sqlc.arg(size)::integer; + LIMIT sqlc.arg(size)::bigint; -- name: ListEvaluationHistoryStaleRecords :many SELECT s.evaluation_time, diff --git a/internal/crypto/nonce.go b/internal/crypto/nonce.go index a678fd3d08..e442d89615 100644 --- a/internal/crypto/nonce.go +++ b/internal/crypto/nonce.go @@ -18,6 +18,7 @@ import ( "crypto/rand" "encoding/base64" "encoding/binary" + "math" "time" ) @@ -49,9 +50,15 @@ func IsNonceValid(nonce string, noncePeriod int64) (bool, error) { return false, nil } - storedTimestamp := int64(binary.BigEndian.Uint64(nonceBytes[:8])) + storedTimestamp := binary.BigEndian.Uint64(nonceBytes[:8]) + if storedTimestamp > math.MaxInt64 { + // stored timestamp is before Unix epoch -> bad value + return false, nil + } currentTimestamp := time.Now().Unix() - timeDiff := currentTimestamp - storedTimestamp + // Checked explicitly for overflow in stored timestamp + // nolint: gosec + timeDiff := currentTimestamp - int64(storedTimestamp) if timeDiff > noncePeriod { // 5 minutes = 300 seconds return false, nil diff --git a/internal/db/embedded/testdb.go b/internal/db/embedded/testdb.go index c3b6d4f0af..774e868049 100644 --- a/internal/db/embedded/testdb.go +++ b/internal/db/embedded/testdb.go @@ -81,11 +81,13 @@ func GetFakeStore() (db.Store, CancelFunc, error) { return db.NewStore(sqlDB), cancel, nil } -func pickUnusedPort() (int, error) { +func pickUnusedPort() (int32, error) { l, err := net.Listen("tcp", "localhost:0") if err != nil { return 0, err } defer l.Close() - return l.Addr().(*net.TCPAddr).Port, nil + // largest TCP port is 2^16, overflow should not happen + // nolint: gosec + return int32(l.Addr().(*net.TCPAddr).Port), nil } diff --git a/internal/db/entity_execution_lock.sql_test.go b/internal/db/entity_execution_lock.sql_test.go index 6ce93a381f..185150f936 100644 --- a/internal/db/entity_execution_lock.sql_test.go +++ b/internal/db/entity_execution_lock.sql_test.go @@ -81,8 +81,8 @@ func TestQueries_LockIfThresholdNotExceeded(t *testing.T) { wg.Wait() - assert.Equal(t, int32(concurrentCalls-1), queueCount.Load(), "expected all but one to be queued") - assert.Equal(t, int32(1), effectiveFlush.Load(), "expected only one flush to be queued") + assert.Equal(t, concurrentCalls-1, int(queueCount.Load()), "expected all but one to be queued") + assert.Equal(t, 1, int(effectiveFlush.Load()), "expected only one flush to be queued") t.Log("sleeping for threshold") time.Sleep(time.Duration(threshold) * time.Second) diff --git a/internal/db/eval_history.sql.go b/internal/db/eval_history.sql.go index ebcb5b1e3c..9b6bd7f052 100644 --- a/internal/db/eval_history.sql.go +++ b/internal/db/eval_history.sql.go @@ -405,7 +405,7 @@ SELECT s.id::uuid AS evaluation_id, ORDER BY CASE WHEN $1::timestamp without time zone IS NULL THEN s.evaluation_time END ASC, CASE WHEN $2::timestamp without time zone IS NULL THEN s.evaluation_time END DESC - LIMIT $18::integer + LIMIT $18::bigint ` type ListEvaluationHistoryParams struct { @@ -426,7 +426,7 @@ type ListEvaluationHistoryParams struct { Fromts sql.NullTime `json:"fromts"` Tots sql.NullTime `json:"tots"` Projectid uuid.UUID `json:"projectid"` - Size int32 `json:"size"` + Size int64 `json:"size"` } type ListEvaluationHistoryRow struct { diff --git a/internal/engine/ingester/diff/diff.go b/internal/engine/ingester/diff/diff.go index ae005bbeab..db9744fbee 100644 --- a/internal/engine/ingester/diff/diff.go +++ b/internal/engine/ingester/diff/diff.go @@ -218,6 +218,7 @@ func ingestFileForFullDiff(filename, patch, patchUrl string) (*pbinternal.PrCont result = append(result, &pbinternal.PrContents_File_Line{ Content: line[1:], // see the use of strconv.ParseInt above: this is a safe downcast + // nolint: gosec LineNumber: int32(currentLineNumber), }) diff --git a/internal/history/service.go b/internal/history/service.go index 447aba0cb8..7f3ba84326 100644 --- a/internal/history/service.go +++ b/internal/history/service.go @@ -204,7 +204,7 @@ func (_ *evaluationHistoryService) ListEvaluationHistory( filter ListEvaluationFilter, ) (*ListEvaluationHistoryResult, error) { params := db.ListEvaluationHistoryParams{ - Size: int32(size), + Size: int64(size), } if err := toSQLCursor(cursor, ¶ms); err != nil { diff --git a/internal/util/cli/styles.go b/internal/util/cli/styles.go index ba241ae56a..bb2a4191e3 100644 --- a/internal/util/cli/styles.go +++ b/internal/util/cli/styles.go @@ -16,6 +16,7 @@ package cli import ( + "math" "os" "github.com/charmbracelet/lipgloss" @@ -78,7 +79,13 @@ var ( func init() { // Get the terminal width, if available, and set widths based on terminal width - w, _, err := term.GetSize(int(os.Stdout.Fd())) + fd := os.Stdout.Fd() + if fd > math.MaxInt32 { + return + } + // checked for overflow explicitly + // nolint: gosec + w, _, err := term.GetSize(int(fd)) if err == nil { DefaultBannerWidth = w } diff --git a/internal/util/helpers.go b/internal/util/helpers.go index 46d288df8e..ec2139fc9e 100644 --- a/internal/util/helpers.go +++ b/internal/util/helpers.go @@ -24,6 +24,7 @@ import ( "fmt" "io" "io/fs" + "math" "net/http" "net/url" "os" @@ -469,6 +470,11 @@ func Int32FromString(v string) (int32, error) { if err != nil { return 0, fmt.Errorf("error converting string to int: %w", err) } + if asInt32 > math.MaxInt32 || asInt32 < math.MinInt32 { + return 0, fmt.Errorf("integer %d cannot fit into int32", asInt32) + } + // already validated overflow + // nolint:gosec return int32(asInt32), nil } diff --git a/internal/util/rand/random.go b/internal/util/rand/random.go index b71b69b8df..90acd3c7f7 100644 --- a/internal/util/rand/random.go +++ b/internal/util/rand/random.go @@ -72,7 +72,7 @@ func RandomFrom[T any](choices []T, seed int64) T { // Marking a nosec here because we want this to listen on all addresses to // ensure a reliable connection chance for the client. This is based on lessons // learned from the sigstore CLI. -func GetRandomPort() (int, error) { +func GetRandomPort() (int32, error) { listener, err := net.Listen("tcp", ":0") // #nosec if err != nil { return 0, err @@ -80,5 +80,7 @@ func GetRandomPort() (int, error) { defer listener.Close() port := listener.Addr().(*net.TCPAddr).Port - return port, nil + // largest TCP port is 2^16, overflow should not happen + // nolint: gosec + return int32(port), nil }