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

[API Proposal]: RSA PSS salt length #104080

Open
krwq opened this issue Jun 27, 2024 · 8 comments
Open

[API Proposal]: RSA PSS salt length #104080

krwq opened this issue Jun 27, 2024 · 8 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security
Milestone

Comments

@krwq
Copy link
Member

krwq commented Jun 27, 2024

Background and motivation

Some implementations of RSA PSS by default use different salt length for RSA PSS than .NET and it's currently not possible to change it.

.NET currently follows https://datatracker.ietf.org/doc/html/rfc3447#section-9.1 recommendation and uses salt length equal to hash length but spec doesn't forbid other values. On OpenSSL some providers (i.e. tpm2) use different salt length making it impossible to validate such signature (interestingly signature validates on Linux<-->Linux by default uses salt length detection during validation: RSA_PSS_SALTLEN_AUTO)

We should also support typical placeholder values as well when implementing this:

  • sLen=hLen - that would be equivalent to OpenSSL RSA_PSS_SALTLEN_DIGEST
  • RSA_PSS_SALTLEN_MAX - max salt len
  • 0 - zero salt length

and ideally detection of salt length or at least support for sLen=hLen and RSA_PSS_SALTLEN_MAX which seem to be most frequently used.

The detection should be easily implemented because after unmasking encoded message PS length can be detected by counting number of leading zeros which should be followed by 0x01. From that remainder is salt.

API Proposal

Not proposing specific APIs but for example:

partial class RSASignaturePadding
{
    public const int PSSSaltLengthEqualToHashLength = -1;
    public const int PSSSaltLengthMax = -2;
    public RSASignaturePadding(RSASignaturePaddingMode mode, int saltLength = PSSSaltLengthEqualToHashLength) {}
    // or
    public static CreatePSS(int saltLength) {}
}

API Usage

RSASignaturePadding pssWithCustomSalt = RSASignaturePadding.CreatePSS(RSASignaturePadding.PSSSaltLengthMax);
// ..
rsa.VerifyData(data, sig, hashName, pssWithCustomSalt);

Alternative Designs

No response

Risks

No response

@krwq krwq added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security labels Jun 27, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jun 27, 2024
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.

@bartonjs
Copy link
Member

The detection should be easily implemented because after unmasking encoded message PS length can be detected

While we have code for doing PSS, we prefer to let the OS do it. I don't think there's enough demand for this feature that we'd change our policy there, which means, basically "we can't do this".

On OpenSSL some providers (i.e. tpm2) use different salt length

If the provider isn't respecting the salt length we've set, it certainly won't respect some other value. So that doesn't seem like justification for new API.

0 - zero salt length

"Why?" or "No.". Take your pick 😄.

@krwq
Copy link
Member Author

krwq commented Jun 28, 2024

For "why?" it's compat. I don't think this is super blocking in any way given it's tpm2 provider who should be creating signatures as requested but I think on our side it would be really nice to have an opt-in switch for either manual salt length or "accept whatever salt length" (emphasize on opt-in with warning that is theoretically making security worse). Also it would make debugging these sort of issues easier since there is currently no way to make non-standard size salt work (and spec doesn't forbid sLen != hLen - it only recommends to use equal lengths).

@vcsjones
Copy link
Member

Bear in mind that it is the TPM that has opinions about salt lengths. The Platform Crypto Provider appears to reject any RSA signing operation using a PSS salt where $hLen \neq sLen$.

Output

Salt size 32: It worked.
Salt size 31: FAILED 0x40290423
Salt size 30: FAILED 0x40290423
Salt size 33: FAILED 0x40290423
Salt size 34: FAILED 0x40290423

0x40290423 is TPM_E_PCP_UNSUPPORTED_PSS_SALT. At least in the case of Windows for the specified use case, I don't see a new API accomplishing anything since the TPM will reject any value other than the one we are providing.

using System.Security.Cryptography;
using System.Runtime.InteropServices;
using Microsoft.Win32.SafeHandles;
using System;
using System.Text;

#pragma warning disable CA1416 // Thingy I do not care about.

public class Program {
    static unsafe void Main() {
        CngKey? key = null;

        try {
            key = CngKey.Create(CngAlgorithm.Rsa, RandomNumberGenerator.GetHexString(32), new CngKeyCreationParameters {
                Provider = CngProvider.MicrosoftPlatformCryptoProvider
            });

            TryRsaWithSaltSize(key, SHA256.HashSizeInBytes);
            TryRsaWithSaltSize(key, SHA256.HashSizeInBytes - 1);
            TryRsaWithSaltSize(key, SHA256.HashSizeInBytes - 2);
            TryRsaWithSaltSize(key, SHA256.HashSizeInBytes + 1);
            TryRsaWithSaltSize(key, SHA256.HashSizeInBytes + 2);
        }
        finally {
            key.Delete();
        }
    }

