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

Use a CSPRNG to generate the code verifier #99

Merged

Conversation

alexbakker
Copy link
Contributor

The PKCE for OAuth spec requires that the code verifier be a "high-entropy cryptographic random string": https://datatracker.ietf.org/doc/html/rfc7636#section-4.1

Previously, the GenerateNonce function was using System.Random to generate the code verifier, which is not cryptographically secure.

TargetFramework has been bumped to netstandard2.1 in order to get access to RandomNumberGenerator.GetInt32.

The PKCE for OAuth spec requires that the code verifier be a
"high-entropy cryptographic random string":
https://datatracker.ietf.org/doc/html/rfc7636#section-4.1

Previously, the ``GenerateNonce`` function was using ``System.Random``
to generate the code verifier, which is not cryptographically secure.

TargetFramework has been bumped to netstandard2.1 in order to get access
to ``RandomNumberGenerator.GetInt32``.
@acupofjose acupofjose merged commit b7ccac5 into supabase-community:master Jul 12, 2024
1 check passed
@acupofjose
Copy link
Contributor

Appreciate your work on this @alexbakker!

@acupofjose
Copy link
Contributor

acupofjose commented Jul 13, 2024

@alexbakker I'm returning to this PR and would appreciate your feedback - would the following suffice if we went back down to netstandard2.0?

public static string GenerateNonce()
{
	// ReSharper disable once StringLiteralTypo
	const string chars = "abcdefghijklmnopqrstuvwxyz123456789";
	var nonce = new char[128];
	var rng = RandomNumberGenerator.Create();
	for (var i = 0; i < nonce.Length; i++)
	{
		var rngBytes = new byte[4];
		rng.GetBytes(rngBytes);
		var index = BitConverter.ToUInt16(rngBytes, 0) % (chars.Length - 1);
		nonce[i] = chars[index];
	}

	return new string(nonce);
}

Using this for reference on cryptographically strong RNG.

@alexbakker
Copy link
Contributor Author

@acupofjose As I explained in our exchange over email, I don't feel comfortable getting a reimplementation of RandomNumberGenerator.GetInt32 right, and this is a good example of the reason why.

This approach looks correct upon first glance, but the problem is that the remainder calculation introduces a very slight bias towards the first couple of characters.

It's better to rely on the standard library implementation.

@acupofjose
Copy link
Contributor

Got it! Thanks for the clarification @alexbakker

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.

2 participants