[WIP] Improve Code Quality (#422)

* Add implicit casting to CborByteString and CborTextString * Use constant size for stack allocation * Eliminate a few nullability suppressions * Remove workaround old .NET framework behavior .NET core populates Oid on named curves for Windows and Linux * Improve CredentialPublicKey test coverage * Skip new secP256k1 test on macOS * Prefer OperatingSystem to IsOSPlatform * Breakout CreateCertInfo helper from Fido2Tests * Prefer Assert.Equal to check for sequence equality * Breakout PubAreaHelper and improve variable names * Improve SafetyNet error messages * Skip test on macOS * Fix test regression * Breakout CreateCertInfo from Tpm * Breakout CreatePubArea from Tpm * Format Tpm tests * Revert const stackalloc change * Dispose MemoryStreams
passwordless-lib · Aug 23, 2023 · 2b0fe71 · 2b0fe71
1 parent 86b6c6d
commit 2b0fe71
Show file tree

Hide file tree

Showing 27 changed files with 705 additions and 919 deletions.
diff --git a/Src/Fido2.Models/COSETypes.cs b/Src/Fido2.Models/COSETypes.cs
@@ -186,7 +186,7 @@ public enum EllipticCurve
         /// </summary>
         Ed448 = 7,
         /// <summary> 
-        /// secp256k1 (pending IANA - requested assignment 8)
+        /// secp256k1
         /// </summary>
         P256K = 8
     }

diff --git a/Src/Fido2/AttestationFormat/AndroidKey.cs b/Src/Fido2/AttestationFormat/AndroidKey.cs
@@ -15,11 +15,12 @@ internal sealed class AndroidKey : AttestationVerifier
     {
         foreach (var ext in exts)
         {
-            if (ext.Oid!.Value is "1.3.6.1.4.1.11129.2.1.17") // AttestationRecordOid
+            if (ext.Oid?.Value is "1.3.6.1.4.1.11129.2.1.17") // AttestationRecordOid
             {
                 return ext.RawData;
             }
         }
+
         return null;
     }
 