    static unsafe void TryRsaWithSaltSize(CngKey key, int saltSize) {
        // SHA256\0 as UTF16-LE
        ReadOnlySpan<byte> alg = [0x53, 0x00, 0x48, 0x00, 0x41, 0x00, 0x32, 0x00, 0x35, 0x00, 0x36, 0x00, 0x00, 0x00];
        byte[] hash = RandomNumberGenerator.GetBytes(SHA256.HashSizeInBytes); // A "hash" to sign.
        byte[] sigBuffer = new byte[1024]; // Buffer to receive a signature. It's oversized. We'll slice it later.

        fixed (byte* pSigBuffer = sigBuffer)
        fixed (byte* pHash = hash)
        fixed (byte* pAlg = alg) {
            BCRYPT_PSS_PADDING_INFO padding = new() {
                pszAlgId = pAlg,
                cbSalt = saltSize
            };

            int ret = InteropStuff.NCryptSignHash(key.Handle, &padding, pHash, SHA256.HashSizeInBytes, pSigBuffer, sigBuffer.Length, out int sigWritten, AsymmetricPaddingMode.NCRYPT_PAD_PSS_FLAG);

            if (ret != 0) {
                Console.WriteLine($"Salt size {saltSize}: FAILED 0x{ret:X08}");
            }
            else {
                Console.WriteLine($"Salt size {saltSize}: It worked.");
            }
        }
    }
}

internal partial class InteropStuff {
    [LibraryImport("ncrypt.dll", StringMarshalling = StringMarshalling.Utf16, SetLastError = true)]
    internal static unsafe partial int NCryptSignHash(SafeNCryptKeyHandle hKey, void* pPaddingInfo, byte* pbHashValue, int cbHashValue, byte* pbSignature, int cbSignature, out int pcbResult, AsymmetricPaddingMode dwFlags);
}

internal enum AsymmetricPaddingMode : int {
    None = 0x00000000,
    NCRYPT_NO_PADDING_FLAG = 0x00000001,
    NCRYPT_PAD_PKCS1_FLAG = 0x00000002,
    NCRYPT_PAD_OAEP_FLAG = 0x00000004,
    NCRYPT_PAD_PSS_FLAG = 0x00000008,
}

[StructLayout(LayoutKind.Sequential)]
internal unsafe struct BCRYPT_PSS_PADDING_INFO {
    internal byte* pszAlgId;
    internal int cbSalt;
}

@krwq
Copy link
Member Author

krwq commented Jul 1, 2024

@vcsjones TPM 2.0 spec (https://trustedcomputinggroup.org/resource/tpm-library-specification/) Part 1 B.7:

image

but I agree per latest TPM 2.0 + FIPS 186-5 combined basically tells in many words that sLen == hLen. I'm not sure if there is any statement that we're not allowed to validate this though (also I'm not in US so FIPS is just foreign country recommendation to me not a strict enforcement).

@vcsjones
Copy link
Member

vcsjones commented Jul 1, 2024

My point is more than TPMs do not have a provision to let clients pick a salt size. The TPM is going to use the salt size that it is configured to use.

These are the Table 169 RSA schemes that only need a hash algorithm as a scheme parameter.
For the TPM_ALG_RSAPSS signing scheme, the same hash algorithm is used for digesting TPM-generated data (an attestation structure) and in the KDF used for the masking operation. The salt size is
always the largest salt value that will fit into the available space.

Table 169 — Definition of {RSA} Types for RSA Signature Schemes

Type Name Description
TPMS_SCHEME_HASH TPMS_SIG_SCHEME_!ALG.AX

Where TPMS_SCHEME_HASH has one parameter, hashAlg, of type TPMI_ALG_HASH.

My interpretation of the TPM spec indicates to me that the most control you have over a PSS signature is the hash algorithm. The salt size is dictated by the specification. What it is dictated to depends on how the TPM adheres to FIPS, some have a "FIPS mode" (FIPS_140_2 attribute) that may alter behavior, but the signing operation itself does not pick the salt length.

So, even if we added some new API for configuring the salt size, the TPM would probably have to error, or ignore, every value other than the one that the TPM uses.

@krwq
Copy link
Member Author

krwq commented Jul 2, 2024

Note that my main focus of this API is validation of signatures produced by TPM - not signing. I'm already convinced signing is not important for other lengths given latest specs only allow single salt size. I'd really love to say that the problem is only with some old TPM modules but according to tpm2-software/tpm2-openssl#115 (comment) it seems that currently TPMs mostly use different salt lengths than sLen==hLen - this will probably change in the following years but I expect in couple of years that will still be relatively large number.

Perhaps it would be possible to at least add some opt-in APIs for relaxing this a bit for validation only (and enforce sLen==hLen on signing). I.e. setting could be called according to FIPS version so at least people know what they're following. And honestly I can only see supporting 4 settings for validation:

  • any salt length
  • max salt length or hLen (those are the only two I encountered so far)
  • max salt length only
  • hLen only
  • (possibly also 0 length because RFC 3447 mentions that but I don't have evidence anyone actually uses that in practice)

so possibly it could be just a list of allowed lengths defaulting to just hLen with couple of predefined settings which match different versions of FIPS so that people know which one is the latest.

@bartonjs bartonjs added this to the Future milestone Jul 12, 2024
@jeffhandley jeffhandley removed the untriaged New issue has not been triaged by the area owner label Jul 20, 2024
@amin1best
Copy link

@vcsjones , @krwq

In this environment:

Windows 10 22H2 Build 19045.4651
TPM2 INTC Version 403.1.0.0

Output of #104080 (comment)

Salt size 32: It worked.
Salt size 31: It worked.
Salt size 30: It worked.
Salt size 33: It worked.
Salt size 34: It worked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security
Projects
None yet
Development

No branches or pull requests

5 participants