uruk-project / Jwt

JSON Web Token implementation for .Net & .Net Core
MIT License
80 stars 13 forks source link

ECJwk GetCanonicalizeSize wrong - Kid randomly wrong #581

Open inf9144 opened 2 months ago

inf9144 commented 2 months ago

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 commented 2 months ago

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

ycrumeyrolle commented 2 months ago

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.