From ef467332957e663b70ce9f01f2ed68846f806d19 Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Mon, 4 Mar 2024 18:03:33 -0500 Subject: [PATCH 1/6] Wrapped calls to MakeNewCredentialAsync in try/catch --- .../CreateWebAuthnLoginCredentialCommand.cs | 25 +++++++++++++------ .../Services/Implementations/UserService.cs | 20 +++++++++------ 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/CreateWebAuthnLoginCredentialCommand.cs b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/CreateWebAuthnLoginCredentialCommand.cs index 65c98dea3b63..9d77a19da312 100644 --- a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/CreateWebAuthnLoginCredentialCommand.cs +++ b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/CreateWebAuthnLoginCredentialCommand.cs @@ -3,6 +3,7 @@ using Bit.Core.Entities; using Bit.Core.Utilities; using Fido2NetLib; +using Microsoft.Extensions.Logging; namespace Bit.Core.Auth.UserFeatures.WebAuthnLogin.Implementations; @@ -12,11 +13,15 @@ internal class CreateWebAuthnLoginCredentialCommand : ICreateWebAuthnLoginCreden private readonly IFido2 _fido2; private readonly IWebAuthnCredentialRepository _webAuthnCredentialRepository; + private readonly ILogger _logger; - public CreateWebAuthnLoginCredentialCommand(IFido2 fido2, IWebAuthnCredentialRepository webAuthnCredentialRepository) + public CreateWebAuthnLoginCredentialCommand(IFido2 fido2, + IWebAuthnCredentialRepository webAuthnCredentialRepository, + ILogger logger) { _fido2 = fido2; _webAuthnCredentialRepository = webAuthnCredentialRepository; + _logger = logger; } public async Task CreateWebAuthnLoginCredentialAsync(User user, string name, CredentialCreateOptions options, AuthenticatorAttestationRawResponse attestationResponse, bool supportsPrf, string encryptedUserKey = null, string encryptedPublicKey = null, string encryptedPrivateKey = null) @@ -30,16 +35,22 @@ public async Task 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, diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index a40d4bf302ef..6930f9551a29 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -487,19 +487,25 @@ public async Task 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(); From 1d7b3f6040e4222e0e6cf1b26ee06d056f13f5dc Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Mon, 4 Mar 2024 18:25:36 -0500 Subject: [PATCH 2/6] Added exception handling for missed MakeAssertionAsync() --- .../AssertWebAuthnLoginCredentialCommand.cs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs index 88e5ec32b2f9..f02c8f1fe479 100644 --- a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs +++ b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs @@ -6,6 +6,7 @@ using Bit.Core.Repositories; using Bit.Core.Utilities; using Fido2NetLib; +using Microsoft.Extensions.Logging; namespace Bit.Core.Auth.UserFeatures.WebAuthnLogin.Implementations; @@ -14,6 +15,7 @@ internal class AssertWebAuthnLoginCredentialCommand : IAssertWebAuthnLoginCreden private readonly IFido2 _fido2; private readonly IWebAuthnCredentialRepository _webAuthnCredentialRepository; private readonly IUserRepository _userRepository; + private readonly ILogger _logger; public AssertWebAuthnLoginCredentialCommand(IFido2 fido2, IWebAuthnCredentialRepository webAuthnCredentialRepository, IUserRepository userRepository) { @@ -46,8 +48,14 @@ public AssertWebAuthnLoginCredentialCommand(IFido2 fido2, IWebAuthnCredentialRep // 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) { + throw new BadRequestException("Unable to verify credential."); + } // Update SignatureCounter credential.Counter = (int)assertionVerificationResult.Counter; From 06ef83d62ede99303a3d2b97c66f25a61262d514 Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Mon, 4 Mar 2024 18:38:33 -0500 Subject: [PATCH 3/6] Removed logger that wasn't used. --- .../Implementations/AssertWebAuthnLoginCredentialCommand.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs index f02c8f1fe479..e46b5231d9bb 100644 --- a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs +++ b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs @@ -6,7 +6,6 @@ using Bit.Core.Repositories; using Bit.Core.Utilities; using Fido2NetLib; -using Microsoft.Extensions.Logging; namespace Bit.Core.Auth.UserFeatures.WebAuthnLogin.Implementations; @@ -15,7 +14,6 @@ internal class AssertWebAuthnLoginCredentialCommand : IAssertWebAuthnLoginCreden private readonly IFido2 _fido2; private readonly IWebAuthnCredentialRepository _webAuthnCredentialRepository; private readonly IUserRepository _userRepository; - private readonly ILogger _logger; public AssertWebAuthnLoginCredentialCommand(IFido2 fido2, IWebAuthnCredentialRepository webAuthnCredentialRepository, IUserRepository userRepository) { From b97a4d38aca934835604e03319814581a844dd8e Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Mon, 4 Mar 2024 18:42:24 -0500 Subject: [PATCH 4/6] Linting. --- .../AssertWebAuthnLoginCredentialCommand.cs | 9 ++++++--- .../CreateWebAuthnLoginCredentialCommand.cs | 9 ++++++--- src/Core/Services/Implementations/UserService.cs | 7 +++++-- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs index e46b5231d9bb..8ce13f14c82f 100644 --- a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs +++ b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs @@ -46,12 +46,15 @@ public AssertWebAuthnLoginCredentialCommand(IFido2 fido2, IWebAuthnCredentialRep // 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); - + Fido2NetLib.Objects.AssertionVerificationResult assertionVerificationResult = null; - try { + try + { assertionVerificationResult = await _fido2.MakeAssertionAsync( assertionResponse, options, credentialPublicKey, (uint)credential.Counter, callback); - } catch (Fido2VerificationException) { + } + catch (Fido2VerificationException) + { throw new BadRequestException("Unable to verify credential."); } diff --git a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/CreateWebAuthnLoginCredentialCommand.cs b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/CreateWebAuthnLoginCredentialCommand.cs index 9d77a19da312..15d1abcf8928 100644 --- a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/CreateWebAuthnLoginCredentialCommand.cs +++ b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/CreateWebAuthnLoginCredentialCommand.cs @@ -15,7 +15,7 @@ internal class CreateWebAuthnLoginCredentialCommand : ICreateWebAuthnLoginCreden private readonly IWebAuthnCredentialRepository _webAuthnCredentialRepository; private readonly ILogger _logger; - public CreateWebAuthnLoginCredentialCommand(IFido2 fido2, + public CreateWebAuthnLoginCredentialCommand(IFido2 fido2, IWebAuthnCredentialRepository webAuthnCredentialRepository, ILogger logger) { @@ -36,9 +36,12 @@ public async Task CreateWebAuthnLoginCredentialAsync(User user, string nam IsCredentialIdUniqueToUserAsyncDelegate callback = (args, cancellationToken) => Task.FromResult(!existingCredentialIds.Contains(CoreHelpers.Base64UrlEncode(args.CredentialId))); Fido2.CredentialMakeResult credentialResponse = null; - try { + try + { credentialResponse = await _fido2.MakeNewCredentialAsync(attestationResponse, options, callback); - } catch (Fido2VerificationException e) { + } + catch (Fido2VerificationException e) + { _logger.LogError(e, "Unable to verify WebAuthn credential."); return false; } diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index 6930f9551a29..c1db9835a9c6 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -488,9 +488,12 @@ public async Task CompleteWebAuthRegistrationAsync(User user, int id, stri IsCredentialIdUniqueToUserAsyncDelegate callback = (args, cancellationToken) => Task.FromResult(true); Fido2.CredentialMakeResult credentialResponse = null; - try { + try + { credentialResponse = await _fido2.MakeNewCredentialAsync(attestationResponse, options, callback); - } catch (Fido2VerificationException e) { + } + catch (Fido2VerificationException e) + { base.Logger.LogError(e, "Unable to verify WebAuthn credential."); return false; } From bd46adce7727750045f31dda692f0caf6d5b50e2 Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Tue, 14 May 2024 14:40:55 -0400 Subject: [PATCH 5/6] Updated message to prevent enumeration. --- .../AssertWebAuthnLoginCredentialCommand.cs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs index 8ce13f14c82f..2764c8ebb54d 100644 --- a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs +++ b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/AssertWebAuthnLoginCredentialCommand.cs @@ -26,13 +26,13 @@ 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); @@ -40,7 +40,7 @@ public AssertWebAuthnLoginCredentialCommand(IFido2 fido2, IWebAuthnCredentialRep 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 @@ -55,7 +55,7 @@ public AssertWebAuthnLoginCredentialCommand(IFido2 fido2, IWebAuthnCredentialRep } catch (Fido2VerificationException) { - throw new BadRequestException("Unable to verify credential."); + ThrowInvalidCredentialException(); } // Update SignatureCounter @@ -64,9 +64,14 @@ public AssertWebAuthnLoginCredentialCommand(IFido2 fido2, IWebAuthnCredentialRep if (assertionVerificationResult.Status != "ok") { - throw new BadRequestException("Invalid credential."); + ThrowInvalidCredentialException(); } return (user, credential); } + + private void ThrowInvalidCredentialException() + { + throw new BadRequestException("Invalid credential."); + } } From 6c2f8182e81787c5c2f0988462580471fdfdc4d4 Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Tue, 14 May 2024 14:53:34 -0400 Subject: [PATCH 6/6] Updated command to be public for testing. --- .../Implementations/CreateWebAuthnLoginCredentialCommand.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/CreateWebAuthnLoginCredentialCommand.cs b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/CreateWebAuthnLoginCredentialCommand.cs index 15d1abcf8928..5eebc8b7e20e 100644 --- a/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/CreateWebAuthnLoginCredentialCommand.cs +++ b/src/Core/Auth/UserFeatures/WebAuthnLogin/Implementations/CreateWebAuthnLoginCredentialCommand.cs @@ -7,7 +7,7 @@ namespace Bit.Core.Auth.UserFeatures.WebAuthnLogin.Implementations; -internal class CreateWebAuthnLoginCredentialCommand : ICreateWebAuthnLoginCredentialCommand +public class CreateWebAuthnLoginCredentialCommand : ICreateWebAuthnLoginCredentialCommand { public const int MaxCredentialsPerUser = 5;