From d4322f8537f48b70be44a4dd6ba76c2ea8b4fade Mon Sep 17 00:00:00 2001 From: Emil Lundberg Date: Fri, 1 Mar 2019 16:51:49 +0100 Subject: [PATCH] Improve support for unimplemented attstmt formats and untrusted attestation - Don't crash in step 13 if format is defined but not implemented - Make returned attestation type `Optional` - When `allowUntrustedAttestation = true`, don't crash in step 15 if trust roots cannot be resolved for supported attstmt formats See https://github.com/Yubico/java-webauthn-server/issues/16 --- .../webauthn/FinishRegistrationSteps.java | 70 +++++++---- .../com/yubico/webauthn/RelyingParty.java | 6 + .../RelyingPartyRegistrationSpec.scala | 113 ++++++++++++++---- 3 files changed, 139 insertions(+), 50 deletions(-) diff --git a/webauthn-server-core/src/main/java/com/yubico/webauthn/FinishRegistrationSteps.java b/webauthn-server-core/src/main/java/com/yubico/webauthn/FinishRegistrationSteps.java index ae780fb0d..ee7cd0a0e 100644 --- a/webauthn-server-core/src/main/java/com/yubico/webauthn/FinishRegistrationSteps.java +++ b/webauthn-server-core/src/main/java/com/yubico/webauthn/FinishRegistrationSteps.java @@ -53,7 +53,6 @@ import lombok.extern.slf4j.Slf4j; import static com.yubico.internal.util.ExceptionUtil.assure; -import static com.yubico.webauthn.data.AttestationType.NONE; @Builder @Slf4j @@ -373,24 +372,18 @@ class Step13 implements Step { private final List prevWarnings; @Override - public void validate() { - assure(formatSupported(), "Unsupported attestation statement format: %s", format()); - } + public void validate() {} @Override public Step14 nextStep() { - return new Step14(clientDataJsonHash, attestation, attestationStatementVerifier().get(), allWarnings()); + return new Step14(clientDataJsonHash, attestation, attestationStatementVerifier(), allWarnings()); } public String format() { return attestation.getFormat(); } - public boolean formatSupported() { - return attestationStatementVerifier().isPresent(); - } - - private Optional attestationStatementVerifier() { + public Optional attestationStatementVerifier() { switch (format()) { case "fido-u2f": return Optional.of(new FidoU2fAttestationStatementVerifier()); @@ -410,15 +403,19 @@ private Optional attestationStatementVerifier() { class Step14 implements Step { private final ByteArray clientDataJsonHash; private final AttestationObject attestation; - private final AttestationStatementVerifier attestationStatementVerifier; + private final Optional attestationStatementVerifier; private final List prevWarnings; @Override public void validate() { - assure( - attestationStatementVerifier.verifyAttestationSignature(attestation, clientDataJsonHash), - "Invalid attestation signature." - ); + attestationStatementVerifier.ifPresent(verifier -> { + assure( + verifier.verifyAttestationSignature(attestation, clientDataJsonHash), + "Invalid attestation signature." + ); + }); + + assure(attestationType() != null, "Failed to determine attestation type"); } @Override @@ -428,18 +425,40 @@ public Step15 nextStep() { public AttestationType attestationType() { try { - return attestationStatementVerifier.getAttestationType(attestation); + if (attestationStatementVerifier.isPresent()) { + return attestationStatementVerifier.get().getAttestationType(attestation); + } else { + switch (attestation.getFormat()) { + case "android-key": + // TODO delete this once android-key attestation verification is implemented + return AttestationType.BASIC; + case "tpm": + // TODO delete this once tpm attestation verification is implemented + if (attestation.getAttestationStatement().has("x5c")) { + return AttestationType.ATTESTATION_CA; + } else { + return AttestationType.ECDAA; + } + default: + throw new IllegalArgumentException("Failed to resolve attestation type; unknown attestation statement format: " + attestation.getFormat()); + } + } } catch (IOException | CoseException | CertificateException e) { throw new IllegalArgumentException("Failed to resolve attestation type.", e); } } public Optional> attestationTrustPath() { - if (attestationStatementVerifier instanceof X5cAttestationStatementVerifier) { - try { - return ((X5cAttestationStatementVerifier) attestationStatementVerifier).getAttestationTrustPath(attestation); - } catch (CertificateException e) { - throw new IllegalArgumentException("Failed to resolve attestation trust path.", e); + if (attestationStatementVerifier.isPresent()) { + AttestationStatementVerifier verifier = attestationStatementVerifier.get(); + if (verifier instanceof X5cAttestationStatementVerifier) { + try { + return ((X5cAttestationStatementVerifier) verifier).getAttestationTrustPath(attestation); + } catch (CertificateException e) { + throw new IllegalArgumentException("Failed to resolve attestation trust path.", e); + } + } else { + return Optional.empty(); } } else { return Optional.empty(); @@ -456,10 +475,6 @@ class Step15 implements Step { @Override public void validate() { - assure( - attestationType == AttestationType.SELF_ATTESTATION || attestationType == NONE || trustResolver().isPresent(), - "Failed to obtain attestation trust anchors." - ); } @Override @@ -504,6 +519,11 @@ class Step16 implements Step { @Override public void validate() { + assure( + trustResolver.isPresent() || allowUntrustedAttestation, + "Failed to obtain attestation trust anchors." + ); + switch (attestationType) { case SELF_ATTESTATION: assure(allowUntrustedAttestation, "Self attestation is not allowed."); diff --git a/webauthn-server-core/src/main/java/com/yubico/webauthn/RelyingParty.java b/webauthn-server-core/src/main/java/com/yubico/webauthn/RelyingParty.java index 383d8d220..bbe7dc034 100644 --- a/webauthn-server-core/src/main/java/com/yubico/webauthn/RelyingParty.java +++ b/webauthn-server-core/src/main/java/com/yubico/webauthn/RelyingParty.java @@ -231,6 +231,12 @@ public class RelyingParty { * attestation and none attestation. * *

