Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: fix gosec warnings via ignore annotations in comments #1770

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

cstockton
Copy link
Contributor

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. 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:

	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.

@cstockton cstockton requested a review from a team as a code owner September 6, 2024 21:28
@coveralls
Copy link

Pull Request Test Coverage Report for Build 10745487850

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 57.92%

Totals Coverage Status
Change from base Build 10684529517: 0.0%
Covered Lines: 9138
Relevant Lines: 15777

💛 - Coveralls

@hf
Copy link
Contributor

hf commented Sep 9, 2024

The linter accepts chore wow.

@hf hf merged commit a6c1824 into supabase:master Sep 9, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants