Skip to content

Commit cf0dd8a

Browse files
authored
Improve Code Quality 7 (#279)
* Eliminate HashAlgorithm allocations * Make CryptoUtils internal (to allow for future optimizations) * Replace CompareTo with Equals * Seal Packed AttestationVerifier * Fix member visibility
1 parent 721e667 commit cf0dd8a

File tree

6 files changed

+246
-483
lines changed

6 files changed

+246
-483
lines changed

Src/Fido2/AttestationFormat/Packed.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
namespace Fido2NetLib
99
{
10-
internal class Packed : AttestationVerifier
10+
internal sealed class Packed : AttestationVerifier
1111
{
1212
private static readonly string[] s_newLine = new string[] { Environment.NewLine };
1313

Src/Fido2/AttestationFormat/Tpm.cs

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -111,23 +111,21 @@ public override (AttestationType, X509Certificate2[]) Verify()
111111
// Handled in CertInfo constructor, see CertInfo.Type
112112

113113
// 4c. Verify that extraData is set to the hash of attToBeSigned using the hash algorithm employed in "alg"
114-
if (!(Alg is CborInteger))
114+
if (Alg is not CborInteger)
115115
throw new Fido2VerificationException("Invalid TPM attestation algorithm");
116116

117117
var alg = (COSE.Algorithm)(int)Alg;
118118

119-
using (HashAlgorithm hasher = CryptoUtils.GetHasher(CryptoUtils.HashAlgFromCOSEAlg(alg)))
120-
{
121-
if (!hasher.ComputeHash(Data).AsSpan().SequenceEqual(certInfo.ExtraData))
122-
throw new Fido2VerificationException("Hash value mismatch extraData and attToBeSigned");
123-
}
124-
119+
ReadOnlySpan<byte> dataHash = CryptoUtils.HashData(CryptoUtils.HashAlgFromCOSEAlg(alg), Data);
120+
121+
if (!dataHash.SequenceEqual(certInfo.ExtraData))
122+
throw new Fido2VerificationException("Hash value mismatch extraData and attToBeSigned");
123+
125124
// 4d. Verify that attested contains a TPMS_CERTIFY_INFO structure, whose name field contains a valid Name for pubArea, as computed using the algorithm in the nameAlg field of pubArea
126-
using (HashAlgorithm hasher = CryptoUtils.GetHasher(CryptoUtils.HashAlgFromCOSEAlg((COSE.Algorithm)certInfo.Alg)))
127-
{
128-
if (!hasher.ComputeHash(pubArea.Raw).AsSpan().SequenceEqual(certInfo.AttestedName))
129-
throw new Fido2VerificationException("Hash value mismatch attested and pubArea");
130-
}
125+
ReadOnlySpan<byte> pubAreaRawHash = CryptoUtils.HashData(CryptoUtils.HashAlgFromCOSEAlg((COSE.Algorithm)certInfo.Alg), pubArea.Raw);
126+
127+
if (!pubAreaRawHash.SequenceEqual(certInfo.AttestedName))
128+
throw new Fido2VerificationException("Hash value mismatch attested and pubArea");
131129

132130
// 4e. Note that the remaining fields in the "Standard Attestation Structure" [TPMv2-Part1] section 31.2, i.e., qualifiedSigner, clockInfo and firmwareVersion are ignored. These fields MAY be used as an input to risk engines.
133131

Src/Fido2/CryptoUtils.cs

Lines changed: 13 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,35 +7,18 @@
77

88
namespace Fido2NetLib
99
{
10-
public static class CryptoUtils
10+
internal static class CryptoUtils
1111
{
12-
public static HashAlgorithm GetHasher(HashAlgorithmName hashName)
12+
public static byte[] HashData(HashAlgorithmName hashName, ReadOnlySpan<byte> data)
1313
{
14-
switch (hashName.Name)
14+
return hashName.Name switch
1515
{
16-
case "SHA1":
17-
return SHA1.Create();
18-
case "SHA256":
19-
case "HS256":
20-
case "RS256":
21-
case "ES256":
22-
case "PS256":
23-
return SHA256.Create();
24-
case "SHA384":
25-
case "HS384":
26-
case "RS384":
27-
case "ES384":
28-
case "PS384":
29-
return SHA384.Create();
30-
case "SHA512":
31-
case "HS512":
32-
case "RS512":
33-
case "ES512":
34-
case "PS512":
35-
return SHA512.Create();
36-
default:
37-
throw new ArgumentOutOfRangeException(nameof(hashName));
38-
}
16+
"SHA1" => SHA1.HashData(data),
17+
"SHA256" or "HS256" or "RS256" or "ES256" or "PS256" => SHA256.HashData(data),
18+
"SHA384" or "HS384" or "RS384" or "ES384" or "PS384" => SHA384.HashData(data),
19+
"SHA512" or "HS512" or "RS512" or "ES512" or "PS512" => SHA512.HashData(data),
20+
_ => throw new ArgumentOutOfRangeException(nameof(hashName)),
21+
};
3922
}
4023

4124
public static HashAlgorithmName HashAlgFromCOSEAlg(COSE.Algorithm alg)
@@ -72,12 +55,14 @@ public static bool ValidateTrustChain(X509Certificate2[] trustPath, X509Certific
7255
// A trust anchor can be a root certificate, an intermediate CA certificate or even the attestation certificate itself.
7356

7457
// Let's check the simplest case first. If subject and issuer are the same, and the attestation cert is in the list, that's all the validation we need
75-
if (trustPath.Length == 1 && trustPath[0].Subject.CompareTo(trustPath[0].Issuer) == 0)
58+
if (trustPath.Length == 1 && trustPath[0].Subject.Equals(trustPath[0].Issuer, StringComparison.Ordinal))
7659
{
7760
foreach (X509Certificate2? cert in attestationRootCertificates)
7861
{
79-
if (cert.Thumbprint.CompareTo(trustPath[0].Thumbprint) == 0)
62+
if (cert.Thumbprint.Equals(trustPath[0].Thumbprint, StringComparison.Ordinal))
63+
{
8064
return true;
65+
}
8166
}
8267
return false;
8368
}

Src/Fido2/Metadata/ConformanceMetadataRepository.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,12 +98,12 @@ public async Task<MetadataBLOBPayload> GetBLOBAsync()
9898
return combinedBlob;
9999
}
100100

101-
protected Task<string> DownloadStringAsync(string url)
101+
private Task<string> DownloadStringAsync(string url)
102102
{
103103
return _httpClient.GetStringAsync(url);
104104
}
105105

106-
protected Task<byte[]> DownloadDataAsync(string url)
106+
private Task<byte[]> DownloadDataAsync(string url)
107107
{
108108
return _httpClient.GetByteArrayAsync(url);
109109
}

0 commit comments

Comments
 (0)