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 BCrypt for ephemeral RSA on Windows #76277

Merged
merged 4 commits into from
Sep 30, 2022
Merged

Conversation

bartonjs
Copy link
Member

Windows CNG has two different libraries: bcrypt.dll (BCrypt* functions) for in-memory/ephemeral operations, and ncrypt.dll (NCrypt* functions) for persisted key operations. Since the NCrypt functions can also operate on ephemeral keys our cryptographic operations have generally been done in terms of NCrypt.

NCrypt's flexibility (to also work with persisted keys) comes at a cost. All key operations are done out-of-process (in lsass.exe), and that requires an (L)RPC call for every operation. It also means there are simply more moving parts, and thus more room for error.

With this change we will use BCrypt for RSA operations on Windows from RSA.Create() and cert.GetRSAPublicKey(). ECDSA/ECDH/DSA can any/all be changed to follow suit later.

For keys created from RSA.Create() a verification operation currently looks like

  • Virtual invoke to RSAWrapper.VerifyHash
  • Maybe-devirtualized invoke to RSACng.VerifyHash
  • P/Invoke to NCryptVerifySignature
  • "Virtual" invoke to MSKSPVerifySignature (or whatever it's really called)
  • LRPC call
  • Find key in the MRU ring
  • Effectively a call to BCryptVerifySignature

After this change it is instead

  • Virtual invoke to RSABCrypt.VerifyHash
  • P/Invoke to BCryptVerifySignature

Removing all of those other indirections removes about 40us from a 50us operation (on my primary devbox).

As part of making some code be shared between RSACng and RSABCrypt, some allocating code changed to pooling code and some array code got spanified.

Might contributes to #25979, either way it improves perf.

@ghost
Copy link

ghost commented Sep 28, 2022

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

Issue Details

Windows CNG has two different libraries: bcrypt.dll (BCrypt* functions) for in-memory/ephemeral operations, and ncrypt.dll (NCrypt* functions) for persisted key operations. Since the NCrypt functions can also operate on ephemeral keys our cryptographic operations have generally been done in terms of NCrypt.

NCrypt's flexibility (to also work with persisted keys) comes at a cost. All key operations are done out-of-process (in lsass.exe), and that requires an (L)RPC call for every operation. It also means there are simply more moving parts, and thus more room for error.

With this change we will use BCrypt for RSA operations on Windows from RSA.Create() and cert.GetRSAPublicKey(). ECDSA/ECDH/DSA can any/all be changed to follow suit later.

For keys created from RSA.Create() a verification operation currently looks like

  • Virtual invoke to RSAWrapper.VerifyHash
  • Maybe-devirtualized invoke to RSACng.VerifyHash
  • P/Invoke to NCryptVerifySignature
  • "Virtual" invoke to MSKSPVerifySignature (or whatever it's really called)
  • LRPC call
  • Find key in the MRU ring
  • Effectively a call to BCryptVerifySignature

After this change it is instead

  • Virtual invoke to RSABCrypt.VerifyHash
  • P/Invoke to BCryptVerifySignature

Removing all of those other indirections removes about 40us from a 50us operation (on my primary devbox).

As part of making some code be shared between RSACng and RSABCrypt, some allocating code changed to pooling code and some array code got spanified.

Might contributes to #25979, either way it improves perf.

Author: bartonjs
Assignees: bartonjs
Labels:

area-System.Security

Milestone: -

@bartonjs
Copy link
Member Author

Perf Table
Method Toolchain Mean Error StdDev Ratio RatioSD Allocated Alloc Ratio
RSACreate PR 49,617.37 us 1,323.591 us 3,881.863 us 0.99 0.10 445 B 0.33
RSACreate net7.0 50,405.02 us 1,312.273 us 3,848.670 us 1.00 0.00 1356 B 1.00
RSASign PR 460.76 us 1.164 us 1.032 us 0.88 0.01 - 0.00
RSASign net7.0 524.24 us 9.687 us 8.587 us 1.00 0.00 56 B 1.00
RSAVerify PR 14.03 us 0.071 us 0.063 us 0.27 0.00 - 0.00
RSAVerify net7.0 52.19 us 0.479 us 0.448 us 1.00 0.00 56 B 1.00
RSAEncrypt PR 17.48 us 0.050 us 0.042 us 0.24 0.00 - 0.00
RSAEncrypt net7.0 74.10 us 0.861 us 0.805 us 1.00 0.00 56 B 1.00
RSADecrypt PR 467.30 us 0.850 us 0.753 us 0.89 0.00 - 0.00
RSADecrypt net7.0 523.77 us 1.628 us 1.360 us 1.00 0.00 57 B 1.00
RSAVerifyFromCert PR 27.78 us 0.081 us 0.072 us 0.08 0.00 696 B 0.69
RSAVerifyFromCert net7.0 357.09 us 2.296 us 2.035 us 1.00 0.00 1008 B 1.00
Benchmark Code
    private static RSA _rsa = RSA.Create();
    private static byte[] _sig = _rsa.SignHash(new byte[256 / 8], HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1);
    private static byte[] _enc = _rsa.Encrypt(new byte[3], RSAEncryptionPadding.OaepSHA256);

    private static X509Certificate2 s_cert = new X509Certificate2(Convert.FromBase64String(@"
MIIE7DCCA9SgAwIBAgITMwAAALARrwqL0Duf3QABAAAAsDANBgkqhkiG9w0BAQUF
ADB5MQswCQYDVQQGEwJVUzETMBEGA1UECBMKV2FzaGluZ3RvbjEQMA4GA1UEBxMH
UmVkbW9uZDEeMBwGA1UEChMVTWljcm9zb2Z0IENvcnBvcmF0aW9uMSMwIQYDVQQD
ExpNaWNyb3NvZnQgQ29kZSBTaWduaW5nIFBDQTAeFw0xMzAxMjQyMjMzMzlaFw0x
NDA0MjQyMjMzMzlaMIGDMQswCQYDVQQGEwJVUzETMBEGA1UECBMKV2FzaGluZ3Rv
bjEQMA4GA1UEBxMHUmVkbW9uZDEeMBwGA1UEChMVTWljcm9zb2Z0IENvcnBvcmF0
aW9uMQ0wCwYDVQQLEwRNT1BSMR4wHAYDVQQDExVNaWNyb3NvZnQgQ29ycG9yYXRp
b24wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDor1yiIA34KHy8BXt/
re7rdqwoUz8620B9s44z5lc/pVEVNFSlz7SLqT+oN+EtUO01Fk7vTXrbE3aIsCzw
WVyp6+HXKXXkG4Unm/P4LZ5BNisLQPu+O7q5XHWTFlJLyjPFN7Dz636o9UEVXAhl
HSE38Cy6IgsQsRCddyKFhHxPuRuQsPWj/ov0DJpOoPXJCiHiquMBNkf9L4JqgQP1
qTXclFed+0vUDoLbOI8S/uPWenSIZOFixCUuKq6dGB8OHrbCryS0DlC83hyTXEmm
ebW22875cHsoAYS4KinPv6kFBeHgD3FN/a1cI4Mp68fFSsjoJ4TTfsZDC5UABbFP
ZXHFAgMBAAGjggFgMIIBXDATBgNVHSUEDDAKBggrBgEFBQcDAzAdBgNVHQ4EFgQU
WXGmWjNN2pgHgP+EHr6H+XIyQfIwUQYDVR0RBEowSKRGMEQxDTALBgNVBAsTBE1P
UFIxMzAxBgNVBAUTKjMxNTk1KzRmYWYwYjcxLWFkMzctNGFhMy1hNjcxLTc2YmMw
NTIzNDRhZDAfBgNVHSMEGDAWgBTLEejK0rQWWAHJNy4zFha5TJoKHzBWBgNVHR8E
TzBNMEugSaBHhkVodHRwOi8vY3JsLm1pY3Jvc29mdC5jb20vcGtpL2NybC9wcm9k
dWN0cy9NaWNDb2RTaWdQQ0FfMDgtMzEtMjAxMC5jcmwwWgYIKwYBBQUHAQEETjBM
MEoGCCsGAQUFBzAChj5odHRwOi8vd3d3Lm1pY3Jvc29mdC5jb20vcGtpL2NlcnRz
L01pY0NvZFNpZ1BDQV8wOC0zMS0yMDEwLmNydDANBgkqhkiG9w0BAQUFAAOCAQEA
MdduKhJXM4HVncbr+TrURE0Inu5e32pbt3nPApy8dmiekKGcC8N/oozxTbqVOfsN
4OGb9F0kDxuNiBU6fNutzrPJbLo5LEV9JBFUJjANDf9H6gMH5eRmXSx7nR2pEPoc
sHTyT2lrnqkkhNrtlqDfc6TvahqsS2Ke8XzAFH9IzU2yRPnwPJNtQtjofOYXoJto
aAko+QKX7xEDumdSrcHps3Om0mPNSuI+5PNO/f+h4LsCEztdIN5VP6OukEAxOHUo
XgSpRm3m9Xp5QL0fzehF1a7iXT71dcfmZmNgzNWahIeNJDD37zTQYx2xQmdKDku/
Og7vtpU6pzjkJZIIpohmgg=="));

        using (RSA rsa = RSA.Create())
        {
            rsa.ExportParameters(false);
        }
    }

    [Benchmark]
    public void RSASign()
    {
        Span<byte> hash = stackalloc byte[256 >> 3];
        Span<byte> destination = stackalloc byte[_rsa.KeySize >> 3];
        _rsa.TrySignHash(hash, destination, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1, out _);
    }

    [Benchmark]
    public void RSAVerify()
    {
        Span<byte> hash = stackalloc byte[256 >> 3];
        _rsa.VerifyHash(hash, _sig, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1);
    }

    [Benchmark]
    public void RSAEncrypt()
    {
        Span<byte> source = stackalloc byte[3];
        Span<byte> destination = stackalloc byte[_rsa.KeySize >> 3];
        _rsa.TryEncrypt(source, destination, RSAEncryptionPadding.OaepSHA256, out _);
    }

    [Benchmark]
    public void RSADecrypt()
    {
        Span<byte> destination = stackalloc byte[_rsa.KeySize >> 3];
        _rsa.TryDecrypt(_enc, destination, RSAEncryptionPadding.OaepSHA256, out _);
    }

    [Benchmark]
    public void RSAVerifyFromCert()
    {
        using (RSA rsa = s_cert.GetRSAPublicKey())
        {
            Span<byte> sig = stackalloc byte[rsa.KeySize >> 3];
            ReadOnlySpan<byte> hash = sig.Slice(0, 256 >> 3);
            rsa.VerifyHash(hash, sig, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1);
        }
    }

@vcsjones
Copy link
Member

There's a decent amount of new p/invoke in here. Perhaps it would be worthwhile to run tests with DEBUG_SAFEHANDLE_FINALIZATION=1 to look for handle finalization?

@bartonjs
Copy link
Member Author

Perhaps it would be worthwhile to run tests with DEBUG_SAFEHANDLE_FINALIZATION=1 to look for handle finalization?

All of the finalizations of SafeBCryptKeyHandle come via tests not disposing, or via X509Certificate2.CreateFromPem. Internally it looks OK.

Copy link
Member

@vcsjones vcsjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Had some questions / suggestions but they are not things I would complain about if it got merged as-is.

@bartonjs bartonjs merged commit fbc2232 into dotnet:main Sep 30, 2022
@bartonjs bartonjs deleted the bcrypt_keys branch September 30, 2022 23:35
@ghost ghost locked as resolved and limited conversation to collaborators Oct 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants