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

ECJwk GetCanonicalizeSize wrong - Kid randomly wrong #581

Open
inf9144 opened this issue Apr 19, 2024 · 2 comments
Open

ECJwk GetCanonicalizeSize wrong - Kid randomly wrong #581

inf9144 opened this issue Apr 19, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@inf9144
Copy link

inf9144 commented Apr 19, 2024

Hello,
in EC JWK the size is calculated by:

	protected internal override int GetCanonicalizeSize()
	{
		return 35 + Base64Url.GetArraySizeRequiredToEncode(Crv.Name.EncodedUtf8Bytes.Length) + Base64Url.GetArraySizeRequiredToEncode(X.Length) + Base64Url.GetArraySizeRequiredToEncode(Y.Length);
	}

but it should be

	protected internal override int GetCanonicalizeSize()
	{
		return 35 + Crv.Name.EncodedUtf8Bytes.Length + Base64Url.GetArraySizeRequiredToEncode(X.Length) + Base64Url.GetArraySizeRequiredToEncode(Y.Length);
	}

because Crv.Name.EncodedUtf8Bytes is copied in Canonicalize:

	protected internal override void Canonicalize(Span<byte> buffer)
	{
		int length = StartCanonicalizeValue.Length;
		StartCanonicalizeValue.CopyTo(buffer);
		EllipticalCurve ellipticalCurve = Crv;
		ellipticalCurve.Name.EncodedUtf8Bytes.CopyTo(buffer.Slice(length));
		int num = length;
		ellipticalCurve = Crv;
		length = num + ellipticalCurve.Name.EncodedUtf8Bytes.Length;
		Middle1CanonicalizeValue.CopyTo(buffer.Slice(length));
		length += Middle1CanonicalizeValue.Length;
		length += Base64Url.Encode(X, buffer.Slice(length));
		Middle2CanonicalizeValue.CopyTo(buffer.Slice(length));
		length += Middle2CanonicalizeValue.Length;
		length += Base64Url.Encode(Y, buffer.Slice(length));
		EndCanonicalizeValue.CopyTo(buffer.Slice(length));
	}

This is a problem because ComputeThumbprint uses this Value for the call to ComputeHash:

Sha256.Shared.ComputeHash(buffer.Slice(0, canonicalizeSize), span);

The buffer is either from ArrayPool or from Stackalloc - ArrayPool does not garantuee to null the array. So you get random Kids when the last bytes differ because they dont get initialized.

I think the Renting is also wrong - you use Stackalloc if canonicalSize is bigger than 256 - i think the condition should be reversed - using stack alloc for smaller arrays right?

I didnt check for RSA and the others, potentially they share some of those bugs.

I can do PRs if you like. Right now our project is running on your "2.0.0-beta.4" i would like to have a stable version :-)

If you dont do maintenance on this project i would be ready to fork this - your library is awesome and much better than the crap Microsoft implemented for their authorization. Want to see it fly :-)

@ycrumeyrolle
Copy link
Collaborator

Hi, I will try to check this issue this weekend, there is also another to fix.

@ycrumeyrolle
Copy link
Collaborator

ycrumeyrolle commented Apr 22, 2024

This issue was already fixed but not published. This is now available with the package https://www.nuget.org/packages/JsonWebToken/2.0.0-beta.5.

This is still a beta version, because there are still issues with some JSON structures.

@ycrumeyrolle ycrumeyrolle added the bug Something isn't working label Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants