Skip to content
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.

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

[PM-6631] Handle Fido2VerificationException during passkey attestation and assertion #59

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -26,38 +26,52 @@ public AssertWebAuthnLoginCredentialCommand(IFido2 fido2, IWebAuthnCredentialRep
{
if (!GuidUtilities.TryParseBytes(assertionResponse.Response.UserHandle, out var userId))
{
throw new BadRequestException("Invalid credential.");
ThrowInvalidCredentialException();
}

var user = await _userRepository.GetByIdAsync(userId);
if (user == null)
{
throw new BadRequestException("Invalid credential.");
ThrowInvalidCredentialException();
}

var userCredentials = await _webAuthnCredentialRepository.GetManyByUserIdAsync(user.Id);
var assertedCredentialId = CoreHelpers.Base64UrlEncode(assertionResponse.Id);
var credential = userCredentials.FirstOrDefault(c => c.CredentialId == assertedCredentialId);
if (credential == null)
{
throw new BadRequestException("Invalid credential.");
ThrowInvalidCredentialException();
}

// Always return true, since we've already filtered the credentials after user id
IsUserHandleOwnerOfCredentialIdAsync callback = (args, cancellationToken) => Task.FromResult(true);
var credentialPublicKey = CoreHelpers.Base64UrlDecode(credential.PublicKey);
var assertionVerificationResult = await _fido2.MakeAssertionAsync(
assertionResponse, options, credentialPublicKey, (uint)credential.Counter, callback);

Fido2NetLib.Objects.AssertionVerificationResult assertionVerificationResult = null;
try
{
assertionVerificationResult = await _fido2.MakeAssertionAsync(
assertionResponse, options, credentialPublicKey, (uint)credential.Counter, callback);
}
catch (Fido2VerificationException)
{
ThrowInvalidCredentialException();
}
Comment on lines +50 to +59
Copy link

Choose a reason for hiding this comment

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

style: The catch block swallows the specific exception details. Consider logging the exception message or type for better diagnostics.


// Update SignatureCounter
credential.Counter = (int)assertionVerificationResult.Counter;
await _webAuthnCredentialRepository.ReplaceAsync(credential);
Comment on lines 62 to 63
Copy link

Choose a reason for hiding this comment

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

logic: Ensure that assertionVerificationResult is not null before accessing its properties.


if (assertionVerificationResult.Status != "ok")
{
throw new BadRequestException("Invalid credential.");
ThrowInvalidCredentialException();
}

return (user, credential);
}

private void ThrowInvalidCredentialException()
{
throw new BadRequestException("Invalid credential.");
}
Comment on lines +73 to +76
Copy link

Choose a reason for hiding this comment

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

style: Consider making this method more flexible by allowing custom error messages or including more context in the exception.

}
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,25 @@
using Bit.Core.Entities;
using Bit.Core.Utilities;
using Fido2NetLib;
using Microsoft.Extensions.Logging;

namespace Bit.Core.Auth.UserFeatures.WebAuthnLogin.Implementations;

internal class CreateWebAuthnLoginCredentialCommand : ICreateWebAuthnLoginCredentialCommand
public class CreateWebAuthnLoginCredentialCommand : ICreateWebAuthnLoginCredentialCommand
{
public const int MaxCredentialsPerUser = 5;

private readonly IFido2 _fido2;
private readonly IWebAuthnCredentialRepository _webAuthnCredentialRepository;
private readonly ILogger<CreateWebAuthnLoginCredentialCommand> _logger;

public CreateWebAuthnLoginCredentialCommand(IFido2 fido2, IWebAuthnCredentialRepository webAuthnCredentialRepository)
public CreateWebAuthnLoginCredentialCommand(IFido2 fido2,
IWebAuthnCredentialRepository webAuthnCredentialRepository,
ILogger<CreateWebAuthnLoginCredentialCommand> logger)
{
_fido2 = fido2;
_webAuthnCredentialRepository = webAuthnCredentialRepository;
_logger = logger;
}

public async Task<bool> CreateWebAuthnLoginCredentialAsync(User user, string name, CredentialCreateOptions options, AuthenticatorAttestationRawResponse attestationResponse, bool supportsPrf, string encryptedUserKey = null, string encryptedPublicKey = null, string encryptedPrivateKey = null)
Expand All @@ -30,16 +35,25 @@ public async Task<bool> CreateWebAuthnLoginCredentialAsync(User user, string nam
var existingCredentialIds = existingCredentials.Select(c => c.CredentialId);
IsCredentialIdUniqueToUserAsyncDelegate callback = (args, cancellationToken) => Task.FromResult(!existingCredentialIds.Contains(CoreHelpers.Base64UrlEncode(args.CredentialId)));

var success = await _fido2.MakeNewCredentialAsync(attestationResponse, options, callback);
Fido2.CredentialMakeResult credentialResponse = null;
try
{
credentialResponse = await _fido2.MakeNewCredentialAsync(attestationResponse, options, callback);
}
catch (Fido2VerificationException e)
{
_logger.LogError(e, "Unable to verify WebAuthn credential.");
return false;
}

var credential = new WebAuthnCredential
{
Name = name,
CredentialId = CoreHelpers.Base64UrlEncode(success.Result.CredentialId),
PublicKey = CoreHelpers.Base64UrlEncode(success.Result.PublicKey),
Type = success.Result.CredType,
AaGuid = success.Result.Aaguid,
Counter = (int)success.Result.Counter,
CredentialId = CoreHelpers.Base64UrlEncode(credentialResponse.Result.CredentialId),
PublicKey = CoreHelpers.Base64UrlEncode(credentialResponse.Result.PublicKey),
Type = credentialResponse.Result.CredType,
AaGuid = credentialResponse.Result.Aaguid,
Counter = (int)credentialResponse.Result.Counter,
UserId = user.Id,
SupportsPrf = supportsPrf,
EncryptedUserKey = encryptedUserKey,
Expand Down
23 changes: 16 additions & 7 deletions src/Core/Services/Implementations/UserService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -407,19 +407,28 @@ public async Task<bool> CompleteWebAuthRegistrationAsync(User user, int id, stri
// account uses the same 2FA key.
IsCredentialIdUniqueToUserAsyncDelegate callback = (args, cancellationToken) => Task.FromResult(true);

var success = await _fido2.MakeNewCredentialAsync(attestationResponse, options, callback);
Fido2.CredentialMakeResult credentialResponse = null;
try
{
credentialResponse = await _fido2.MakeNewCredentialAsync(attestationResponse, options, callback);
}
catch (Fido2VerificationException e)
{
base.Logger.LogError(e, "Unable to verify WebAuthn credential.");
return false;
}

provider.MetaData.Remove("pending");
provider.MetaData[keyId] = new TwoFactorProvider.WebAuthnData
{
Name = name,
Descriptor = new PublicKeyCredentialDescriptor(success.Result.CredentialId),
PublicKey = success.Result.PublicKey,
UserHandle = success.Result.User.Id,
SignatureCounter = success.Result.Counter,
CredType = success.Result.CredType,
Descriptor = new PublicKeyCredentialDescriptor(credentialResponse.Result.CredentialId),
PublicKey = credentialResponse.Result.PublicKey,
UserHandle = credentialResponse.Result.User.Id,
SignatureCounter = credentialResponse.Result.Counter,
CredType = credentialResponse.Result.CredType,
RegDate = DateTime.Now,
AaGuid = success.Result.Aaguid
AaGuid = credentialResponse.Result.Aaguid
};

var providers = user.GetTwoFactorProviders();
Expand Down
Loading