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/
43 stars 14 forks source link

Authr-MakeCred-Req-1 P-1 Excepts success when sending UV False and no PinUVAuthToken #699

Closed Molyna closed 12 months ago

Molyna commented 1 year 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?

1.7.9

What is the OS and the version are you running?

For desktop tools

For UAF mobile tools

Issue description

Given that we have an authenticator with built-in User Verification and UV set to true, makeCredUvNotRqd absent and ClientPin set to false in GetInfo we expect that a request sent in with options map absent and no PinUVAuthParam returns CTAP2_ERR_OPERATION_DENIED(0x27) but the tests expects a CTAP1_ERR_SUCCESS(0x0).

Operation Denied should trigger through MakeCredential 6.1.2.8.1.2 as MakeCredUVNotRqd is absent, and all 3 requirements for 6.1.2.8.1 are true with the authenticator being protected by some form of user verification, UV being false and PinUVAuthParam being not present. ClientPin is then false in 6.1.2.8.1.1 leading to 6.1.2.8.1.2 resulting in Operation Denied

The specification itself is a bit unclear as the options map is not present here, so we don't actually trigger 6.1.2.5 which has the first field in 6.1.2.5.1 as if the UV is not present it should be treated as having been present and set to false. The UV part of 6.1.2.8.1 then talks about UV being set to false, which might indicate that if the options parameter is absent then UV is never set, even if it has a default value of false. But this feels more like the wording of the specification is a bit wrong.

yackermann commented 1 year ago

@Molyna Are you not support PUAT protocol and only supporting UV?

Molyna commented 1 year ago

We do support both PUAT and UV. But during testing we have tested with tested with all optional setups as well, which included AlwaysUV false(absent and MakeCredUVNotRqd as false(absent) and no ClientPin set. When doing this we noticed this discrepancy in the tool vs the spec as the flow in the step guide would reach step 8.1.2 (8.1.1 if pin is set).

That said the intention of the spec feels a bit weird here along with the ambiguous wording in the spec. Which is why I do have an open question in the google group on this part of the spec. An authenticator with MakeUvNotRqd and AlwaysUV both set to false feels like a weird configuration, even if both of these have a default value of false.

That said, for testing we are running MakeUvNotRqd as true where the tests behave correctly.

yackermann commented 1 year ago

@Molyna 8.1.2 (8.1.1 if pin is set): What spec are you referring to?

Molyna commented 1 year ago

CTAP2.1

Step 8.1.2 of MakeCredential.

iirachek commented 12 months ago

I think we still need to consider all the subpoints regardless of whether the option map is present or not.

6.1.2.5 doesn't end with colon like other conditions, so I interpret If the options parameter is present, process all option keys and values present in the parameter. as self contained, and that lack of option parameter only skips the key-value processing. That way all option keys are considered to be absent by default.

In that case absent uv option is evaluated to its default false value, which is checked against in the 6.1.2.8.2; In the end, MakeCred operation for protected authenticator without supplied pinUvAuthParam ends with one of two error codes based on ClientPin state.

With that said, MakeCred tests were updated to correctly handle the case when both makeCredUvNotRqd and alwaysUv are false/absent.

Molyna commented 11 months ago

Fully agree, @iirachek, and I personally do not think the intention of the spec is the behavior I described in the ticket,. It is quite weird. I would think the intention is the same as you describe above.

But like you say yourself, to reach that conclusion, we need to interpret the specification and read between the lines a bit as there is ambiguity here. And ambiguity in specifications can lead to very messy results over time, plenty of examples of this through the years.

Been resolved here though and so should probably be taken through the google group discussion linked in my first comment, but been little interest in that post.