[API Proposal]: X509KeysStorageFlags.IgnorePrivateKeys

[88737858]

Background and motivation

There has been a history of security issues caused by API developers carelessly using the X509Certificate2 class to parse inputs. The issue stems from the fact that an X509Certificate2 class might literally be an X509 cert, but it might also be something else like PKCS12 that contains private keys, even though it is nominally an "X509 Certificate".

As a result of this design, developers are prone to accidentally ingest private keys for inputs where a caller is expected to only pass a public key, e.g. the caller might unexpectedly upload a PFX file into an API that was expecting a CER file.

To enable API developers to more easily defend against this accidental ingestion of private keys, the proposal is to add a new flag to the Storage Flags enum that indicates to ignore any private keys that may be present. Developers can then use this enum to explicitly indicate their intention to process or not process private keys.

API Proposal

public enum X509KeyStorageFlags
{
    DefaultKeySet = 0,
    UserKeySet = 1,
    MachineKeySet = 2,
    Exportable = 4,
    UserProtected = 8,
    PersistKeySet = 16,
    EphemeralKeySet = 32,
    IgnorePrivateKeys = 64  // <--- New flag value
}

API Usage

Specifying IgnorePrivateKeys flag in constructor strips any private keys that might be present in the data used to construct the class

    // Parse a stream of cryptographic data while ignoring any private key data that may be present
    byte[] bytes;
    X509Certificate2 cert = X509Certificate2(bytes, X509KeyStorageFlags.IgnorePrivateKeys);
    
    // Check that there are indeed never any private keys
    Debug.Assert(cert.HasPrivateKeys == false);

Within MS Security division, we would also use this capability along with static analysis tools to require developers to always declare their intention to process private keys.

    byte[] bytes;

    // Code that does not specify a positive or negative value for IgnorePrivateKeys would fail static analysis checks (e.g. CodeQL)
    X509Certificate2 cert1 = X509Certificate2(bytes);  // Code violation, must specify either IgnorePrivateKeys or ~IgnorePrivateKeys

    // Parse a stream of cryptographic data while ignoring any private key data that may be present
    X509Certificate2 cert2 = X509Certificate2(bytes, X509KeyStorageFlags.IgnorePrivateKeys);
    
    // Parse a stream of cryptographic data and explicitly allow private key data to be ingested
    X509Certificate2 cert3 = X509Certificate2(bytes, X509KeyStorageFlags.DefaultKeySet & ~X509KeyStorageFlags.IgnorePrivateKeys;

Alternative Designs

An alternative design might be to provide a factory method that explicitly creates X509certificate2 objects with a given content type.

public static X509Certificate2 CreateForContentType(byte[] byte, X509ContentType keyContentType)
{
}

An advantage to this design would be that it could fail to construct an object of the given type. This would be preferable as it would more naturally lead to an error propagating back to the caller.

If using this approach, it would probably be best to allow an array of X509ContentType, since it's not a flags enum.

Risks

There are no obvious risks with the addition of a flag for suppressing private keys.

[ghost]

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

Issue Details

---

Background and motivation

There has been a history of security issues caused by API developers carelessly using the X509Certificate2 class to parse inputs. The issue stems from the fact that an X509Certificate2 class might literally be an X509 cert, but it might also be something else like PKCS12 that contains private keys, even though it is nominally an "X509 Certificate".

As a result of this design, developers are prone to accidentally ingest private keys for inputs where a caller is expected to only pass a public key, e.g. the caller might unexpectedly upload a PFX file into an API that was expecting a CER file.

To enable API developers to more easily defend against this accidental ingestion of private keys, the proposal is to add a new flag to the Storage Flags enum that indicates to ignore any private keys that may be present. Developers can then use this enum to explicitly indicate their intention to process or not process private keys.

API Proposal

public enum X509KeyStorageFlags
{
    DefaultKeySet = 0,
    UserKeySet = 1,
    MachineKeySet = 2,
    Exportable = 4,
    UserProtected = 8,
    PersistKeySet = 16,
    EphemeralKeySet = 32,
    IgnorePrivateKeys = 64  // <--- New flag value
}

API Usage

Specifying IgnorePrivateKeys flag in constructor strips any private keys that might be present in the data used to construct the class

    // Parse a stream of cryptographic data while ignoring any private key data that may be present
    byte[] bytes;
    X509Certificate2 cert = X509Certificate2(bytes, X509KeyStorageFlags.IgnorePrivateKeys);
    
    // Check that there are indeed never any private keys
    Debug.Assert(cert.HasPrivateKeys == false);

Within MS Security division, we would also use this capability along with static analysis tools to require developers to always declare their intention to process private keys.

    byte[] bytes;

    // Code that does not specify a positive or negative value for IgnorePrivateKeys would fail static analysis checks (e.g. CodeQL)
    X509Certificate2 cert1 = X509Certificate2(bytes);  // Code violation, must specify either IgnorePrivateKeys or ~IgnorePrivateKeys

    // Parse a stream of cryptographic data while ignoring any private key data that may be present
    byte[] bytes;
    X509Certificate2 cert2 = X509Certificate2(bytes, X509KeyStorageFlags.IgnorePrivateKeys);
    
    // Parse a stream of cryptographic data and explicitly allow private key data to be ingested
    byte[] bytes;
    X509Certificate2 cert3 = X509Certificate2(bytes, X509KeyStorageFlags.DefaultKeySet & ~X509KeyStorageFlags.IgnorePrivateKeys;

Alternative Designs

An alternative design might be to provide a factory method that explicitly creates X509certificate2 objects with a given content type.

public static X509Certificate2 CreateForContentType(byte[] byte, X509ContentType keyContentType)
{
}

An advantage to this design would be that it could fail to construct an object of the given type. This would be preferable as it would more naturally lead to an error propagating back to the caller.

If using this approach, it would probably be best to allow an array of X509ContentType, since it's not a flags enum.

Risks

There are no obvious risks with the addition of a flag for suppressing private keys.

Author:	88737858
Assignees:	-
Labels:	api-suggestion, area-System.Security, untriaged
Milestone:	-

[jeffhandley]

@88737858 Thanks for submitting this; it's an area of active investigation for us. We're considering several alternatives here. One option could be to expose flags as you had stated here. We're also looking into whether a more configurable system is desirable so that it can be used by a wider variety of callers. We're not acting on this suggestion just yet since we want to take additional time to investigate all the options.

While I'm moving the issue to Future, our discussions about approaches for this will continue.

[vcsjones]

To follow on @jeffhandley's comment, the alternative proposal is #91763. This is a larger proposal, but one that I believe encompasses this one. In particular, Pkcs12LoaderLimits.IgnorePrivateKeys will allow the loader to ignore private keys in a PKCS12.

[jeffhandley]

I agree that #91763 would cover this proposal. I'm closing this one since it's superceded. Thanks again for submitting this proposal, @88737858.