+ * Regardless of the value of this option, invalid attestation statements of supported formats will always be + * rejected. For example, a "packed" attestation statement with an invalid signature will be rejected even if this + * option is set to true. + *

+ * + *

* The default is true. *

*/ diff --git a/webauthn-server-core/src/test/scala/com/yubico/webauthn/RelyingPartyRegistrationSpec.scala b/webauthn-server-core/src/test/scala/com/yubico/webauthn/RelyingPartyRegistrationSpec.scala index 134908148..f38062130 100644 --- a/webauthn-server-core/src/test/scala/com/yubico/webauthn/RelyingPartyRegistrationSpec.scala +++ b/webauthn-server-core/src/test/scala/com/yubico/webauthn/RelyingPartyRegistrationSpec.scala @@ -30,6 +30,7 @@ import java.nio.charset.Charset import java.security.MessageDigest import java.security.KeyPair import java.security.PrivateKey +import java.security.SignatureException import java.security.cert.X509Certificate import java.util.Optional @@ -52,6 +53,7 @@ import com.yubico.webauthn.data.ByteArray import com.yubico.webauthn.data.RegistrationExtensionInputs import com.yubico.webauthn.data.PublicKeyCredentialDescriptor import com.yubico.webauthn.data.UserIdentity +import com.yubico.webauthn.data.AttestationConveyancePreference import com.yubico.webauthn.data.Generators._ import com.yubico.webauthn.test.Util.toStepWithUtilities import javax.security.auth.x500.X500Principal @@ -570,43 +572,63 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD ) } - def checkFailure(format: String): Unit = { - it(s"""Fails if fmt is "${format}".""") { + def checkUnknown(format: String): Unit = { + it(s"""Returns no known attestation statement verifier if fmt is "${format}".""") { val steps = setup(format) val step: FinishRegistrationSteps#Step13 = steps.begin.next.next.next.next.next.next.next.next.next.next.next.next - step.validations shouldBe a [Failure[_]] - step.validations.failed.get shouldBe an [IllegalArgumentException] - step.tryNext shouldBe a [Failure[_]] + step.validations shouldBe a [Success[_]] + step.tryNext shouldBe a [Success[_]] + step.format should equal (format) + step.attestationStatementVerifier.asScala shouldBe empty } } - def checkSuccess(format: String): Unit = { - it(s"""Succeeds if fmt is "${format}".""") { + def checkKnown(format: String): Unit = { + it(s"""Returns a known attestation statement verifier if fmt is "${format}".""") { val steps = setup(format) val step: FinishRegistrationSteps#Step13 = steps.begin.next.next.next.next.next.next.next.next.next.next.next.next step.validations shouldBe a [Success[_]] step.tryNext shouldBe a [Success[_]] step.format should equal (format) - step.formatSupported should be(true) + step.attestationStatementVerifier.asScala should not be empty } } - ignore("Succeeds if fmt is android-key.") { checkSuccess("android-key") } - ignore("Succeeds if fmt is android-safetynet.") { checkSuccess("android-safetynet") } - checkSuccess("fido-u2f") - checkSuccess("none") - checkSuccess("packed") - ignore("Succeeds if fmt is tpm.") { checkSuccess("tpm") } + checkKnown("android-safetynet") + checkKnown("fido-u2f") + checkKnown("none") + checkKnown("packed") - checkFailure("FIDO-U2F") - checkFailure("Fido-U2F") - checkFailure("bleurgh") + checkUnknown("android-key") + checkUnknown("tpm") + + checkUnknown("FIDO-U2F") + checkUnknown("Fido-U2F") + checkUnknown("bleurgh") } describe("14. Verify that attStmt is a correct attestation statement, conveying a valid attestation signature, by using the attestation statement format fmt’s verification procedure given attStmt, authData and the hash of the serialized client data computed in step 7.") { + describe("If allowUntrustedAttestation is set,") { + it("a fido-u2f attestation is still rejected if invalid.") { + val testData = RegistrationTestData.FidoU2f.BasicAttestation.editAttestationObject("attStmt", { attStmtNode: JsonNode => + attStmtNode.asInstanceOf[ObjectNode] + .set("sig", jsonFactory.binaryNode(Array(0, 0, 0, 0))) + }) + val steps = finishRegistration( + testData = testData, + allowUntrustedAttestation = true + ) + val step: FinishRegistrationSteps#Step14 = steps.begin.next.next.next.next.next.next.next.next.next.next.next.next.next + + step.validations shouldBe a [Failure[_]] + step.validations.failed.get.getCause shouldBe a [SignatureException] + step.tryNext shouldBe a [Failure[_]] + } + } + describe("For the fido-u2f statement format,") { it("the default test case is a valid basic attestation.") { val steps = finishRegistration(testData = RegistrationTestData.FidoU2f.BasicAttestation) @@ -632,7 +654,7 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD val step: FinishRegistrationSteps#Step14 = new steps.Step14( new BouncyCastleCrypto().hash(new ByteArray(testData.clientDataJsonBytes.getBytes.updated(20, (testData.clientDataJsonBytes.getBytes()(20) + 1).toByte))), new AttestationObject(testData.attestationObject), - new FidoU2fAttestationStatementVerifier, + Some(new FidoU2fAttestationStatementVerifier).asJava, Nil.asJava ) @@ -651,7 +673,7 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD val step: FinishRegistrationSteps#Step14 = new steps.Step14( new BouncyCastleCrypto().hash(testData.clientDataJsonBytes), new AttestationObject(testData.attestationObject), - new FidoU2fAttestationStatementVerifier, + Some(new FidoU2fAttestationStatementVerifier).asJava, Nil.asJava ) @@ -683,7 +705,7 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD val step: FinishRegistrationSteps#Step14 = new steps.Step14( new BouncyCastleCrypto().hash(testData.clientDataJsonBytes), new AttestationObject(testData.attestationObject), - new FidoU2fAttestationStatementVerifier, + Some(new FidoU2fAttestationStatementVerifier).asJava, Nil.asJava ) @@ -778,7 +800,7 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD val step: FinishRegistrationSteps#Step14 = new steps.Step14( new BouncyCastleCrypto().hash(testData.clientDataJsonBytes), new AttestationObject(testData.attestationObject), - new NoneAttestationStatementVerifier, + Some(new NoneAttestationStatementVerifier).asJava, Nil.asJava ) @@ -809,7 +831,7 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD val steps = finishRegistration(testData = RegistrationTestData.Packed.BasicAttestation) val step: FinishRegistrationSteps#Step14 = steps.begin.next.next.next.next.next.next.next.next.next.next.next.next.next - step.getAttestationStatementVerifier shouldBe a [PackedAttestationStatementVerifier] + step.getAttestationStatementVerifier.get shouldBe a [PackedAttestationStatementVerifier] } describe("the verification procedure is:") { @@ -1188,12 +1210,12 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD it("the attestation statement verifier implementation is AndroidSafetynetAttestationStatementVerifier.") { val steps = finishRegistration( testData = defaultTestData, - allowUntrustedAttestation = true, + allowUntrustedAttestation = false, rp = defaultTestData.rpId ) val step: FinishRegistrationSteps#Step14 = steps.begin.next.next.next.next.next.next.next.next.next.next.next.next.next - step.getAttestationStatementVerifier shouldBe an [AndroidSafetynetAttestationStatementVerifier] + step.getAttestationStatementVerifier.get shouldBe an [AndroidSafetynetAttestationStatementVerifier] } describe("the verification procedure is:") { @@ -1306,6 +1328,16 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD step.validations shouldBe a [Success[_]] step.tryNext shouldBe a [Success[_]] } + + it("Unknown attestation statement formats fail.") { + val steps = finishRegistration(testData = RegistrationTestData.FidoU2f.BasicAttestation.editAttestationObject("fmt", "urgel")) + val step: FinishRegistrationSteps#Step14 = steps.begin.next.next.next.next.next.next.next.next.next.next.next.next.next + + step.validations shouldBe a [Failure[_]] + step.validations.failed.get shouldBe an [IllegalArgumentException] + step.tryNext shouldBe a [Failure[_]] + } + } describe("15. If validation is successful, obtain a list of acceptable trust anchors (attestation root certificates or ECDAA-Issuer public keys) for that attestation type and attestation statement format fmt, from a trusted source or from policy. For example, the FIDO Metadata Service [FIDOMetadataService] provides one way to obtain such information, using the aaguid in the attestedCredentialData in authData.") { @@ -1362,7 +1394,6 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD step.tryNext shouldBe a [Success[_]] } } - } describe("16. Assess the attestation trustworthiness using the outputs of the verification procedure in step 14, as follows:") { @@ -1602,9 +1633,41 @@ class RelyingPartyRegistrationSpec extends FunSpec with Matchers with GeneratorD steps.run.isAttestationTrusted should be (false) } + it("The test case with unknown attestation fails.") { + val testData = RegistrationTestData.FidoU2f.BasicAttestation.editAttestationObject("fmt", "urgel") + val steps = finishRegistration( + testData = testData, + allowUntrustedAttestation = true, + credentialRepository = Some(emptyCredentialRepository) + ) + val result = Try(steps.run) + result.failed.get shouldBe an [IllegalArgumentException] + } + describe("NOTE: However, if permitted by policy, the Relying Party MAY register the credential ID and credential public key but treat the credential as one with self attestation (see §6.4.3 Attestation Types). If doing so, the Relying Party is asserting there is no cryptographic proof that the public key credential has been generated by a particular authenticator model. See [FIDOSecRef] and [UAFProtocol] for a more detailed discussion.") { it("Nothing to test.") {} } + + def testUntrusted(testData: RegistrationTestData): Unit = { + val fmt = new AttestationObject(testData.attestationObject).getFormat + it(s"""A test case with good "${fmt}" attestation but no metadata service succeeds, but reports attestation as not trusted.""") { + val testData = RegistrationTestData.FidoU2f.BasicAttestation + val steps = finishRegistration( + testData = testData, + metadataService = None, + allowUntrustedAttestation = true, + credentialRepository = Some(emptyCredentialRepository) + ) + steps.run.getKeyId.getId should be (testData.response.getId) + steps.run.isAttestationTrusted should be (false) + } + } + + testUntrusted(RegistrationTestData.AndroidKey.BasicAttestation) + testUntrusted(RegistrationTestData.AndroidSafetynet.BasicAttestation) + testUntrusted(RegistrationTestData.FidoU2f.BasicAttestation) + testUntrusted(RegistrationTestData.NoneAttestation.Default) + testUntrusted(RegistrationTestData.Tpm.PrivacyCa) } it("(Deleted) If verification of the attestation statement failed, the Relying Party MUST fail the registration ceremony.") {