Skip to content

Commit 2737fba

Browse files
cstocktonChris Stockton
authored andcommitted
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](securego/gosec#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 <[email protected]>
1 parent 907ef22 commit 2737fba

File tree

6 files changed

+13
-12
lines changed

6 files changed

+13
-12
lines changed

internal/api/verify.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -718,7 +718,7 @@ func isOtpValid(actual, expected string, sentAt *time.Time, otpExp uint) bool {
718718
}
719719

720720
func isOtpExpired(sentAt *time.Time, otpExp uint) bool {
721-
return time.Now().After(sentAt.Add(time.Second * time.Duration(otpExp)))
721+
return time.Now().After(sentAt.Add(time.Second * time.Duration(otpExp))) // #nosec G115
722722
}
723723

724724
// isPhoneOtpVerification checks if the verification came from a phone otp

internal/crypto/crypto.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ func (es *EncryptedString) Decrypt(id string, decryptionKeys map[string]string)
112112
return nil, err
113113
}
114114

115-
decrypted, err := cipher.Open(nil, es.Nonce, es.Data, nil)
115+
decrypted, err := cipher.Open(nil, es.Nonce, es.Data, nil) // #nosec G407
116116
if err != nil {
117117
return nil, err
118118
}
@@ -203,7 +203,7 @@ func NewEncryptedString(id string, data []byte, keyID string, keyBase64URL strin
203203
panic(err)
204204
}
205205

206-
es.Data = cipher.Seal(nil, es.Nonce, data, nil)
206+
es.Data = cipher.Seal(nil, es.Nonce, data, nil) // #nosec G407
207207

208208
return &es, nil
209209
}

internal/crypto/password.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ func compareHashAndPasswordArgon2(ctx context.Context, hash, password string) er
141141
attribute.Int64("t", int64(input.time)),
142142
attribute.Int("p", int(input.threads)),
143143
attribute.Int("len", len(input.rawHash)),
144-
}
144+
} // #nosec G115
145145

146146
var match bool
147147
var derivedKey []byte
@@ -157,10 +157,10 @@ func compareHashAndPasswordArgon2(ctx context.Context, hash, password string) er
157157

158158
switch input.alg {
159159
case "argon2i":
160-
derivedKey = argon2.Key([]byte(password), input.salt, uint32(input.time), uint32(input.memory)*1024, uint8(input.threads), uint32(len(input.rawHash)))
160+
derivedKey = argon2.Key([]byte(password), input.salt, uint32(input.time), uint32(input.memory)*1024, uint8(input.threads), uint32(len(input.rawHash))) // #nosec G115
161161

162162
case "argon2id":
163-
derivedKey = argon2.IDKey([]byte(password), input.salt, uint32(input.time), uint32(input.memory)*1024, uint8(input.threads), uint32(len(input.rawHash)))
163+
derivedKey = argon2.IDKey([]byte(password), input.salt, uint32(input.time), uint32(input.memory)*1024, uint8(input.threads), uint32(len(input.rawHash))) // #nosec G115
164164
}
165165

166166
match = subtle.ConstantTimeCompare(derivedKey, input.rawHash) == 0

internal/models/audit_log_entry.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,8 +156,8 @@ func FindAuditLogEntries(tx *storage.Connection, filterColumns []string, filterV
156156
logs := []*AuditLogEntry{}
157157
var err error
158158
if pageParams != nil {
159-
err = q.Paginate(int(pageParams.Page), int(pageParams.PerPage)).All(&logs)
160-
pageParams.Count = uint64(q.Paginator.TotalEntriesSize)
159+
err = q.Paginate(int(pageParams.Page), int(pageParams.PerPage)).All(&logs) // #nosec G115
160+
pageParams.Count = uint64(q.Paginator.TotalEntriesSize) // #nosec G115
161161
} else {
162162
err = q.All(&logs)
163163
}

internal/models/cleanup.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,11 @@ package models
33
import (
44
"context"
55
"fmt"
6+
"sync/atomic"
7+
68
"github.com/sirupsen/logrus"
79
"go.opentelemetry.io/otel"
810
"go.opentelemetry.io/otel/metric"
9-
"sync/atomic"
1011

1112
"go.opentelemetry.io/otel/attribute"
1213

@@ -111,7 +112,7 @@ func (c *Cleanup) Clean(db *storage.Connection) (int, error) {
111112
defer span.SetAttributes(attribute.Int64("gotrue.cleanup.affected_rows", int64(affectedRows)))
112113

113114
if err := db.WithContext(ctx).Transaction(func(tx *storage.Connection) error {
114-
nextIndex := atomic.AddUint32(&c.cleanupNext, 1) % uint32(len(c.cleanupStatements))
115+
nextIndex := atomic.AddUint32(&c.cleanupNext, 1) % uint32(len(c.cleanupStatements)) // #nosec G115
115116
statement := c.cleanupStatements[nextIndex]
116117

117118
count, terr := tx.RawQuery(statement).ExecWithCount()

internal/models/user.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -683,8 +683,8 @@ func FindUsersInAudience(tx *storage.Connection, aud string, pageParams *Paginat
683683

684684
var err error
685685
if pageParams != nil {
686-
err = q.Paginate(int(pageParams.Page), int(pageParams.PerPage)).All(&users)
687-
pageParams.Count = uint64(q.Paginator.TotalEntriesSize)
686+
err = q.Paginate(int(pageParams.Page), int(pageParams.PerPage)).All(&users) // #nosec G115
687+
pageParams.Count = uint64(q.Paginator.TotalEntriesSize) // #nosec G115
688688
} else {
689689
err = q.All(&users)
690690
}

0 commit comments

Comments
 (0)