fido-alliance / conformance-test-tools-resources

Certification Test Tools Resources. For security and privacy related issues email tools@certification.fidoalliance.org
https://fidoalliance.org/certification/functional-certification/conformance/
40 stars 14 forks source link

pinUvAuthProtocol 2 being sent in credentialManagement tests #752

Closed nooobcoder closed 3 months ago

nooobcoder commented 5 months ago

By submitting this issue you are acknowledging that any information regarding this issue will be publicly available.

If you have privacy concerns, please email conformance-tools@fidoalliance.org

FIRST PRE CHECK

What protocol are you implementing?

NOTE: UAF 1.0 certification have been officially sunset. U2F 1.2 only supported version of U2F.

What is your implementation class?

If you are platform authenticator vendor, please email conformance-tools@fidoalliance.org

What is the version of the tool are you using?

FIDO Alliance - Certification Conformance Testing Tools v1.7.18-1

What is the OS and the version are you running?

Edition Windows 11 Enterprise Version 22H2 OS build 22621.3155 Experience Windows Feature Experience Pack 1000.22684.1000.0

For desktop tools

For UAF mobile tools

Issue description

The pinUvAuthProtocol value in getKeyAgreement command is being sent as "2" though my metadata statement mentions supports pinProtocol value "1", i.e.

{
    "pinUvAuthProtocols":["1"]
}

Which is the same thing in my authenticatorGetInfo response.

9. Mandatory Features -> Point 6 says,

"MUST include an array element with the value 2 in the authenticatorGetInfo response’s pinUvAuthProtocols member (i.e. support PIN/UV auth protocol two) if it includes any values at all."

-> Does that mean, the authenticator must support clientPin protocol "2" always? Or can we have an authenticator that support only pinUvAuthProtocol "1"?

I have also reproduced, by making the parameter "pinUvAuthProtocols" absent in the authenticatorGetInfo response, then the following tests/test suites fail in the conformance tool,

  1. Ext: Min Pin Length -> P-1 "Create a new credential with extensions containing minPinLenth set to True, and check that authenticator succeeds Check that MakeCredential response extensions contain minPinLength extension. If authenticator supports GetInfo minPINLength, check that minPinLength extension result equal to GetInfo.minPINLength"
  2. Entire Credential Management API testsuite
  3. Entire AuthenticatorConfig testsuite

Below is the error screenshot on executing credMgmt tests in the conformance tool 👇 image

-> Can you explain the reason behind such behavior, and when can such errors occur? We were expecting the test to pass, by making the parameter as absent in getInfo?

Below is the MDS statement that we are using, Note: MDS tests are not failing, when I am mentioning "pinUvAuthProtocols": [1] also making it "absent", which again violates the spec's mandatory feature.

{
  "aaguid": "11223344-5566-4312-2233-334455667702",
  "description": "xxxxxxxxxxxxxxxxxxxxxxxx",
  "schema": 3,
  "protocolFamily": "fido2",
  "authenticatorVersion": 1,
  "upv": [
    {
      "major": 1,
      "minor": 1
    },
    {
      "major": 1,
      "minor": 0
    }
  ],
  "authenticationAlgorithms": [
    "secp256r1_ecdsa_sha256_raw"
  ],
  "publicKeyAlgAndEncodings": [
    "cose"
  ],
  "attestationTypes": [
    "basic_full"
  ],
  "userVerificationDetails": [
    [
      {
        "userVerificationMethod": "none"
      }
    ],
    [
      {
        "userVerificationMethod": "presence_internal"
      }
    ],
    [
      {
        "userVerificationMethod": "passcode_external",
        "caDesc": {
          "base": 64,
          "minLength": 4,
          "maxRetries": 8
        }
      }
    ],
    [
      {
        "userVerificationMethod": "passcode_external",
        "caDesc": {
          "base": 64,
          "minLength": 4,
          "maxRetries": 8
        }
      },
      {
        "userVerificationMethod": "presence_internal"
      }
    ]
  ],
  "keyProtection": [
    "hardware",
    "secure_element"
  ],
  "matcherProtection": [
    "on_chip"
  ],
  "cryptoStrength": 128,
  "attachmentHint": [
    "external",
    "nfc"
  ],
  "tcDisplay": [],
  "attestationRootCertificates": [
    "removed"
  ],
  "icon": "removed",
  "authenticatorGetInfo": {
    "versions": [
      "U2F_V2",
      "FIDO_2_0",
      "FIDO_2_1"
    ],
    "extensions": [
      "hmac-secret",
      "credProtect",
      "minPinLength"
    ],
    "aaguid": "11223344556643122233334455667702",
    "options": {
      "rk": true,
      "clientPin": false,
      "up": true,
      "pinUvAuthToken": true,
      "authnrCfg": true,
      "credMgmt": true,
      "setMinPINLength": true,
      "makeCredUvNotRqd": true
    },
    "maxMsgSize": 1024,
    + "pinUvAuthProtocols": [
    +  1
    ],
    "maxCredentialCountInList": 10,
    "maxCredentialIdLength": 256,
    "transports": [
      "nfc",
      "usb"
    ],
    "algorithms": [
      {
        "alg": -7,
        "type": "public-key"
      }
    ],
    "minPINLength": 4,
    "maxRPIDsForSetMinPINLength": 35,
    "certifications": {
      "FIDO": 6,
      "CC-EAL": 5
    },
    "remainingDiscoverableCredentials": 8
  }
}

This issue also relates to https://github.com/fido-alliance/conformance-test-tools-resources/issues/684 which is closed, but left me questioning if test case in the conf tool is right or we can expect changes in the conformance tool?

iirachek commented 5 months ago

Does that mean, the authenticator must support clientPin protocol "2" always?

Yes, this is correct. Authenticators that implement CTAP 2.1 (FIDO_2_1) must also implement support for PIN/UV Auth protocol 2.

Because of that, the module for CTAP 2.1 conformance is unlikely to be updated to support authenticators with only pinUvAuthProtocol: 1.

If the pinUvAuthProtocol is absent from metadata statement, the tooling assumes this authenticator to be a UV-only authenticator, which aren't supported at the moment due to the difficulties of performing automated testing on them.

nooobcoder commented 5 months ago

Thanks @iirachek for sharing your thoughts on this. @dangfan, dmattisonfido, @herrjemand and @nuno0529 can you comment on the related issue?