diff --git a/Src/Fido2/AttestationFormat/AndroidSafetyNet.cs b/Src/Fido2/AttestationFormat/AndroidSafetyNet.cs
@@ -29,26 +29,31 @@ public override (AttestationType, X509Certificate2[]) Verify(VerifyAttestationRe
         // 2. Verify that response is a valid SafetyNet response of version ver
         if (!request.TryGetVer(out string? ver))
         {
-            throw new Fido2VerificationException("Invalid version in SafetyNet data");
+            throw new Fido2VerificationException(Fido2ErrorMessages.InvalidSafetyNetVersion);
         }
 
-        if (!(request.AttStmt["response"] is CborByteString { Length: > 0 }))
-            throw new Fido2VerificationException("Invalid response in SafetyNet data");
+        if (!(request.AttStmt["response"] is CborByteString { Length: > 0 } responseByteString))
+            throw new Fido2VerificationException(Fido2ErrorMessages.InvalidSafetyNetResponse);
 
-        var response = (byte[])request.AttStmt["response"]!;
-        var responseJWT = Encoding.UTF8.GetString(response);
+        var responseJwt = Encoding.UTF8.GetString(responseByteString);
 
-        if (string.IsNullOrWhiteSpace(responseJWT))
-            throw new Fido2VerificationException("SafetyNet response null or whitespace");
+        var jwtComponents = responseJwt.Split('.');
 
-        var jwtParts = responseJWT.Split('.');
+        if (jwtComponents.Length != 3)
+            throw new Fido2VerificationException(Fido2ErrorMessages.MalformedSafetyNetJwt);
 
-        if (jwtParts.Length != 3)
-            throw new Fido2VerificationException("SafetyNet response JWT does not have the 3 expected components");
+        byte[] jwtHeaderBytes;
 
-        string jwtHeaderString = jwtParts[0];
+        try
+        {
+            jwtHeaderBytes = Base64Url.Decode(jwtComponents[0]);
+        }
+        catch (FormatException)
+        {
+            throw new Fido2VerificationException(Fido2ErrorMessages.MalformedSafetyNetJwt);
+        }
 
-        using var jwtHeaderJsonDoc = JsonDocument.Parse(Base64Url.Decode(jwtHeaderString));
+        using var jwtHeaderJsonDoc = JsonDocument.Parse(jwtHeaderBytes);
         var jwtHeaderJson = jwtHeaderJsonDoc.RootElement;
 
         if (!jwtHeaderJson.TryGetProperty("x5c", out var x5cEl))
@@ -97,7 +102,7 @@ public override (AttestationType, X509Certificate2[]) Verify(VerifyAttestationRe
         SecurityToken validatedToken;
         try
         { 
-            tokenHandler.ValidateToken(responseJWT, validationParameters, out validatedToken);
+            tokenHandler.ValidateToken(responseJwt, validationParameters, out validatedToken);
         }
         catch (SecurityTokenException ex)
         {

diff --git a/Src/Fido2/AttestationFormat/Apple.cs b/Src/Fido2/AttestationFormat/Apple.cs
@@ -14,7 +14,7 @@ internal sealed class Apple : AttestationVerifier
 {
     public static byte[] GetAppleAttestationExtensionValue(X509ExtensionCollection exts)
     {
-        var appleExtension = exts.FirstOrDefault(static e => e.Oid!.Value is "1.2.840.113635.100.8.2");
+        var appleExtension = exts.FirstOrDefault(static e => e.Oid?.Value is "1.2.840.113635.100.8.2");
 
         if (appleExtension is null || appleExtension.RawData is null || appleExtension.RawData.Length < 0x26)
             throw new Fido2VerificationException(Fido2ErrorCode.InvalidAttestation, "Extension with OID 1.2.840.113635.100.8.2 not found on Apple attestation credCert");

diff --git a/Src/Fido2/AttestationFormat/AppleAppAttest.cs b/Src/Fido2/AttestationFormat/AppleAppAttest.cs
@@ -13,7 +13,7 @@ internal sealed class AppleAppAttest : AttestationVerifier
 {
     public static byte[] GetAppleAppIdFromCredCertExtValue(X509ExtensionCollection exts)
     {
-        var appleExtension = exts.FirstOrDefault(static e => e.Oid!.Value is "1.2.840.113635.100.8.5");
+        var appleExtension = exts.FirstOrDefault(static e => e.Oid?.Value is "1.2.840.113635.100.8.5");
 
         if (appleExtension is null || appleExtension.RawData is null)
             throw new Fido2VerificationException("Extension with OID 1.2.840.113635.100.8.5 not found on Apple AppAttest credCert");

diff --git a/Src/Fido2/AttestationFormat/FidoU2f.cs b/Src/Fido2/AttestationFormat/FidoU2f.cs
@@ -36,15 +36,9 @@ public override (AttestationType, X509Certificate2[]) Verify(VerifyAttestationRe
         var pubKey = attCert.GetECDsaPublicKey()!;
         var keyParams = pubKey.ExportParameters(false);
 
-        if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+        if (!keyParams.Curve.Oid.Value!.Equals(ECCurve.NamedCurves.nistP256.Oid.Value, StringComparison.Ordinal))
         {
-            if (!keyParams.Curve.Oid.FriendlyName!.Equals(ECCurve.NamedCurves.nistP256.Oid.FriendlyName, StringComparison.Ordinal))
-                throw new Fido2VerificationException(Fido2ErrorCode.InvalidAttestation, "Attestation certificate public key is not an Elliptic Curve (EC) public key over the P-256 curve");
-        }
-        else
-        {
-            if (!keyParams.Curve.Oid.Value!.Equals(ECCurve.NamedCurves.nistP256.Oid.Value, StringComparison.Ordinal))
-                throw new Fido2VerificationException(Fido2ErrorCode.InvalidAttestation, "Attestation certificate public key is not an Elliptic Curve (EC) public key over the P-256 curve");
+            throw new Fido2VerificationException(Fido2ErrorCode.InvalidAttestation, "Attestation certificate public key is not an Elliptic Curve (EC) public key over the P-256 curve");
         }
 
         // 3. Extract the claimed rpIdHash from authenticatorData, and the claimed credentialId and credentialPublicKey from authenticatorData

diff --git a/Src/Fido2/AttestationFormat/Tpm.cs b/Src/Fido2/AttestationFormat/Tpm.cs
@@ -239,7 +239,7 @@ private static (string?, string?, string?) SANFromAttnCertExts(X509ExtensionColl
 
         foreach (var extension in exts)
         {
-            if (extension.Oid!.Value is "2.5.29.17") // subject alternative name
+            if (extension.Oid?.Value is "2.5.29.17") // subject alternative name
             {
                 if (extension.RawData.Length is 0)
                     throw new Fido2VerificationException(Fido2ErrorCode.InvalidAttestation, "SAN missing from TPM attestation certificate");
@@ -362,7 +362,7 @@ private static bool EKUFromAttnCertExts(X509ExtensionCollection exts, string exp
     {
         foreach (var ext in exts)
         {
-            if (ext.Oid!.Value is "2.5.29.37" && ext is X509EnhancedKeyUsageExtension enhancedKeyUsageExtension)
+            if (ext.Oid?.Value is "2.5.29.37" && ext is X509EnhancedKeyUsageExtension enhancedKeyUsageExtension)
             {
                 foreach (var oid in enhancedKeyUsageExtension.EnhancedKeyUsages)
                 {

diff --git a/Src/Fido2/AuthenticatorAttestationResponse.cs b/Src/Fido2/AuthenticatorAttestationResponse.cs
@@ -280,20 +280,20 @@ public ParsedAttestationObject(string fmt, CborMap attStmt, AuthenticatorData au
 
         public AuthenticatorData AuthData { get; }
 
-        internal static ParsedAttestationObject FromCbor(CborObject cbor)
+        internal static ParsedAttestationObject FromCbor(CborMap cbor)
         {
             if (!(
-                cbor["fmt"] is { Type: CborType.TextString } fmt &&
-                cbor["attStmt"] is { Type: CborType.Map } attStmt &&
-                cbor["authData"] is { Type: CborType.ByteString } authData))
+                cbor["fmt"] is CborTextString fmt &&
+                cbor["attStmt"] is CborMap attStmt &&
+                cbor["authData"] is CborByteString authData))
             {
                 throw new Fido2VerificationException(Fido2ErrorCode.MalformedAttestationObject, Fido2ErrorMessages.MalformedAttestationObject);
             }
 
             return new ParsedAttestationObject(
-                fmt      : (string)fmt,
-                attStmt  : (CborMap)attStmt,
-                authData : AuthenticatorData.Parse((byte[])authData)
+                fmt      : fmt,
+                attStmt  : attStmt,
+                authData : AuthenticatorData.Parse(authData)
             );
         }
     }

diff --git a/Src/Fido2/Cbor/CborByteString.cs b/Src/Fido2/Cbor/CborByteString.cs
@@ -1,9 +1,13 @@
-namespace Fido2NetLib.Cbor;
+using System;
+
+namespace Fido2NetLib.Cbor;
 
 public sealed class CborByteString : CborObject
 {
     public CborByteString(byte[] value)
     {
+        ArgumentNullException.ThrowIfNull(value);
+
         Value = value;
     }
 
@@ -12,4 +16,6 @@ public CborByteString(byte[] value)
     public byte[] Value { get; }
 
     public int Length => Value.Length;
+
+    public static implicit operator byte[](CborByteString value) => value.Value;
 }
diff --git a/Src/Fido2/Cbor/CborTextString.cs b/Src/Fido2/Cbor/CborTextString.cs
@@ -15,6 +15,8 @@ public CborTextString(string value)
 
     public string Value { get; }
 
+    public static implicit operator string(CborTextString value) => value.Value;
+
     public override bool Equals(object? obj)
     {
         return obj is CborTextString other && other.Value.Equals(Value, StringComparison.Ordinal);

diff --git a/Src/Fido2/Extensions/CryptoUtils.cs b/Src/Fido2/Extensions/CryptoUtils.cs
@@ -190,7 +190,7 @@ public static string CDPFromCertificateExts(X509ExtensionCollection exts)
         var cdp = "";
         foreach (var ext in exts)
         {
-            if (ext.Oid!.Value is "2.5.29.31") // id-ce-CRLDistributionPoints
+            if (ext.Oid?.Value is "2.5.29.31") // id-ce-CRLDistributionPoints
             {
                 var asnData = Asn1Element.Decode(ext.RawData);
 

diff --git a/Src/Fido2/Extensions/EcCurveExtensions.cs b/Src/Fido2/Extensions/EcCurveExtensions.cs
@@ -1,5 +1,4 @@
 using System;
-using System.Runtime.InteropServices;
 using System.Security.Cryptography;
 
 using Fido2NetLib.Objects;
@@ -10,32 +9,18 @@ internal static class EcCurveExtensions
 {
     public static COSE.EllipticCurve ToCoseCurve(this ECCurve curve)
     {
-        if (curve.Oid.FriendlyName is "secP256k1")
+        if (curve.Oid.FriendlyName is "secP256k1") // OID = 1.3.132.0.10
             return COSE.EllipticCurve.P256K;
 
-        if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
-        {
-            if (curve.Oid.FriendlyName!.Equals(ECCurve.NamedCurves.nistP256.Oid.FriendlyName, StringComparison.Ordinal))
-                return COSE.EllipticCurve.P256;
+        if (curve.Oid.Value!.Equals(ECCurve.NamedCurves.nistP256.Oid.Value, StringComparison.Ordinal))
+            return COSE.EllipticCurve.P256;
 
-            else if (curve.Oid.FriendlyName.Equals(ECCurve.NamedCurves.nistP384.Oid.FriendlyName, StringComparison.Ordinal))
-                return COSE.EllipticCurve.P384;
-
-            else if (curve.Oid.FriendlyName.Equals(ECCurve.NamedCurves.nistP521.Oid.FriendlyName, StringComparison.Ordinal))
-                return COSE.EllipticCurve.P521;
-        }
-        else
-        {
-            if (curve.Oid.Value!.Equals(ECCurve.NamedCurves.nistP256.Oid.Value, StringComparison.Ordinal))
-                return COSE.EllipticCurve.P256;
-
-            else if (curve.Oid.Value.Equals(ECCurve.NamedCurves.nistP384.Oid.Value, StringComparison.Ordinal))
-                return COSE.EllipticCurve.P384;
-
-            else if (curve.Oid.Value.Equals(ECCurve.NamedCurves.nistP521.Oid.Value, StringComparison.Ordinal))
-                return COSE.EllipticCurve.P521;
-        }
+        else if (curve.Oid.Value.Equals(ECCurve.NamedCurves.nistP384.Oid.Value, StringComparison.Ordinal))
+            return COSE.EllipticCurve.P384;
 
+        else if (curve.Oid.Value.Equals(ECCurve.NamedCurves.nistP521.Oid.Value, StringComparison.Ordinal))
+            return COSE.EllipticCurve.P521;
+
         throw new Exception($"Invalid ECCurve. Was {curve.Oid}");
     }
 }
diff --git a/Src/Fido2/Fido2ErrorMessages.cs b/Src/Fido2/Fido2ErrorMessages.cs
@@ -65,4 +65,9 @@ internal static class Fido2ErrorMessages
     public static readonly string InvalidFidoU2fAttestationSignature     = "Invalid fido-u2f attestation signature";
     public static readonly string InvalidPackedAttestationSignature      = "Invalid packed attestation signature";
     public static readonly string InvalidTpmAttestationSignature         = "Invalid TPM attestation signature";
+
+
+    public static readonly string InvalidSafetyNetVersion  = "Invalid version in SafetyNet data";
+    public static readonly string InvalidSafetyNetResponse = "Invalid response in SafetyNet data";
+    public static readonly string MalformedSafetyNetJwt    = "SafetyNet response JWT is malformed";
 }
diff --git a/Src/Fido2/Objects/CredentialPublicKey.cs b/Src/Fido2/Objects/CredentialPublicKey.cs
@@ -82,7 +82,7 @@ public bool Verify(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature)
                 }
 
             case COSE.KeyType.RSA:
-                using (RSA rsa = CreateRsa())
+                using (RSA rsa = CreateRSA())
                 {
                     return rsa.VerifyData(data, signature, CryptoUtils.HashAlgFromCOSEAlg(_alg), Padding);
                 }
@@ -93,7 +93,7 @@ public bool Verify(ReadOnlySpan<byte> data, ReadOnlySpan<byte> signature)
         throw new InvalidOperationException($"Missing or unknown kty {_type}");
     }
 
-    internal RSA CreateRsa()
+    internal RSA CreateRSA()
     {
         if (_type != COSE.KeyType.RSA)
         {
@@ -129,9 +129,9 @@ public ECDsa CreateECDsa()
         switch ((_alg, crv))
         {
             case (COSE.Algorithm.ES256K, COSE.EllipticCurve.P256K):
-                if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) // see https://github.com/dotnet/runtime/issues/47770
+                if (OperatingSystem.IsMacOS()) // see https://github.com/dotnet/runtime/issues/47770
                 {
-                    throw new PlatformNotSupportedException($"No support currently for secP256k1 on macOS");
+                    throw new PlatformNotSupportedException("The secP256k1 curve is not supported on macOS");
                 }
 
                 curve = ECCurve.CreateFromFriendlyName("secP256k1");