dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.36k stars 4.74k forks source link

Mangled code in CngCommon throws away errors #101520

Open jhudsoncedaron opened 6 months ago

jhudsoncedaron commented 6 months ago

Description

This code is from namespace System.Security.Cryptography.CngCommon

    public unsafe static bool VerifyHash(this SafeNCryptKeyHandle keyHandle, ReadOnlySpan<byte> hash, ReadOnlySpan<byte> signature, global::Interop.NCrypt.AsymmetricPaddingMode paddingMode, void* pPaddingInfo)
    {
        global::Interop.NCrypt.ErrorCode errorCode = global::Interop.NCrypt.NCryptVerifySignature(keyHandle, pPaddingInfo, hash, hash.Length, signature, signature.Length, paddingMode);
        if (errorCode == global::Interop.NCrypt.ErrorCode.STATUS_UNSUCCESSFUL)
        {
            errorCode = global::Interop.NCrypt.NCryptVerifySignature(keyHandle, pPaddingInfo, hash, hash.Length, signature, signature.Length, paddingMode);
        }
        return errorCode == global::Interop.NCrypt.ErrorCode.ERROR_SUCCESS;
    }

I got to the return statement with errorCode = NTE_INVALID_PARAMETER; trying to validate a known valid ECDSA signature. I would have expected either a successful validation or a throw along the lines of NotImplementedException but nope just says false. On debugging; well ...

I have no idea why STATUS_UNSUCCESSFUL means try once more than fail; but it seems to be a repeating pattern in this file.

Reproduction Steps

To reproduce call

using System;
using System.Security.Cryptography;

Console.WriteLine(System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription);
using ECDsa ec = ECDsa.Create();
ReadOnlySpan<byte> sig = ec.SignData(Array.Empty<byte>(), HashAlgorithmName.SHA256, DSASignatureFormat.Rfc3279DerSequence);
Console.WriteLine(ec.VerifyData([], sig, HashAlgorithmName.SHA256, DSASignatureFormat.IeeeP1363FixedFieldConcatenation));

Should throw but prints false. The signature isn't wrong. The signature is malformed.

Code is clearly wrong by inspection. Any arbitrary failure in NCryptVerifySignature just says "no, that's not a valid signature".

Expected behavior

Immediately, I expect it to throw; ultimately I expect it to return true from ECDSA.Verify but that's a different issue.

Actual behavior

Bogus false from ECDA.VerifyHash

Regression?

No response

Known Workarounds

Use third party library

Configuration

.NET 8, Windows x64

Other information

No response

vcsjones commented 6 months ago

The code in question is here:

https://github.com/dotnet/runtime/blob/5b4e770daa190ce69f402c1cf92f388b07d3e144/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/CngCommon.SignVerify.cs#L84-L93

I have no idea why STATUS_UNSUCCESSFUL means try once more than fail; but it seems to be a repeating pattern in this file.

This was introduced because CNG, sometimes, is not able to successfully determine if the signature was valid or not. It's a retry and was introduced in https://github.com/dotnet/corefx/pull/41300.

I got to the return statement with errorCode = NTE_INVALID_PARAMETER;

I cannot reproduce this. On .NET 8, the following prints true.

using System;
using System.Security.Cryptography;

Console.WriteLine(System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription);
using ECDsa ec = ECDsa.Create();
ReadOnlySpan<byte> sig = ec.SignData(Array.Empty<byte>(), HashAlgorithmName.SHA256, DSASignatureFormat.Rfc3279DerSequence);
Console.WriteLine(ec.VerifyData([], sig, HashAlgorithmName.SHA256, DSASignatureFormat.Rfc3279DerSequence));

Can you please include a complete, fully runnable application that reproduces the issue you are facing?

jhudsoncedaron commented 6 months ago

@vcsjones Updated reproduction using your sample as baseline. Should throw, but returns false.

vcsjones commented 1 month ago

I think everything is working here as expected.

All of the Verify methods for asymmetric signatures VerifyHash, VerifyData, etc. return false for any signature that does not validate for whatever reason. Be it because the signature is invalid, malformed, or empty.

Changing signature validation to throw on malformed signatures would be a fairly significant breaking change, I would think.

jhudsoncedaron commented 1 month ago

@vcsjones : Think about the meaning of " Any arbitrary failure in NCryptVerifySignature"; if it returns the error code for out of memory you still say it's not valid.