Skip to content

Commit

Permalink
Improve support for unimplemented attstmt formats and untrusted attes…
Browse files Browse the repository at this point in the history 
…tation

- 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 #16
  • Loading branch information
@emlun
emlun committed Mar 1, 2019
1 parent 1a1fc56 commit d4322f8
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 50 deletions.
70 changes: 45 additions & 25 deletions webauthn-server-core/src/main/java/com/yubico/webauthn/FinishRegistrationSteps.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -373,24 +372,18 @@ class Step13 implements Step<Step14> {
private final List<String> 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> attestationStatementVerifier() {
public Optional<AttestationStatementVerifier> attestationStatementVerifier() {
switch (format()) {
case "fido-u2f":
return Optional.of(new FidoU2fAttestationStatementVerifier());
Expand All @@ -410,15 +403,19 @@ private Optional<AttestationStatementVerifier> attestationStatementVerifier() {
class Step14 implements Step<Step15> {
private final ByteArray clientDataJsonHash;
private final AttestationObject attestation;
private final AttestationStatementVerifier attestationStatementVerifier;
private final Optional<AttestationStatementVerifier> attestationStatementVerifier;
private final List<String> 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
Expand All @@ -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<List<X509Certificate>> 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();
Expand All @@ -456,10 +475,6 @@ class Step15 implements Step<Step16> {

@Override
public void validate() {
assure(
attestationType == AttestationType.SELF_ATTESTATION || attestationType == NONE || trustResolver().isPresent(),
"Failed to obtain attestation trust anchors."
);
}

@Override
Expand Down Expand Up @@ -504,6 +519,11 @@ class Step16 implements Step<Step17> {

@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.");
Expand Down
6 changes: 6 additions & 0 deletions webauthn-server-core/src/main/java/com/yubico/webauthn/RelyingParty.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,12 @@ public class RelyingParty {
* attestation and none attestation.
*
* <p>
* 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 <code>true</code>.
* </p>
*
* <p>
* The default is <code>true</code>.
* </p>
*/
Expand Down
113 changes: 88 additions & 25 deletions webauthn-server-core/src/test/scala/com/yubico/webauthn/RelyingPartyRegistrationSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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
)

Expand All @@ -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
)

Expand Down Expand Up @@ -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
)

Expand Down Expand Up @@ -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
)

Expand Down Expand Up @@ -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:") {
Expand Down Expand Up @@ -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:") {
Expand Down Expand Up @@ -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.") {
Expand Down Expand Up @@ -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:") {
Expand Down Expand Up @@ -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.") {
Expand Down

0 comments on commit d4322f8

Please sign in to comment.