Skip to content

Commit

Permalink
chore: fix gosec warnings via ignore annotations in comments (#1770)
Browse files Browse the repository at this point in the history
## 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 <chris.stockton@supabase.io>
  • Loading branch information
cstockton and Chris Stockton authored Sep 9, 2024
1 parent 9d419b4 commit a6c1824
Show file tree
Hide file tree
Showing 6 changed files with 13 additions and 12 deletions.
2 changes: 1 addition & 1 deletion internal/api/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions internal/crypto/crypto.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
6 changes: 3 additions & 3 deletions internal/crypto/password.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions internal/models/audit_log_entry.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
5 changes: 3 additions & 2 deletions internal/models/cleanup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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()
Expand Down
4 changes: 2 additions & 2 deletions internal/models/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down

0 comments on commit a6c1824

Please sign in to comment.