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.

Sign up for GitHub

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

Jump to bottom

Windows CNG virtualization-based security #102495

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Windows CNG virtualization-based security #102495

wants to merge 4 commits into from
+199 −5

Conversation

krwq
Copy link
Member

@krwq krwq commented May 21, 2024 •
edited

Fixes: #102492

One of the Windows 11 builds has added framework to help secure Windows keys with virtualization-based security (VBS). With this new capability, keys can be protected from admin-level key theft attacks with negligible effect on performance, reliability, or scale.

Blog post:
https://techcommunity.microsoft.com/t5/windows-it-pro-blog/advancing-key-protection-in-windows-using-vbs/ba-p/4050988

Win API:
https://learn.microsoft.com/en-us/windows/win32/api/ncrypt/nf-ncrypt-ncryptcreatepersistedkey

The proposal is to extend existing CngKeyCreationOptions API to include the new flags.

API Proposal

namespace System.Security.Cryptography;

[Flags]
public enum CngKeyCreationOptions : int
{
    // existing:
    // None = 0x00000000,
    // MachineKey = 0x00000020,            // NCRYPT_MACHINE_KEY_FLAG
    // OverwriteExistingKey = 0x00000080,  // NCRYPT_OVERWRITE_KEY_FLAG

    // new APIs:
    PreferVbs = 0x00010000,             // NCRYPT_PREFER_VBS_FLAG
    RequireVbs = 0x00020000,            // NCRYPT_REQUIRE_VBS_FLAG
    UsePerBootKey = 0x00040000,         // NCRYPT_USE_PER_BOOT_KEY_FLAG
}

Example usage

// Note: this API is Windows only

using System.Security.Cryptography;

CngKeyCreationParameters cngCreationParams = new()
{
    Provider = CngProvider.MicrosoftSoftwareKeyStorageProvider,
    KeyCreationOptions = CngKeyCreationOptions.RequireVbs | CngKeyCreationOptions.OverwriteExistingKey,
};

using (CngKey key = CngKey.Create(CngAlgorithm.ECDsaP256, "mySoftwareKey", cngCreationParams))
using (ECDsaCng ecdsa = new ECDsaCng(key))
{
    // do stuff with the key
}

@krwq krwq added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Security labels May 21, 2024
@krwq krwq requested a review from bartonjs May 21, 2024 13:02
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

This was referenced May 22, 2024
Open
Open
@krwq krwq removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 28, 2024
This was referenced Jun 7, 2024
Open
Closed
@build-analysis build-analysis bot mentioned this pull request Jun 12, 2024
Open
bartonjs
bartonjs reviewed Jun 17, 2024
View reviewed changes
src/libraries/Common/tests/System/Security/Cryptography/CngPlatformProviderKey.cs
Comment on lines +23 to +26
CngKeyCreationParameters cngCreationParameters,
string keySuffix = null,
[CallerMemberName] string testName = null,
params CngProperty[] additionalParameters)
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this constructor, because if additionalParameters are provided then it modifies cngCreationParameters.

Copy link
Member

Choose a reason for hiding this comment

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

Based on usages later, I expect you're using this class because the dispose deletes the key.

To me, that means this class should just be renamed to SelfDeletingCngKey or CngSelfDeletingKey, or even CngKeyWrapper. If need be for preview 6 deadline reasons, this can be done as a followup. But we shouldn't have a name "PlatformProvider" for a thing that we're extending to work with other providers.

This ctor should be removed, and replaced with a static method for using the software KSP. E.g. public static SelfDeletingCngKey CreateSoftwareKey, which takes whatever options you need. That'll probably require making a private ctor that takes the prebuilt CngKeyCreationParameters; leave the existing one alone.

In an ideal world (but perhaps not a time constrained one), the existing public ctor would also be removed and replaced with a CreatePlatformKey (or some suitable name) with parameters that look like the current ctor.

bartonjs
bartonjs reviewed Jun 17, 2024
View reviewed changes
src/libraries/System.Security.Cryptography/tests/CngKeyTests.cs
Comment on lines +22 to +26
using (CngPlatformProviderKey key = new CngPlatformProviderKey(
CngAlgorithm.ECDsaP256,
new CngKeyCreationParameters()
{
Provider = CngProvider.MicrosoftSoftwareKeyStorageProvider,
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using CngPlatformProviderKey for a software KSP key?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vcsjones vcsjones vcsjones left review comments

@bartonjs bartonjs bartonjs left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@krwq krwq

Labels
area-System.Security new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Windows CNG virtualization-based security
3 participants
@krwq @vcsjones @bartonjs