Merge pull request #42 from Yubico/verify-credkey-alg

Verify algorithm of credential keys on registration

NEWS

@@ -8,6 +8,9 @@ Changes:
   containing methods and value constants equivalent to the previous enum.
 * The return type of `PublicKeyCredentialDescriptor.getTransports()` is now a
   `SortedSet` instead of `Set`. The builder still accepts a plain `Set`.
+* Registration ceremony now verifies that the returned credential public key
+  matches one of the algorithms specified in
+  `RelyingParty.preferredPubkeyParams` and can be successfully parsed.
 
 New features:
 

webauthn-server-core/src/main/java/com/yubico/webauthn/FinishRegistrationSteps.java

@@ -25,6 +25,7 @@
 package com.yubico.webauthn;
 
 import COSE.CoseException;
+import com.upokecenter.cbor.CBORObject;
 import com.yubico.internal.util.CollectionUtil;
 import com.yubico.webauthn.attestation.Attestation;
 import com.yubico.webauthn.attestation.MetadataService;
@@ -40,19 +41,23 @@
 import com.yubico.webauthn.data.PublicKeyCredentialDescriptor;
 import com.yubico.webauthn.data.UserVerificationRequirement;
 import java.io.IOException;
+import java.security.NoSuchAlgorithmException;
 import java.security.cert.CertificateEncodingException;
 import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
+import java.security.spec.InvalidKeySpecException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
+import java.util.stream.Collectors;
 import lombok.Builder;
 import lombok.Value;
 import lombok.extern.slf4j.Slf4j;
 
 import static com.yubico.internal.util.ExceptionUtil.assure;
+import static com.yubico.internal.util.ExceptionUtil.wrapAndLog;
 
 @Builder
 @Slf4j
@@ -643,7 +648,7 @@ public Step19 nextStep() {
     }
 
     @Value
