Estimated signature size calculation for ECDsa and RSA may be incorrect

[jozkee]

Based on recent discussions with @bartonjs where I needed to also obtain an estimated size. As per Jeremy, ECDsa should use 2 * ((KeySize + 7) / 8) and for RSA (KeySize + 7) / 8.

What is currently being used is this: https://github.com/dotnet/runtime/blob/ea4ebaa3c5162bcabc63284ba3b59aa683912af4/src/libraries/Common/src/System/Security/Cryptography/ECDsaCng.SignVerify.cs#L21-L29

and this: https://github.com/dotnet/runtime/blob/ea4ebaa3c5162bcabc63284ba3b59aa683912af4/src/libraries/Common/src/System/Security/Cryptography/RSACng.SignVerify.cs#L62

For ECDsa and RSA respectively.

It is not a bug as the code handles the case where the signature buffer wasn't big enough but I think we should be consistent on how to calculate it, maybe even consider adding a GetSignatureSize() API.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones See info in area-owners.md if you want to be subscribed.

Issue Details

---

Based on recent discussions with @bartonjs where I needed to also obtain an estimated size. As per Jeremy, ECDsa should use `2 * ((KeySize + 7) / 8)` and for RSA `(KeySize + 7) / 8`. What is currently being used is this: https://github.com/dotnet/runtime/blob/ea4ebaa3c5162bcabc63284ba3b59aa683912af4/src/libraries/Common/src/System/Security/Cryptography/ECDsaCng.SignVerify.cs#L21-L29 and this: https://github.com/dotnet/runtime/blob/ea4ebaa3c5162bcabc63284ba3b59aa683912af4/src/libraries/Common/src/System/Security/Cryptography/RSACng.SignVerify.cs#L62 For ECDsa and RSA respectively. **It is not a bug** as the code handles the case where the signature buffer wasn't big enough but I think we should be consistent on how to calculate it, maybe even consider adding a `GetSignatureSize()` API.

Author:	Jozkee
Assignees:	-
Labels:	`area-System.Security`, `untriaged`
Milestone:	-

[filipnavara]

The difference is actually just the rounding. For common key sizes that's not going to be a problem since they happen to round up nicely, or they are already special cased in ECDsa (521).

[vcsjones]

maybe even consider adding a GetSignatureSize() API.

I could see the utility in that. I would probably do something more along the lines of:

namespace System.Security.Cryptography {
    public partial class ECDsa {
        public int GetMaxSignatureSize(DSASignatureFormat signatureFormat);
    }

    public partial class DSA {
        public int GetMaxSignatureSize(DSASignatureFormat signatureFormat);
    }

    public partial class RSA {
        public int GetMaxSignatureSize();
    }
}

The Max in the name is for the case of Rfc3279DerSequence, where we don’t know the actual signature size until we actually have the signature. For that case, we would just assume “worst case”, where R,S need a leading zero for sign purposes, plus all the DER overhead.

[bartonjs]

Yeah, we use those calculations all over the place. Making there be methods seems reasonable.

[vcsjones]

Making there be methods seems reasonable.

I made the logical leap to public methods, thus an API proposal, but maybe that wasn't intended. Though, I do think that these methods would be handy with TrySign. Presumably then we wouldn't need to spin until we find a buffer big enough.

[bartonjs]

Adding them as public API, and also adding the non-Try versions of the Span-writing methods, seems good.

Since RSA has always had a key-sized output, I don't think Max is needed in the name there. If we ever add some non-fixed-size thing then I'll eat my crow :smile:

For the -DSA types we could also add a non-Max version like GetIeeeP1363SignatureSize()

[bartonjs]

For the -DSA types we could also add a non-Max version like GetIeeeP1363SignatureSize()

I'm going to change my suggestion to GetSignatureSizeIeeeP1363() so that (a) any other future signature format (golly I hope there aren't any) will appear grouped up with this one in docs/IntelliSense and (b) it doesn't feel like "Max" is an alternative to IEEE-P1363.

