Skip to content

FIDO Conformance Tools v1.7.15 fixes #456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed

FIDO Conformance Tools v1.7.15 fixes #456

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
944f168
FIDO Conformance Tools v1.7.15 fixes
Oct 24, 2023
b700a2f
Json serialization fix
Oct 25, 2023
2b2382b
Unit test fix
Oct 25, 2023
13d2a3c
tokenbindig, AppId, UVP
googyi Dec 21, 2023
3432820
unit test fix (tokenbinding dto parsing)
googyi Dec 21, 2023
3113ee8
Merge branch 'master' into conformance-test-fixes
googyi Dec 21, 2023
c55c799
fix azure pipeline
googyi Dec 21, 2023
7534243
Improve trustanchor test coverage
googyi Jan 5, 2024
40a68e7
Merge branch 'master' into conformance-test-fixes
googyi Jan 5, 2024
cfaa1f9
TestPackedttestationAsyncFailTrustAnchorOnRootCertInTrustPath only wo…
googyi Jan 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Demo/TestController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Linq;
using System.Text;
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;

Expand Down Expand Up @@ -74,7 +75,7 @@ public JsonResult MakeCredentialOptionsTest([FromBody] TEST_MakeCredentialParams
var options = _fido2.RequestNewCredential(user, existingKeys, opts.AuthenticatorSelection, opts.Attestation, exts);

// 4. Temporarily store options, session/in-memory cache/redis/db
HttpContext.Session.SetString("fido2.attestationOptions", options.ToJson());
HttpContext.Session.SetString("fido2.attestationOptions", JsonSerializer.Serialize(options));

// 5. return options to client
return Json(options);
Expand Down Expand Up @@ -144,7 +145,7 @@ public IActionResult AssertionOptionsTest([FromBody] TEST_AssertionClientParams
);

// 4. Temporarily store options, session/in-memory cache/redis/db
HttpContext.Session.SetString("fido2.assertionOptions", options.ToJson());
HttpContext.Session.SetString("fido2.assertionOptions", JsonSerializer.Serialize(options));

// 5. Return options to client
return Json(options);
Expand Down
2 changes: 1 addition & 1 deletion Src/Fido2.Models/AuthenticatorAttestationRawResponse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public sealed class AuthenticatorAttestationRawResponse
public byte[] RawId { get; set; }

[JsonPropertyName("type")]
public PublicKeyCredentialType Type { get; set; } = PublicKeyCredentialType.PublicKey;
public PublicKeyCredentialType? Type { get; set; }

[JsonPropertyName("response")]
public ResponseData Response { get; set; }
Expand Down
4 changes: 3 additions & 1 deletion Src/Fido2.Models/CredentialCreateOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ public static CredentialCreateOptions Create(Fido2Configuration config, byte[] c
PubKeyCredParam.ES512,
PubKeyCredParam.RS512,
PubKeyCredParam.PS512,
PubKeyCredParam.RS1,
},
AuthenticatorSelection = authenticatorSelection,
Attestation = attestationConveyancePreference,
Expand Down Expand Up @@ -148,6 +149,7 @@ public PubKeyCredParam(COSE.Algorithm alg, PublicKeyCredentialType type = Public
public static readonly PubKeyCredParam PS384 = new(COSE.Algorithm.PS384);
public static readonly PubKeyCredParam PS512 = new(COSE.Algorithm.PS512);
public static readonly PubKeyCredParam Ed25519 = new(COSE.Algorithm.EdDSA);
public static readonly PubKeyCredParam RS1 = new(COSE.Algorithm.RS1);
}

/// <summary>
Expand Down Expand Up @@ -205,7 +207,7 @@ public class AuthenticatorSelection
[JsonPropertyName("residentKey")]
public ResidentKeyRequirement ResidentKey
{
get => _residentKey;
private get => _residentKey;
set
{
_residentKey = value;
Expand Down
9 changes: 7 additions & 2 deletions Src/Fido2.Models/Objects/AuthenticationExtensionsClientInputs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@ public sealed class AuthenticationExtensionsClientInputs
/// </summary>
[JsonPropertyName("appid")]
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public string AppID { get; set; }
public string AppID { private get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you put the getter private and add a method to retrieve the value of the app id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these private getters are used to prevent serialization in response. AppId and Uwm were not welcome by some asserts in the test tool.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here @aseigler -- Not sure about this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attribute [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] is there to avoid the prop to be serialized if unset. The private getter should not be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the conformance test tool, AppID should not be part of the response at all. There is a specific testcase for this.

Copy link
Contributor Author

@googyi googyi Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Fido guys are only slowly following the RFC, the token binding is a good example for this. I had to put it back to pass their tests.
In some places they don't even want to follow the RFC,
For example: googyi@84c9909

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a private branch with a bunch of tweaks that you are running this on? I don't see how you are possibly legitimately passing these without heavy changes and special workarounds. One of the fundamental issues with the way the tool works is the tests are a simple pass/fail -- which sounds simple on the surface, but for the failure case you actually have to be failing for the right reason, which you can only verify (in my experience) by menu -> open inspector and watching the responses to make sure they all line up with the appropriate test

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think everything is public in my fork and you should use the "net-standard-conversion" branch.
This is my latest commit in that branch:
image
Just check that you don't have this [Obsolete("Use property ResidentKey.")] stuff in the code.

I have ported the state of this .net core pull request and I readded your tokenbinding implementation.
And there was a lot work with asn stuff, serialization and cert verification, etc, but your old v2 version helped a lot in this.

"which you can only verify (in my experience) by menu -> open inspector"
I agree. So far I have not seen any testcase where it failed because of the wrong reason.
These tests have no big value, unless you want to get their certification. Then you have to do what they want.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That branch definitely looks like it is passing everything legitimately. Agree with your assessment in the value. I wouldn't mind trying to get the implementation certified if some organization wanted to sponsor it. I think we broke some of the FIDO tests implementing new WebAuthn features and moving away from the third party ASN.1 and CBOR libraries we were using.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed a few changes to this branch to reach 100%.
I don't remember any ASN.1 or CBOR related thing that broke anything.


public string GetAppID()
{
return AppID;
}

/// <summary>
/// This extension enables the WebAuthn Relying Party to determine which extensions the authenticator supports.
Expand All @@ -36,7 +41,7 @@ public sealed class AuthenticationExtensionsClientInputs
/// </summary>
[JsonPropertyName("uvm")]
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public bool? UserVerificationMethod { get; set; }
public bool? UserVerificationMethod { private get; set; }

#nullable enable
/// <summary>
Expand Down
21 changes: 12 additions & 9 deletions Src/Fido2/AuthenticatorAssertionResponse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,20 +118,23 @@ public async Task<VerifyAssertionResult> VerifyAsync(
// https://www.w3.org/TR/webauthn/#sctn-appid-extension
// FIDO AppID Extension:
// If true, the AppID was used and thus, when verifying an assertion, the Relying Party MUST expect the rpIdHash to be the hash of the AppID, not the RP ID.
var rpid = Raw.Extensions?.AppID ?? false ? options.Extensions?.AppID : options.RpId;
var rpid = Raw.Extensions?.AppID ?? false ? options.Extensions?.GetAppID() : options.RpId;
byte[] hashedRpId = SHA256.HashData(Encoding.UTF8.GetBytes(rpid ?? string.Empty));
byte[] hash = SHA256.HashData(Raw.Response.ClientDataJson);

if (!authData.RpIdHash.SequenceEqual(hashedRpId))
throw new Fido2VerificationException(Fido2ErrorCode.InvalidRpidHash, Fido2ErrorMessages.InvalidRpidHash);

// 14. Verify that the UP bit of the flags in authData is set.
if (!authData.UserPresent)
throw new Fido2VerificationException(Fido2ErrorCode.UserPresentFlagNotSet, Fido2ErrorMessages.UserPresentFlagNotSet);

// 15. If the Relying Party requires user verification for this assertion, verify that the UV bit of the flags in authData is set.
if (options.UserVerification is UserVerificationRequirement.Required && !authData.UserVerified)
throw new Fido2VerificationException(Fido2ErrorCode.UserVerificationRequirementNotMet, Fido2ErrorMessages.UserVerificationRequirementNotMet);
if (options.UserVerification is UserVerificationRequirement.Required)
{
// 14. Verify that the UP bit of the flags in authData is set.
if (!authData.UserPresent)
throw new Fido2VerificationException(Fido2ErrorCode.UserPresentFlagNotSet, Fido2ErrorMessages.UserPresentFlagNotSet);

// 15. If the Relying Party requires user verification for this assertion, verify that the UV bit of the flags in authData is set.
if (!authData.UserVerified)
throw new Fido2VerificationException(Fido2ErrorCode.UserVerificationRequirementNotMet, Fido2ErrorMessages.UserVerificationRequirementNotMet);
}

// 16. If the credential backup state is used as part of Relying Party business logic or policy, let currentBe and currentBs be the values of the BE and BS bits, respectively, of the flags in authData.
// Compare currentBe and currentBs with credentialRecord.BE and credentialRecord.BS and apply Relying Party policy, if any.
Expand Down Expand Up @@ -214,7 +217,7 @@ public async Task<VerifyAssertionResult> VerifyAsync(
if (metadataService?.ConformanceTesting() is true && metadataEntry is null && attType != AttestationType.None && fmt is not "fido-u2f")
throw new Fido2VerificationException(Fido2ErrorCode.AaGuidNotFound, "AAGUID not found in MDS test metadata");

TrustAnchor.Verify(metadataEntry, trustPath);
TrustAnchor.Verify(metadataEntry, trustPath, metadataService?.ConformanceTesting() is true);
}

return new VerifyAssertionResult
Expand Down
6 changes: 3 additions & 3 deletions Src/Fido2/AuthenticatorAttestationResponse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public async Task<RegisteredPublicKeyCredential> VerifyAsync(
if (metadataService?.ConformanceTesting() is true && metadataEntry is null && attType != AttestationType.None && AttestationObject.Fmt is not "fido-u2f")
throw new Fido2VerificationException(Fido2ErrorCode.AaGuidNotFound, "AAGUID not found in MDS test metadata");

TrustAnchor.Verify(metadataEntry, trustPath);
TrustAnchor.Verify(metadataEntry, trustPath, metadataService?.ConformanceTesting() is true);

// 22. Assess the attestation trustworthiness using the outputs of the verification procedure in step 14, as follows:
// If self attestation was used, check if self attestation is acceptable under Relying Party policy.
Expand Down Expand Up @@ -186,7 +186,7 @@ public async Task<RegisteredPublicKeyCredential> VerifyAsync(

return new RegisteredPublicKeyCredential
{
Type = Raw.Type,
Type = Raw.Type.Value,
Id = authData.AttestedCredentialData.CredentialId,
PublicKey = authData.AttestedCredentialData.CredentialPublicKey.GetBytes(),
SignCount = authData.SignCount,
Expand Down Expand Up @@ -250,7 +250,7 @@ private async Task<byte[]> DevicePublicKeyRegistrationAsync(
if (metadataService?.ConformanceTesting() is true && metadataEntry is null && attType != AttestationType.None && devicePublicKeyAuthenticatorOutput.Fmt is not "fido-u2f")
throw new Fido2VerificationException(Fido2ErrorCode.AaGuidNotFound, "AAGUID not found in MDS test metadata");

TrustAnchor.Verify(metadataEntry, trustPath);
TrustAnchor.Verify(metadataEntry, trustPath, metadataService?.ConformanceTesting() is true);

// Check status reports for authenticator with undesirable status
var latestStatusReport = metadataEntry?.GetLatestStatusReport();
Expand Down
15 changes: 8 additions & 7 deletions Src/Fido2/Extensions/CryptoUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public static HashAlgorithmName HashAlgFromCOSEAlg(COSE.Algorithm alg)
};
}

public static bool ValidateTrustChain(X509Certificate2[] trustPath, X509Certificate2[] attestationRootCertificates)
public static bool ValidateTrustChain(X509Certificate2[] trustPath, X509Certificate2[] attestationRootCertificates, bool conformance = false)
{
// https://fidoalliance.org/specs/fido-v2.0-id-20180227/fido-metadata-statement-v2.0-id-20180227.html#widl-MetadataStatement-attestationRootCertificates

Expand All @@ -59,7 +59,9 @@ public static bool ValidateTrustChain(X509Certificate2[] trustPath, X509Certific
// A trust anchor can be a root certificate, an intermediate CA certificate or even the attestation certificate itself.

// Let's check the simplest case first. If subject and issuer are the same, and the attestation cert is in the list, that's all the validation we need
if (trustPath.Length == 1 && trustPath[0].Subject.Equals(trustPath[0].Issuer, StringComparison.Ordinal))

// We have the same singular root cert in trustpath and it is in attestationRootCertificates
if (trustPath.Length == 1)
{
foreach (X509Certificate2 cert in attestationRootCertificates)
{
Expand All @@ -68,7 +70,6 @@ public static bool ValidateTrustChain(X509Certificate2[] trustPath, X509Certific
return true;
}
}
return false;
}

// If the attestation cert is not self signed, we will need to build a chain
Expand Down Expand Up @@ -101,10 +102,10 @@ public static bool ValidateTrustChain(X509Certificate2[] trustPath, X509Certific
chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust;

// if the attestation cert has a CDP extension, go ahead and turn on online revocation checking
if (!string.IsNullOrEmpty(CDPFromCertificateExts(trustPath[0].Extensions)))
chain.ChainPolicy.RevocationMode = X509RevocationMode.Online;

// don't allow unknown root now that we have a custom root
if (!string.IsNullOrEmpty(CDPFromCertificateExts(trustPath[0].Extensions)) && !conformance)
chain.ChainPolicy.RevocationMode = X509RevocationMode.Online;
// don't allow unknown root now that we have a custom root
chain.ChainPolicy.VerificationFlags = X509VerificationFlags.NoFlag;

// now, verify chain again with all checks turned on
Expand Down
9 changes: 7 additions & 2 deletions Src/Fido2/TrustAnchor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace Fido2NetLib;

public static class TrustAnchor
{
public static void Verify(MetadataBLOBPayloadEntry? metadataEntry, X509Certificate2[] trustPath)
public static void Verify(MetadataBLOBPayloadEntry? metadataEntry, X509Certificate2[] trustPath, bool conformance)
{
if (trustPath != null && metadataEntry?.MetadataStatement?.AttestationTypes is not null)
{
Expand All @@ -29,7 +29,12 @@ static bool ContainsAttestationType(MetadataBLOBPayloadEntry entry, MetadataAtte
attestationRootCertificates[i] = new X509Certificate2(Convert.FromBase64String(certStrings[i]));
}

if (!CryptoUtils.ValidateTrustChain(trustPath, attestationRootCertificates))
if (trustPath.Length > 1 && attestationRootCertificates.Any(c => string.Equals(c.Thumbprint, trustPath[^1].Thumbprint, StringComparison.Ordinal)))
{
throw new Fido2VerificationException(Fido2ErrorMessages.InvalidCertificateChain);
}

if (!CryptoUtils.ValidateTrustChain(trustPath, attestationRootCertificates, conformance))
{
throw new Fido2VerificationException(Fido2ErrorMessages.InvalidCertificateChain);
}
Expand Down