-    class Step19 implements Step<Finished> {
+    class Step19 implements Step<CustomLastStep> {
         private final AttestationType attestationType;
         private final Optional<Attestation> attestationMetadata;
         private final boolean attestationTrusted;
@@ -653,6 +658,40 @@ class Step19 implements Step<Finished> {
         public void validate() {
         }
 
+        @Override
+        public CustomLastStep nextStep() {
+            return new CustomLastStep(attestationType, attestationMetadata, attestationTrusted, allWarnings());
+        }
+    }
+
+    /**
+     * Steps that aren't yet standardised in a stable edition of the spec
+     */
+    @Value
+    class CustomLastStep implements Step<Finished> {
+        private final AttestationType attestationType;
+        private final Optional<Attestation> attestationMetadata;
+        private final boolean attestationTrusted;
+        private final List<String> prevWarnings;
+
+        @Override
+        public void validate() {
+            ByteArray publicKeyCose = response.getResponse().getAttestation().getAuthenticatorData().getAttestedCredentialData().get().getCredentialPublicKey();
+            CBORObject publicKeyCbor = CBORObject.DecodeFromBytes(publicKeyCose.getBytes());
+            int alg = publicKeyCbor.get(CBORObject.FromObject(3)).AsInt32();
+            assure(
+                request.getPubKeyCredParams().stream().anyMatch(pkcparam -> pkcparam.getAlg().getId() == alg),
+                "Unrequested credential key algorithm: got %d, expected one of: %s",
+                alg,
+                request.getPubKeyCredParams().stream().map(pkcparam -> pkcparam.getAlg()).collect(Collectors.toList())
+            );
+            try {
+                WebAuthnCodecs.importCosePublicKey(publicKeyCose);
+            } catch (CoseException | IOException | InvalidKeySpecException | NoSuchAlgorithmException e) {
+                throw wrapAndLog(log, "Failed to parse credential public key", e);
+            }
+        }
+
         @Override
         public Finished nextStep() {
             return new Finished(attestationType, attestationMetadata, attestationTrusted, allWarnings());

webauthn-server-core/src/main/java/com/yubico/webauthn/WebAuthnCodecs.java

@@ -93,7 +93,7 @@ private static ECPublicKey importCoseP256PublicKey(CBORObject cose) throws CoseE
         return (ECPublicKey) new OneKey(cose).AsPublicKey();
     }
 
-    private static PublicKey importCoseEdDsaPublicKey(CBORObject cose) {
+    private static PublicKey importCoseEdDsaPublicKey(CBORObject cose) throws InvalidKeySpecException, NoSuchAlgorithmException {
         final int curveId = cose.get(CBORObject.FromObject(-1)).AsInt32();
         switch (curveId) {
             case 6: return importCoseEd25519PublicKey(cose);
@@ -102,19 +102,15 @@ private static PublicKey importCoseEdDsaPublicKey(CBORObject cose) {
         }
     }
 
-    private static PublicKey importCoseEd25519PublicKey(CBORObject cose) {
+    private static PublicKey importCoseEd25519PublicKey(CBORObject cose) throws InvalidKeySpecException, NoSuchAlgorithmException {
         final ByteArray rawKey = new ByteArray(cose.get(CBORObject.FromObject(-2)).GetByteString());
         final ByteArray x509Key = new ByteArray(new byte[]{0x30, (byte) (ED25519_CURVE_OID.size() + 3 + rawKey.size()) })
             .concat(ED25519_CURVE_OID)
             .concat(new ByteArray(new byte[]{ 0x03, (byte) (rawKey.size() + 1), 0}))
             .concat(rawKey);
 
-        try {
-            KeyFactory kFact = KeyFactory.getInstance("EdDSA", new BouncyCastleProvider());
-            return kFact.generatePublic(new X509EncodedKeySpec(x509Key.getBytes()));
-        } catch (InvalidKeySpecException | NoSuchAlgorithmException e) {
-            throw new RuntimeException(e);
-        }
+        KeyFactory kFact = KeyFactory.getInstance("EdDSA", new BouncyCastleProvider());
+        return kFact.generatePublic(new X509EncodedKeySpec(x509Key.getBytes()));
     }
 
     static Optional<COSEAlgorithmIdentifier> getCoseKeyAlg(ByteArray key) {

webauthn-server-core/src/test/scala/com/yubico/webauthn/RegistrationTestData.scala

@@ -186,7 +186,7 @@ object RegistrationTestData {
 
     val BasicAttestationEdDsa: RegistrationTestData = new RegistrationTestData(
       alg = COSEAlgorithmIdentifier.EdDSA,
-      attestationObject = ByteArray.fromHex("bf686175746844617461588d49960de5880e8c687434170f6476605b8fe4aeb9a28632c7995cf3ba831d97634100000539000102030405060708090a0b0c0d0e0f00203fc68d69a967669622022c385118ee75bdfb7a475b319d632c3e880259ca70e9a403270101200621582c302a300506032b65700321006222d82d1bced50433523789bb427b3b3f05a04a4dc9cfbc1fa34c9de7d8ef0363666d74667061636b65646761747453746d74bf63616c67266373696758483046022100afed1c9dc532f5301232f187fa4a294b93209cbd106c088dd78a66cb8047c6dd022100b1ae89e10d909f88377485b3ed82371cd7a29d7cfbcd9244eaa3307a4d4fe37f63783563815901e6308201e230820189a00302010202020539300a06082a8648ce3d04030230673123302106035504030c1a59756269636f20576562417574686e20756e6974207465737473310f300d060355040a0c0659756269636f31223020060355040b0c1941757468656e74696361746f72204174746573746174696f6e310b3009060355040613025345301e170d3138303930363137343230305a170d3138303930363137343230305a30673123302106035504030c1a59756269636f20576562417574686e20756e6974207465737473310f300d060355040a0c0659756269636f31223020060355040b0c1941757468656e74696361746f72204174746573746174696f6e310b30090603550406130253453059301306072a8648ce3d020106082a8648ce3d03010703420004afc54bc52dfc96cfbd74088f2e3bd1ba21896c5d5a5420c860c7109ac3d782c7680461b464b9e6e195b9838b57ca7355e49cede0a8881f0fa30e5073cb9a8f63a32530233021060b2b0601040182e51c01010404120410000102030405060708090a0b0c0d0e0f300a06082a8648ce3d040302034700304402203b636e6c758264968f487af40ea7e94faef91ca2a5899ae3a7973aaa97028de20220593083c7743a08bc7c7f783f958af61a966e411337ae1f38ffcbc7db83e9533bffff"),
+      attestationObject = ByteArray.fromHex("bf686175746844617461588149960de5880e8c687434170f6476605b8fe4aeb9a28632c7995cf3ba831d97634100000539000102030405060708090a0b0c0d0e0f00202467806d264a3d2c96b2aeb2540b2be1b19eb5e88723662b359f28b7231dcaf1a4032701012006215820e6187bd5599aeffbc136a9167c1fc272d7558f0416fde145512eeaf02f2b27e463666d74667061636b65646761747453746d74bf63616c67266373696758483046022100bd2a622b4aff927ac137a1b8622beb67f11589af760e818c7aac8e3dc1766aee022100f14885243bc9bfa5a181469ecee5b5257360aeb389f5597009d7d6bc0a4006b563783563815901e7308201e330820189a00302010202020539300a06082a8648ce3d04030230673123302106035504030c1a59756269636f20576562417574686e20756e6974207465737473310f300d060355040a0c0659756269636f31223020060355040b0c1941757468656e74696361746f72204174746573746174696f6e310b3009060355040613025345301e170d3138303930363137343230305a170d3138303930363137343230305a30673123302106035504030c1a59756269636f20576562417574686e20756e6974207465737473310f300d060355040a0c0659756269636f31223020060355040b0c1941757468656e74696361746f72204174746573746174696f6e310b30090603550406130253453059301306072a8648ce3d020106082a8648ce3d030107034200045fc21ceae3b9f5be42a5a0c9e240759316314759cc56f25cf82ffb8dfc4584ed96b6af7b13017793898ce72ced725ad2b0ee56d4d463e92e204c8a409e1f2e66a32530233021060b2b0601040182e51c01010404120410000102030405060708090a0b0c0d0e0f300a06082a8648ce3d0403020348003045022100c372ad425396e695606663c0c6e93ad1f76af622c8358d20f028ed50e855e06602206761970cbf25b394b17edb42ea7594370aa77f540c8ba25b81de510b6651597bffff"),
       clientDataJson = """{"challenge":"AAEBAgMFCA0VIjdZEGl5Yls","origin":"https://localhost","type":"webauthn.create","tokenBinding":{"status":"supported"},"clientExtensions":{}}""",
     ) { override def regenerate() = TestAuthenticator.createBasicAttestedCredential(keyAlgorithm = COSEAlgorithmIdentifier.EdDSA, attestationMaker = AttestationMaker.packed(AttestationSigner.selfsigned(COSEAlgorithmIdentifier.ES256))) }
 
@@ -338,7 +338,7 @@ case class RegistrationTestData(
       .rp(rpId)
       .user(userId)
       .challenge(clientData.getChallenge)
-      .pubKeyCredParams(List(PublicKeyCredentialParameters.builder().alg(COSEAlgorithmIdentifier.ES256).build()).asJava)
+      .pubKeyCredParams(List(PublicKeyCredentialParameters.ES256, PublicKeyCredentialParameters.EdDSA, PublicKeyCredentialParameters.RS256).asJava)
       .extensions(requestedExtensions)
       .authenticatorSelection(authenticatorSelection.asJava)
       .build()

webauthn-server-core/src/test/scala/com/yubico/webauthn/RelyingPartyRegistrationSpec.scala

@@ -50,10 +50,12 @@ import com.yubico.webauthn.data.CollectedClientData
 import com.yubico.webauthn.data.COSEAlgorithmIdentifier
 import com.yubico.webauthn.data.Generators._
 import com.yubico.webauthn.data.PublicKeyCredentialDescriptor
+import com.yubico.webauthn.data.PublicKeyCredentialParameters
 import com.yubico.webauthn.data.RegistrationExtensionInputs
 import com.yubico.webauthn.data.RelyingPartyIdentity
 import com.yubico.webauthn.data.UserIdentity
 import com.yubico.webauthn.data.UserVerificationRequirement
+import com.yubico.webauthn.exception.RegistrationFailedException
 import com.yubico.webauthn.test.Util.toStepWithUtilities
 import com.yubico.webauthn.TestAuthenticator.AttestationCert
 import com.yubico.webauthn.TestAuthenticator.AttestationMaker
@@ -108,16 +110,17 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD
     allowUntrustedAttestation: Boolean = false,
     callerTokenBindingId: Option[ByteArray] = None,
     credentialId: Option[ByteArray] = None,
-    credentialRepository: Option[CredentialRepository] = None,
+    credentialRepository: CredentialRepository = unimplementedCredentialRepository,
     metadataService: Option[MetadataService] = None,
     origins: Option[Set[String]] = None,
+    preferredPubkeyParams: List[PublicKeyCredentialParameters] = Nil,
     rp: RelyingPartyIdentity = RelyingPartyIdentity.builder().id("localhost").name("Test party").build(),
     testData: RegistrationTestData
   ): FinishRegistrationSteps = {
     val builder = RelyingParty.builder()
       .identity(rp)
-      .credentialRepository(credentialRepository.getOrElse(unimplementedCredentialRepository))
-      .preferredPubkeyParams(Nil.asJava)
+      .credentialRepository(credentialRepository)
+      .preferredPubkeyParams(preferredPubkeyParams.asJava)
       .allowOriginPort(allowOriginPort)
       .allowOriginSubdomain(allowOriginSubdomain)
       .allowUntrustedAttestation(allowUntrustedAttestation)
@@ -1776,7 +1779,7 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD
           val steps = finishRegistration(
             allowUntrustedAttestation = true,
             testData = testData,
-            credentialRepository = Some(credentialRepository)
+            credentialRepository = credentialRepository
           )
           val step: FinishRegistrationSteps#Step17 = steps.begin.next.next.next.next.next.next.next.next.next.next.next.next.next.next.next.next
 
@@ -1797,7 +1800,7 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD
           val steps = finishRegistration(
             allowUntrustedAttestation = true,
             testData = testData,
-            credentialRepository = Some(credentialRepository)
+            credentialRepository = credentialRepository
           )
           val step: FinishRegistrationSteps#Step17 = steps.begin.next.next.next.next.next.next.next.next.next.next.next.next.next.next.next.next
 
@@ -1812,7 +1815,7 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD
           val steps = finishRegistration(
             testData = testData,
             metadataService = Some(new TestMetadataService(Some(Attestation.builder().trusted(true).build()))),
-            credentialRepository = Some(emptyCredentialRepository)
+            credentialRepository = emptyCredentialRepository
           )
           steps.run.getKeyId.getId should be (testData.response.getId)
           steps.run.isAttestationTrusted should be (true)
@@ -1825,7 +1828,7 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD
           val steps = finishRegistration(
             testData = testData,
             allowUntrustedAttestation = true,
-            credentialRepository = Some(emptyCredentialRepository)
+            credentialRepository = emptyCredentialRepository
           )
           steps.run.getKeyId.getId should be (testData.response.getId)
           steps.run.isAttestationTrusted should be (false)
@@ -1836,7 +1839,7 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD
           val steps = finishRegistration(
             testData = testData,
             allowUntrustedAttestation = true,
-            credentialRepository = Some(emptyCredentialRepository)
+            credentialRepository = emptyCredentialRepository
           )
           val result = Try(steps.run)
           result.failed.get shouldBe an [IllegalArgumentException]
@@ -1854,7 +1857,7 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD
               testData = testData,
               metadataService = None,
               allowUntrustedAttestation = true,
-              credentialRepository = Some(emptyCredentialRepository)
+              credentialRepository = emptyCredentialRepository
             )
             steps.run.getKeyId.getId should be (testData.response.getId)
             steps.run.isAttestationTrusted should be (false)
@@ -1977,4 +1980,39 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD
 
   }
 
+  describe("Additions in L2-WD02 editor's draft:") {
+
+    describe("Verify that the \"alg\" parameter in the credential public key in authData matches the alg attribute of one of the items in options.pubKeyCredParams.") {
+      it("An ES256 key succeeds if ES256 was a requested algorithm.") {
+        val testData = RegistrationTestData.FidoU2f.BasicAttestation
+        val result = finishRegistration(
+          testData = testData,
+          credentialRepository = emptyCredentialRepository,
+          allowUntrustedAttestation = true
+        ).run
+
+        result should not be null
+        result.getPublicKeyCose should not be null
+      }
+
+      it("An ES256 key fails if only RSA and EdDSA are allowed.") {
+        val testData = RegistrationTestData.FidoU2f.BasicAttestation
+        val result = Try(finishRegistration(
+          testData = testData.copy(
+            overrideRequest = Some(testData.request.toBuilder
+              .pubKeyCredParams(List(PublicKeyCredentialParameters.EdDSA, PublicKeyCredentialParameters.RS256).asJava)
+              .build()
+            )
+          ),
+          credentialRepository = emptyCredentialRepository,
+          allowUntrustedAttestation = true
+        ).run)
+
+        result shouldBe a [Failure[_]]
+        result.failed.get shouldBe an [IllegalArgumentException]
+      }
+    }
+
+  }
+
 }

webauthn-server-core/src/test/scala/com/yubico/webauthn/WebAuthnTestCodecs.scala

@@ -76,7 +76,7 @@ object WebAuthnTestCodecs {
     coseKey.put(3L, COSEAlgorithmIdentifier.EdDSA.getId)
     coseKey.put(-1L, 6L) // crv: Ed25519
 
-    coseKey.put(-2L, key.getEncoded)
+    coseKey.put(-2L, key.getEncoded.takeRight(32)) // Strip ASN.1 prefix
     new ByteArray(CBORObject.FromObject(coseKey).EncodeToBytes)
   }