[vcsjones]

Since RSA has always had a key-sized output, I don't think Max is needed in the name there. If we ever add some non-fixed-size thing then I'll eat my crow

Yeah, I just liked everything having a neat name.

I'm going to change my suggestion to GetSignatureSizeIeeeP1363()

Hm. If we had SignDataIeeeP1363(...) then I would be inclined to agree... but that's not what we have. I like the clarity of the name but it feels like it's different than what SignData produces. I say that standing on a very very small hill.

In that sense then, the proposal looks like:

namespace System.Security.Cryptography {
    public partial class ECDsa {
        public int GetMaxSignatureSize(DSASignatureFormat signatureFormat);
        public int GetSignatureSizeIeeeP1363();
    }
}

Okay, but that is a little weird to have one dedicated to IEEEP1363 and another one for DSASignatureFormat, so why not a method per enum member.

namespace System.Security.Cryptography {
    public partial class ECDsa {
        public int GetMaxSignatureSizeRfc3279();
        public int GetSignatureSizeIeeeP1363();
    }
}

Well, that works but maybe it's a bit odd that we have the enum and don't use it. If a caller had a DSASignatureFormat, they're going to have to switch on it to call the right method.

The API shape I had in mind was:

public void SignMyStuff(DSASignatureFormat format, ReadOnlySpan<byte> data) {
    ECDsa ecdsa = ECDsa.Create();
    // Set up ecdsa
    byte[] rented = ArrayPool<byte>.Shared.Rent(ecdsa.GetMaxSignatureSize(format));
    bool didItWork = ecdsa.TrySign(data, rented, HashAlgorithmName.SHA256, format, out int written);
    Debug.Assert(didItWork);
    ReadOnlySpan<byte> signature = rented.AsSpan(0, written);
    // Do something with signature
    ArrayPool<byte>.Shared.Return(rented, true);
}

While I see the utility in knowing exactly how big a signature is (if we can) I envisioned these further as something to say "You need a buffer that is X bytes in size".

If folks are using ArrayPool (or any other pool) then they won't be getting back exactly in size what they asked for, anyway, so they will still need to slice it down to written. So it's okay if it's thought of as a max.

If they are using stackalloc, then they likely already have a Span<byte> so it's cheap and easy to slice.

If they want a byte[] that is the exact size, well then they might as use the allocating one.

Granted, it's only the case of DER DSA signatures that we don't know the absolute size.

[bartonjs]

The reason, to me, for having the P1363 size method is that it's a) simpler and b) matches the Sign/Verify that don't take the format parameter.

If we wanted to pretend we didn't have X509Der support yet then it'd just be GetSignatureSize() on all three algorithms, and then the -DSAs would then also gain GetMaxSignatureSize(DSASignatureFormat).

I do like the one that takes the format, for the reason you pointed out... if you took the format as a parameter you don't want to have to switch on it yourself.

[vcsjones]

[I]f you took the format as a parameter you don't want to have to switch on it yourself.

Yeah. But now to talk myself out of it... that's probably only useful to library / framework authors. Consumers of the APIs will want one format or another. So if library authors (.NET Libraries) need to do a little switching for a better APIs suited for application developers, that makes sense to me.

Okay. So something like

namespace System.Security.Cryptography {
    public partial class ECDsa {
        public int GetMaxSignatureSizeRfc3279();
        public int GetSignatureSizeIeeeP1363();
    }

    public partial class DSA {
        public int GetMaxSignatureSizeRfc3279();
        public int GetSignatureSizeIeeeP1363();
    }

    public partial class RSA {
        public int GetSignatureSize();
    }
}

[vcsjones]

Oh, huh. This API already exists. https://apisof.net/catalog/30126e10-f818-2b5c-0b80-7e7787f3a4d9

[bartonjs]

So we're just missing RSA.GetSignatureSize() => checked((KeySize + 7) / 8);?

[vcsjones]

Seems so.