Expose AAGUID and attestationCertificateKey for MDS lookup #234

bdewater · 2019-07-02T02:54:22Z

This resolves all cases except SafetyNet returning a zeroed out AAGUID.

Extracted from #208

spec/webauthn/authenticator_attestation_response_spec.rb

@@ -240,17 +226,12 @@

      expect(attestation_response.attestation_type).to eq("Basic")
      expect(attestation_response.attestation_trust_path).to be_kind_of(OpenSSL::X509::Certificate)
+      expect(attestation_response.aaguid).to eq("00000000-0000-0000-0000-000000000000")


grzuy

Thank you Bart.

I am still not sure I totally understand what particular issue we are fixing here.
Is it the fact the we don't provide easy access to the OID AAGUID in the 'fido-u2f' attestation format case?

grzuy · 2019-07-02T13:29:26Z

lib/webauthn/authenticator_attestation_response.rb

@@ -59,7 +59,8 @@ def type
    end

    def valid_attestation_statement?
-      @attestation_type, @attestation_trust_path = attestation_statement.valid?(authenticator_data, client_data.hash)
+      @attestation_type, @attestation_trust_path, @aaguid =
+        attestation_statement.valid?(authenticator_data, client_data.hash)


Do we really need the aaguid to be returned as part of #valid??
Can't we just have it be a separate method that gets the AAGUID from the attestation statement?

I'm even asking myself now why don't we have the same for attestation_type and attestation_trust_path. Possible I am forgetting a strong reason why we went with that approach :-)

Yes, after thinking about this a bit more I agree. I'll change it to attr_reader for AAGUID, AAID and attestationKeyIdentifier.

The current return types are a very literal implementation of the standard 😄 I can change that too.

The procedure returns either:

An error indicating that the attestation is invalid, or

An implementation-specific value representing the attestation type, and the trust path. This attestation trust path is either empty (in case of self attestation), an identifier of an ECDAA-Issuer public key (in the case of ECDAA), or a set of X.509 certificates.

bdewater · 2019-07-05T03:48:57Z

Ready for another round of review 👀

grzuy · 2019-07-06T13:50:07Z

lib/webauthn/authenticator_attestation_response.rb

@@ -35,7 +35,8 @@ def credential

    def attestation_statement
      @attestation_statement ||=
-        WebAuthn::AttestationStatement.from(attestation["fmt"], attestation["attStmt"])
+        WebAuthn::AttestationStatement.from(attestation["fmt"], attestation["attStmt"], authenticator_data,
+                                            client_data.hash)


I understand why you needed to make this change, but I took a couple of minutes to think about alternatives, because I am having doubts about this in particular...

What i mean is, the initial idea was for AttestationStatement object to wrap attStmt, AuthenticatorData to wrap authData, and so on...

This is, in some way, "breaking" that, in that is making the AttestationStatement a wrapper of more than what is actually called, and knowing more about the world that it "needed to" initially.

If all we need is "someone" to respond to the question "What is the AAGUID?", and that information is mixed between attStmt and authData, should we consider delegating that responsibility to the object that wraps both AttestationStatement and AuthenticatorData, i.e. AuthenticatorAttestationResponse.

Would that alternative approach also provide what you need for MDS lookup?

Sorry for the back and forth, and if my previous comment lead you this way.

Makes sense, I changed it. In the end there's no need for the certificate -> attested credential data fallback, since we already check they must be the same if the certificate version is present.

The data captured from a Feitian ePass NFC will be needed for testing extracting the Attestation Certificate Key Identifier from the certificate. The previous device had the AAGUID embedded inside the certificate.

… lookup

bdewater · 2019-07-06T22:15:05Z

lib/webauthn/authenticator_attestation_response.rb

+
+    def attestation_certificate_key
+      raw_subject_key_identifier(attestation_statement.attestation_certificate)&.unpack("H*")&.[](0)
+    end


To answer your earlier question @grzuy - my reason for exposing these here is twofold :)

Access during registration ceremony for metadata lookup

To be stored, so the metadata can be looked up later (during authentication or periodically in a background job)

In both cases the RP can evaluate whether the authenticator is acceptable at any given moment.

Is this only valid for fido-u2f fmt?

Do you have sources talking about this in particular. Did a quick search and could find anything.

Also, should this be part of the Metadata support PR?
Or is this already providing value because it's giving easier access to users who what to perform the metadata lookup themselves?

Yes, only for U2F: sources are FIDO Alliance MDS specs and the part of RFC5280 it references.

