From a6c18243b92b74798b6317e1c35c8a73bc3fd6e1 Mon Sep 17 00:00:00 2001 From: Chris Stockton <180184+cstockton@users.noreply.github.com> Date: Mon, 9 Sep 2024 12:03:14 -0700 Subject: [PATCH] chore: fix gosec warnings via ignore annotations in comments (#1770) ## What kind of change does this PR introduce? Fix to gosec warnings so builds can complete. ## What is the current behavior? The gosec checks are halting builds. ## What is the new behavior? The gosec checks are passing. ## Additional context I didn't see any warnings that led to real vulnerabilities / security issues. That said long term it may be worth adding some defensive bounds checks for a couple of the integer overflow warnings, just to future proof us age the code ages. Given that we allow supabase users to write to the database, not sure we can guarantee a user doesn't provide a bring-your-own-hash singup flow or something like that. Unbound allocations are a prime target for DoS attacks. For the nonce issues, neither is was real issue. Open is not "fixable, see gosec issue [#1211](https://github.com/securego/gosec/issues/1211). For Seal I tried: ``` nonce := make([]byte, cipher.NonceSize()) if _, err := rand.Read(nonce); err != nil { panic(err) } es := EncryptedString{ KeyID: keyID, Algorithm: "aes-gcm-hkdf", Nonce: nonce, } es.Data = cipher.Seal(nil, es.Nonce, data, nil) ``` But it then considers es.Nonce to be stored / hardcoded. The only fix I could get to work was: ```Go nonce := make([]byte, cipher.NonceSize()) if _, err := rand.Read(nonce); err != nil { panic(err) } es := EncryptedString{ KeyID: keyID, Algorithm: "aes-gcm-hkdf", Nonce: nonce, Data: cipher.Seal(nil, nonce, data, nil), } ``` It seems the gosec tool requires using `rand.Read`. I changed the `cipher.NonceSize()` back to `12` (just in case it a numerical constant for a reason) and it started failing again. I think it also checks that cipher.NonceSize() is used as well, just doesn't report that. I ultimately decided to ignore this so there was no changes to crypto functions given the existing code is correct. Co-authored-by: Chris Stockton --- internal/api/verify.go | 2 +- internal/crypto/crypto.go | 4 ++-- internal/crypto/password.go | 6 +++--- internal/models/audit_log_entry.go | 4 ++-- internal/models/cleanup.go | 5 +++-- internal/models/user.go | 4 ++-- 6 files changed, 13 insertions(+), 12 deletions(-) diff --git a/internal/api/verify.go b/internal/api/verify.go index dbd4d76c6..ad5a7d096 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -718,7 +718,7 @@ func isOtpValid(actual, expected string, sentAt *time.Time, otpExp uint) bool { } func isOtpExpired(sentAt *time.Time, otpExp uint) bool { - return time.Now().After(sentAt.Add(time.Second * time.Duration(otpExp))) + return time.Now().After(sentAt.Add(time.Second * time.Duration(otpExp))) // #nosec G115 } // isPhoneOtpVerification checks if the verification came from a phone otp diff --git a/internal/crypto/crypto.go b/internal/crypto/crypto.go index be6a2b5df..236df4b3f 100644 --- a/internal/crypto/crypto.go +++ b/internal/crypto/crypto.go @@ -112,7 +112,7 @@ func (es *EncryptedString) Decrypt(id string, decryptionKeys map[string]string) return nil, err } - decrypted, err := cipher.Open(nil, es.Nonce, es.Data, nil) + decrypted, err := cipher.Open(nil, es.Nonce, es.Data, nil) // #nosec G407 if err != nil { return nil, err } @@ -203,7 +203,7 @@ func NewEncryptedString(id string, data []byte, keyID string, keyBase64URL strin panic(err) } - es.Data = cipher.Seal(nil, es.Nonce, data, nil) + es.Data = cipher.Seal(nil, es.Nonce, data, nil) // #nosec G407 return &es, nil } diff --git a/internal/crypto/password.go b/internal/crypto/password.go index bce145a45..49db8b6a3 100644 --- a/internal/crypto/password.go +++ b/internal/crypto/password.go @@ -141,7 +141,7 @@ func compareHashAndPasswordArgon2(ctx context.Context, hash, password string) er attribute.Int64("t", int64(input.time)), attribute.Int("p", int(input.threads)), attribute.Int("len", len(input.rawHash)), - } + } // #nosec G115 var match bool var derivedKey []byte @@ -157,10 +157,10 @@ func compareHashAndPasswordArgon2(ctx context.Context, hash, password string) er switch input.alg { case "argon2i": - derivedKey = argon2.Key([]byte(password), input.salt, uint32(input.time), uint32(input.memory)*1024, uint8(input.threads), uint32(len(input.rawHash))) + derivedKey = argon2.Key([]byte(password), input.salt, uint32(input.time), uint32(input.memory)*1024, uint8(input.threads), uint32(len(input.rawHash))) // #nosec G115 case "argon2id": - derivedKey = argon2.IDKey([]byte(password), input.salt, uint32(input.time), uint32(input.memory)*1024, uint8(input.threads), uint32(len(input.rawHash))) + derivedKey = argon2.IDKey([]byte(password), input.salt, uint32(input.time), uint32(input.memory)*1024, uint8(input.threads), uint32(len(input.rawHash))) // #nosec G115 } match = subtle.ConstantTimeCompare(derivedKey, input.rawHash) == 0 diff --git a/internal/models/audit_log_entry.go b/internal/models/audit_log_entry.go index 52d9f6d4a..5bbc9b0b6 100644 --- a/internal/models/audit_log_entry.go +++ b/internal/models/audit_log_entry.go @@ -156,8 +156,8 @@ func FindAuditLogEntries(tx *storage.Connection, filterColumns []string, filterV logs := []*AuditLogEntry{} var err error if pageParams != nil { - err = q.Paginate(int(pageParams.Page), int(pageParams.PerPage)).All(&logs) - pageParams.Count = uint64(q.Paginator.TotalEntriesSize) + err = q.Paginate(int(pageParams.Page), int(pageParams.PerPage)).All(&logs) // #nosec G115 + pageParams.Count = uint64(q.Paginator.TotalEntriesSize) // #nosec G115 } else { err = q.All(&logs) } diff --git a/internal/models/cleanup.go b/internal/models/cleanup.go index 46d4c2bdd..69cf7c7a3 100644 --- a/internal/models/cleanup.go +++ b/internal/models/cleanup.go @@ -3,10 +3,11 @@ package models import ( "context" "fmt" + "sync/atomic" + "github.com/sirupsen/logrus" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/metric" - "sync/atomic" "go.opentelemetry.io/otel/attribute" @@ -111,7 +112,7 @@ func (c *Cleanup) Clean(db *storage.Connection) (int, error) { defer span.SetAttributes(attribute.Int64("gotrue.cleanup.affected_rows", int64(affectedRows))) if err := db.WithContext(ctx).Transaction(func(tx *storage.Connection) error { - nextIndex := atomic.AddUint32(&c.cleanupNext, 1) % uint32(len(c.cleanupStatements)) + nextIndex := atomic.AddUint32(&c.cleanupNext, 1) % uint32(len(c.cleanupStatements)) // #nosec G115 statement := c.cleanupStatements[nextIndex] count, terr := tx.RawQuery(statement).ExecWithCount() diff --git a/internal/models/user.go b/internal/models/user.go index e50b9647d..2f40e26f9 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -683,8 +683,8 @@ func FindUsersInAudience(tx *storage.Connection, aud string, pageParams *Paginat var err error if pageParams != nil { - err = q.Paginate(int(pageParams.Page), int(pageParams.PerPage)).All(&users) - pageParams.Count = uint64(q.Paginator.TotalEntriesSize) + err = q.Paginate(int(pageParams.Page), int(pageParams.PerPage)).All(&users) // #nosec G115 + pageParams.Count = uint64(q.Paginator.TotalEntriesSize) // #nosec G115 } else { err = q.All(&users) }