Skip to content

Commit 45ca8af

Browse files
iamcarboniamcarbon
authored
Improve Code Quality 5 (#259)
* [Tests] Minor formats * Eliminate unnecessary X509Certificate2 allocations * Move properties below constructor * Presize CborArray when size is known * Format CredentialPublicKey * Use init properties in AuthenticatorAssertionResponse * Format AttestationFormat * Use DateTimeOffset.UnixEpoch and differentiate between missing and invalid timestampMs * Remove unused using statement * Remove leftover test logic
1 parent 35cf94b commit 45ca8af

22 files changed

+355
-345
lines changed

‎Src/Fido2/AttestationFormat/AndroidKey.cs‎

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -151,25 +151,37 @@ public override (AttestationType, X509Certificate2[]) Verify()
151151

152152
// 2. Verify that sig is a valid signature over the concatenation of authenticatorData and clientDataHash
153153
// using the attestation public key in attestnCert with the algorithm specified in alg
154-
if (!(X5c is CborArray { Length: > 0 } x5cArray && x5cArray[0] is CborByteString { Length: > 0 }))
154+
if (!(X5c is CborArray { Length: > 0 } x5cArray))
155155
throw new Fido2VerificationException("Malformed x5c in android-key attestation");
156156

157+
if (!(Alg is CborInteger))
158+
throw new Fido2VerificationException("Invalid android key attestation algorithm");
157159

158-
X509Certificate2 androidKeyCert;
159-
ECDsa androidKeyPubKey;
160-
try
161-
{
162-
androidKeyCert = new X509Certificate2((byte[])x5cArray[0]);
163-
androidKeyPubKey = androidKeyCert.GetECDsaPublicKey()!; // attestation public key
164-
}
165-
catch (Exception ex)
160+
var alg = (COSE.Algorithm)(int)Alg;
161+
var trustPath = new X509Certificate2[x5cArray.Length];
162+
163+
for (int i = 0; i < x5cArray.Length; i++)
166164
{
167-
throw new Fido2VerificationException("Failed to extract public key from android key: " + ex.Message, ex);
165+
if (x5cArray[i] is CborByteString { Length: > 0 } x5cObject)
166+
{
167+
try
168+
{
169+
trustPath[i] = new X509Certificate2(x5cObject.Value);
170+
}
171+
catch (Exception ex) when (i is 0)
172+
{
173+
throw new Fido2VerificationException("Failed to extract public key from android key: " + ex.Message, ex);
174+
}
175+
}
176+
else
177+
{
178+
throw new Fido2VerificationException("Malformed x5c in android-key attestation");
179+
}
168180
}
169181

170-
if (!(Alg is CborInteger))
171-
throw new Fido2VerificationException("Invalid android key attestation algorithm");
172-
182+
X509Certificate2 androidKeyCert = trustPath[0];
183+
ECDsa androidKeyPubKey = androidKeyCert.GetECDsaPublicKey()!; // attestation public key
184+
173185
byte[] ecsig;
174186
try
175187
{
@@ -180,7 +192,7 @@ public override (AttestationType, X509Certificate2[]) Verify()
180192
throw new Fido2VerificationException("Failed to decode android key attestation signature from ASN.1 encoded form", ex);
181193
}
182194

183-
if (!androidKeyPubKey.VerifyData(Data, ecsig, CryptoUtils.HashAlgFromCOSEAlg((COSE.Algorithm)(int)Alg)))
195+
if (!androidKeyPubKey.VerifyData(Data, ecsig, CryptoUtils.HashAlgFromCOSEAlg(alg)))
184196
throw new Fido2VerificationException("Invalid android key attestation signature");
185197

186198
// 3. Verify that the public key in the first certificate in x5c matches the credentialPublicKey in the attestedCredentialData in authenticatorData.
@@ -217,10 +229,6 @@ public override (AttestationType, X509Certificate2[]) Verify()
217229
if (!IsPurposeSign(attExtBytes))
218230
throw new Fido2VerificationException("Found purpose field not set to KM_PURPOSE_SIGN in android key attestation certificate extension");
219231

220-
var trustPath = x5cArray.Values
221-
.Select(x => new X509Certificate2((byte[])x))
222-
.ToArray();
223-
224232
return (AttestationType.Basic, trustPath);
225233
}
226234
}

‎Src/Fido2/AttestationFormat/AndroidSafetyNet.cs‎

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -42,12 +42,12 @@ public override (AttestationType, X509Certificate2[]) Verify()
4242
}
4343

4444
// 2. Verify that response is a valid SafetyNet response of version ver
45-
var ver = (string)attStmt["ver"];
45+
var ver = (string)attStmt["ver"]!;
4646

4747
if (!(attStmt["response"] is CborByteString { Length: > 0}))
4848
throw new Fido2VerificationException("Invalid response in SafetyNet data");
4949

50-
var response = (byte[])attStmt["response"];
50+
var response = (byte[])attStmt["response"]!;
5151
var responseJWT = Encoding.UTF8.GetString(response);
5252

5353
if (string.IsNullOrWhiteSpace(responseJWT))
@@ -71,12 +71,11 @@ public override (AttestationType, X509Certificate2[]) Verify()
7171
throw new Fido2VerificationException("No keys were present in the TOC header in SafetyNet response JWT");
7272

7373
var certs = new X509Certificate2[x5cStrings.Length];
74-
var keys = new List<SecurityKey>();
74+
var keys = new List<SecurityKey>(certs.Length);
7575

7676
for (int i = 0; i < certs.Length; i++)
7777
{
78-
var certString = x5cStrings[i];
79-
var cert = GetX509Certificate(certString);
78+
var cert = GetX509Certificate(x5cStrings[i]);
8079
certs[i] = cert;
8180

8281
if (cert.GetECDsaPublicKey() is ECDsa ecdsaPublicKey)
@@ -109,9 +108,9 @@ public override (AttestationType, X509Certificate2[]) Verify()
109108
throw new Fido2VerificationException("SafetyNet response security token validation failed", ex);
110109
}
111110

112-
var nonce = "";
111+
string? nonce = null;
113112
bool? ctsProfileMatch = null;
114-
var timestampMs = DateTimeHelper.UnixEpoch;
113+
DateTimeOffset? timestamp = null;
115114

116115
var jwtToken = (JwtSecurityToken)validatedToken;
117116

@@ -127,19 +126,24 @@ public override (AttestationType, X509Certificate2[]) Verify()
127126
}
128127
if (claim is { Type: "timestampMs", ValueType: "http://www.w3.org/2001/XMLSchema#integer64" })
129128
{
130-
timestampMs = DateTimeHelper.UnixEpoch.AddMilliseconds(double.Parse(claim.Value));
129+
timestamp = DateTimeOffset.UnixEpoch.AddMilliseconds(double.Parse(claim.Value));
131130
}
132131
}
133132

134-
var notAfter = DateTime.UtcNow.AddMilliseconds(_driftTolerance);
135-
var notBefore = DateTime.UtcNow.AddMinutes(-1).AddMilliseconds(-(_driftTolerance));
136-
if ((notAfter < timestampMs) || ((notBefore) > timestampMs))
133+
if (!timestamp.HasValue)
137134
{
138-
throw new Fido2VerificationException($"SafetyNet timestampMs must be present and between one minute ago and now, got: {timestampMs}");
135+
throw new Fido2VerificationException($"SafetyNet timestampMs not found SafetyNet attestation");
136+
}
137+
138+
var notAfter = DateTimeOffset.UtcNow.AddMilliseconds(_driftTolerance);
139+
var notBefore = DateTimeOffset.UtcNow.AddMinutes(-1).AddMilliseconds(-(_driftTolerance));
140+
if ((notAfter < timestamp) || ((notBefore) > timestamp.Value))
141+
{
142+
throw new Fido2VerificationException($"SafetyNet timestampMs must be between one minute ago and now, got: {timestamp:o}");
139143
}
140144

141145
// 3. Verify that the nonce in the response is identical to the SHA-256 hash of the concatenation of authenticatorData and clientDataHash
142-
if (nonce is "")
146+
if (string.IsNullOrEmpty(nonce))
143147
throw new Fido2VerificationException("Nonce value not found in SafetyNet attestation");
144148

145149
byte[] nonceHash;

‎Src/Fido2/AttestationFormat/Apple.cs‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,12 @@ public override (AttestationType, X509Certificate2[]) Verify()
5050
// 2. Verify x5c is a valid certificate chain starting from the credCert to the Apple WebAuthn root certificate.
5151
// This happens in AuthenticatorAttestationResponse.VerifyAsync using metadata from MDS3
5252

53-
var trustPath = x5cArray.Values
54-
.Select(x => new X509Certificate2((byte[])x))
55-
.ToArray();
53+
var trustPath = new X509Certificate2[x5cArray.Length];
54+
55+
for (int i = 0; i < trustPath.Length; i++)
56+
{
57+
trustPath[i] = new X509Certificate2((byte[])x5cArray[i]);
58+
}
5659

5760
// credCert is the first certificate in the trust path
5861
var credCert = trustPath[0];
@@ -72,7 +75,7 @@ public override (AttestationType, X509Certificate2[]) Verify()
7275

7376
// 6. Verify credential public key matches the Subject Public Key of credCert.
7477
// First, obtain COSE algorithm being used from credential public key
75-
var coseAlg = (int)CredentialPublicKey[COSE.KeyCommonParameter.Alg];
78+
var coseAlg = (COSE.Algorithm)(int)CredentialPublicKey[COSE.KeyCommonParameter.Alg];
7679

7780
// Next, build temporary CredentialPublicKey for comparison from credCert and COSE algorithm
7881
var cpk = new CredentialPublicKey(credCert, coseAlg);

‎Src/Fido2/AttestationFormat/AttestationFormat.cs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ internal static byte[] AaguidFromAttnCertExts(X509ExtensionCollection exts)
2626
{
2727
byte[] aaguid = null;
2828
var ext = exts.Cast<X509Extension>().FirstOrDefault(e => e.Oid.Value is "1.3.6.1.4.1.45724.1.1.4"); // id-fido-gen-ce-aaguid
29-
if (null != ext)
29+
if (ext != null)
3030
{
3131
var decodedAaguid = Asn1Element.Decode(ext.RawData);
3232
decodedAaguid.CheckTag(Asn1Tag.PrimitiveOctetString);
@@ -39,6 +39,7 @@ internal static byte[] AaguidFromAttnCertExts(X509ExtensionCollection exts)
3939

4040
return aaguid;
4141
}
42+
4243
internal static bool IsAttnCertCACert(X509ExtensionCollection exts)
4344
{
4445
var ext = exts.Cast<X509Extension>().FirstOrDefault(e => e.Oid.Value is "2.5.29.19");
@@ -49,6 +50,7 @@ internal static bool IsAttnCertCACert(X509ExtensionCollection exts)
4950

5051
return true;
5152
}
53+
5254
internal static byte U2FTransportsFromAttnCert(X509ExtensionCollection exts)
5355
{
5456
var u2ftransports = new byte();
@@ -73,6 +75,7 @@ internal static byte U2FTransportsFromAttnCert(X509ExtensionCollection exts)
7375

7476
return u2ftransports;
7577
}
78+
7679
public virtual (AttestationType, X509Certificate2[]) Verify(CborMap attStmt, byte[] authenticatorData, byte[] clientDataHash)
7780
{
7881
this.attStmt = attStmt;

‎Src/Fido2/AttestationFormat/FidoU2f.cs‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
using System;
2-
using System.Linq;
32
using System.Runtime.InteropServices;
43
using System.Security.Cryptography;
54
using System.Security.Cryptography.X509Certificates;
@@ -90,9 +89,8 @@ public override (AttestationType, X509Certificate2[]) Verify()
9089
throw new Fido2VerificationException("Invalid fido-u2f attestation signature");
9190

9291
// 7. Optionally, inspect x5c and consult externally provided knowledge to determine whether attStmt conveys a Basic or AttCA attestation
93-
var trustPath = ((CborArray)X5c).Values
94-
.Select(x => new X509Certificate2((byte[])x))
95-
.ToArray();
92+
93+
var trustPath = new X509Certificate2[1] { attCert };
9694

9795
return (AttestationType.AttCa, trustPath);
9896
}

‎Src/Fido2/AttestationFormat/Packed.cs‎

Lines changed: 28 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,16 @@ namespace Fido2NetLib
99
{
1010
internal class Packed : AttestationVerifier
1111
{
12+
private static readonly string[] s_newLine = new string[] { Environment.NewLine };
13+
1214
public static bool IsValidPackedAttnCertSubject(string attnCertSubj)
1315
{
1416
// parse the DN string using standard rules
1517
var dictSubjectObj = new X500DistinguishedName(attnCertSubj);
1618

1719
// form the string for splitting using new lines to avoid issues with commas
1820
var dictSubjectString = dictSubjectObj.Decode(X500DistinguishedNameFlags.UseNewLines);
19-
var dictSubject = dictSubjectString.Split(new string[] { Environment.NewLine }, StringSplitOptions.None)
21+
var dictSubject = dictSubjectString.Split(s_newLine, StringSplitOptions.None)
2022
.Select(part => part.Split('='))
2123
.ToDictionary(split => split[0], split => split[1]);
2224

@@ -37,35 +39,44 @@ public override (AttestationType, X509Certificate2[]?) Verify()
3739
if (!(Sig is CborByteString { Length: > 0 }))
3840
throw new Fido2VerificationException("Invalid packed attestation signature");
3941

40-
if (Alg is null || Alg is not CborInteger)
42+
if (!(Alg is CborInteger))
4143
throw new Fido2VerificationException("Invalid packed attestation algorithm");
4244

45+
var alg = (COSE.Algorithm)(int)Alg;
46+
4347
// 2. If x5c is present, this indicates that the attestation type is not ECDAA
4448
if (X5c != null)
4549
{
4650
if (!(X5c is CborArray { Length: > 0 } x5cArray) || EcdaaKeyId != null)
4751
throw new Fido2VerificationException("Malformed x5c array in packed attestation statement");
4852

49-
var enumerator = x5cArray.Values.GetEnumerator();
50-
while (enumerator.MoveNext())
53+
var trustPath = new X509Certificate2[x5cArray.Length];
54+
55+
for (int i = 0; i < trustPath.Length; i++)
5156
{
52-
if (!(enumerator.Current is CborByteString { Length: > 0 }))
57+
if (X5c[i] is CborByteString { Length: > 0 } x5cObject)
58+
{
59+
var x5cCert = new X509Certificate2(x5cObject.Value);
60+
61+
// X509Certificate2.NotBefore/.NotAfter return LOCAL DateTimes, so
62+
// it's correct to compare using DateTime.Now.
63+
if (DateTime.Now < x5cCert.NotBefore || DateTime.Now > x5cCert.NotAfter)
64+
throw new Fido2VerificationException("Packed signing certificate expired or not yet valid");
65+
66+
trustPath[i] = x5cCert;
67+
}
68+
else
69+
{
5370
throw new Fido2VerificationException("Malformed x5c cert found in packed attestation statement");
54-
55-
var x5ccert = new X509Certificate2((byte[])enumerator.Current);
56-
57-
// X509Certificate2.NotBefore/.NotAfter return LOCAL DateTimes, so
58-
// it's correct to compare using DateTime.Now.
59-
if (DateTime.Now < x5ccert.NotBefore || DateTime.Now > x5ccert.NotAfter)
60-
throw new Fido2VerificationException("Packed signing certificate expired or not yet valid");
71+
}
6172
}
6273

6374
// The attestation certificate attestnCert MUST be the first element in the array.
64-
var attestnCert = new X509Certificate2((byte[])x5cArray[0]);
75+
X509Certificate2 attestnCert = trustPath[0];
6576

6677
// 2a. Verify that sig is a valid signature over the concatenation of authenticatorData and clientDataHash
6778
// using the attestation public key in attestnCert with the algorithm specified in alg
68-
var cpk = new CredentialPublicKey(attestnCert, (int)Alg);
79+
var cpk = new CredentialPublicKey(attestnCert, alg);
6980

7081
if (!cpk.Verify(Data, (byte[])Sig))
7182
throw new Fido2VerificationException("Invalid full packed signature");
@@ -92,18 +103,15 @@ public override (AttestationType, X509Certificate2[]?) Verify()
92103
// 2c. If attestnCert contains an extension with OID 1.3.6.1.4.1.45724.1.1.4 (id-fido-gen-ce-aaguid) verify that the value of this extension matches the aaguid in authenticatorData
93104
if (aaguid != null)
94105
{
95-
if (0 != AttestedCredentialData.FromBigEndian(aaguid).CompareTo(AuthData.AttestedCredentialData.AaGuid))
106+
if (AttestedCredentialData.FromBigEndian(aaguid).CompareTo(AuthData.AttestedCredentialData.AaGuid) != 0)
96107
throw new Fido2VerificationException("aaguid present in packed attestation cert exts but does not match aaguid from authData");
97108
}
98109

99110
// id-fido-u2f-ce-transports
100111
byte u2ftransports = U2FTransportsFromAttnCert(attestnCert.Extensions);
101112

102113
// 2d. Optionally, inspect x5c and consult externally provided knowledge to determine whether attStmt conveys a Basic or AttCA attestation
103-
var trustPath = x5cArray.Values
104-
.Select(x => new X509Certificate2((byte[])x))
105-
.ToArray();
106-
114+
107115
return (AttestationType.AttCa, trustPath);
108116
}
109117

@@ -124,7 +132,7 @@ public override (AttestationType, X509Certificate2[]?) Verify()
124132
else
125133
{
126134
// 4a. Validate that alg matches the algorithm of the credentialPublicKey in authenticatorData
127-
if (!AuthData.AttestedCredentialData.CredentialPublicKey.IsSameAlg((COSE.Algorithm)(int)Alg))
135+
if (!AuthData.AttestedCredentialData.CredentialPublicKey.IsSameAlg(alg))
128136
throw new Fido2VerificationException("Algorithm mismatch between credential public key and authenticator data in self attestation statement");
129137

130138
// 4b. Verify that sig is a valid signature over the concatenation of authenticatorData and

0 commit comments

Comments
 (0)