Since the metadata PR is pretty large and complex, I've been trying to split off PRs that add some value in itself and land those first. But it can be hard to see the total picture that way, and to be honest with the back and forth here and the metadata PR I've also started thinking of pushing this and the metadata handling up into the WebAuthn::AttestationStatement::Base subclasses.

I noticed that the MDS database is pretty empty at the moment: Android SafetyNet, Android keystore, Windows Hello and Yubikeys entries are all not present despite being certified. Or in other words: Feitian is the only hardware manufacturer I recognize.

Given that we already verify the validity of Android SafetyNet and Keystore with Google's root certificates, we don't need to consult the FIDO MDS to do the same thing again. The standard mentions MDS is just one way of doing things:

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 provides one way to obtain such information, using the aaguid in the attestedCredentialData in authData.

That would keep fido-u2f, packed and tpm to use the MDS, either with aaguid or attestationCertificateIdentifier (subjectKeyIdentifier).

Happy to chat on gitter or hop on a Skype call if that makes it easier to hash out the details :)

Gotcha.

I'm good with either the current implementation or pushing it to the attestation statement subclasses.
Your call.

Thanks for the detailed explanation.

…udes AAGUID

grzuy · 2019-07-10T14:51:12Z

lib/webauthn/authenticator_attestation_response.rb

+
+    def attestation_certificate_key
+      raw_subject_key_identifier(attestation_statement.attestation_certificate)&.unpack("H*")&.[](0)
+    end


Gotcha.

I'm good with either the current implementation or pushing it to the attestation statement subclasses.
Your call.

Thanks for the detailed explanation.

bdewater commented Jul 2, 2019

View reviewed changes

bdewater force-pushed the aaguid-fixes branch from 088cbc9 to ac8c573 Compare July 2, 2019 02:59

grzuy reviewed Jul 2, 2019

View reviewed changes

bdewater force-pushed the aaguid-fixes branch from ac8c573 to d849313 Compare July 5, 2019 03:47

bdewater changed the title ~~Return AAGUIDs from attestation certificates or attested credential data~~ Expose AAGUID and attestationCertificateKey for MDS lookup Jul 5, 2019

bdewater force-pushed the aaguid-fixes branch 2 times, most recently from 58aa2ce to 2e35dfe Compare July 5, 2019 03:57

grzuy reviewed Jul 6, 2019

View reviewed changes

bdewater force-pushed the aaguid-fixes branch from 2e35dfe to 2331bd3 Compare July 6, 2019 21:21

bdewater added 3 commits July 6, 2019 18:01

test: make None attestation test more realistic

a5ec3b2

test: change security key seed from a CTAP2 to a CTAP1 device

807344f

The data captured from a Feitian ePass NFC will be needed for testing extracting the Attestation Certificate Key Identifier from the certificate. The previous device had the AAGUID embedded inside the certificate.

feat: expose AAGUID and attestationKeyIdentifier for metadata service…

014635f

… lookup

bdewater force-pushed the aaguid-fixes branch from 2331bd3 to 014635f Compare July 6, 2019 22:02

bdewater commented Jul 6, 2019

View reviewed changes

test: update Android SafetyNet attestation response for one that incl…

25700d3

…udes AAGUID

grzuy approved these changes Jul 10, 2019

View reviewed changes

bdewater mentioned this pull request Aug 8, 2019

Metadata service support #208

Merged

test: update TPM attestation response for a real one from a Surface 4

f2d30ee

bdewater force-pushed the aaguid-fixes branch from 14c6444 to f2d30ee Compare August 14, 2019 12:56

bdewater merged commit fd9e2bb into cedarcode:master Aug 16, 2019

bdewater deleted the aaguid-fixes branch August 16, 2019 00:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose AAGUID and attestationCertificateKey for MDS lookup #234

Expose AAGUID and attestationCertificateKey for MDS lookup #234

bdewater commented Jul 2, 2019 •

edited

Loading

This comment was marked as outdated.

grzuy left a comment

grzuy Jul 2, 2019

bdewater Jul 2, 2019

grzuy Jul 3, 2019

bdewater commented Jul 5, 2019

grzuy Jul 6, 2019

grzuy Jul 6, 2019

bdewater Jul 6, 2019

bdewater Jul 6, 2019

grzuy Jul 8, 2019

grzuy Jul 8, 2019

bdewater Jul 8, 2019

grzuy Jul 10, 2019

grzuy Jul 10, 2019

Expose AAGUID and attestationCertificateKey for MDS lookup #234

Expose AAGUID and attestationCertificateKey for MDS lookup #234

Conversation

bdewater commented Jul 2, 2019 • edited Loading

This comment was marked as outdated.

grzuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdewater commented Jul 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdewater commented Jul 2, 2019 •

edited

Loading