From eedc96263a680e36243e44d6bebd779f296f3245 Mon Sep 17 00:00:00 2001 From: Oscar Hinton Date: Tue, 5 Dec 2023 17:21:46 +0100 Subject: [PATCH] [PM-3565] Enforce higher minimum KDF (#3304) Extract KDF logic into a new Range class. Increase minimum iterations for PBKDF. --- .../SetKeyConnectorKeyRequestModel.cs | 8 +-- .../Accounts/SetPasswordRequestModel.cs | 8 +-- src/Api/Controllers/AccountsController.cs | 2 +- src/Core/Constants.cs | 39 +++++++++++ src/Core/Utilities/KdfSettingsValidator.cs | 16 ++--- .../Controllers/AccountsController.cs | 2 +- .../Controllers/AccountsControllerTests.cs | 5 +- .../Request/Accounts/KdfRequestModelTests.cs | 10 +-- test/Core.Test/ConstantsTests.cs | 69 +++++++++++++++++++ .../Controllers/AccountsControllerTests.cs | 7 +- 10 files changed, 132 insertions(+), 34 deletions(-) create mode 100644 test/Core.Test/ConstantsTests.cs diff --git a/src/Api/Auth/Models/Request/Accounts/SetKeyConnectorKeyRequestModel.cs b/src/Api/Auth/Models/Request/Accounts/SetKeyConnectorKeyRequestModel.cs index 68cc15d42..25d543b91 100644 --- a/src/Api/Auth/Models/Request/Accounts/SetKeyConnectorKeyRequestModel.cs +++ b/src/Api/Auth/Models/Request/Accounts/SetKeyConnectorKeyRequestModel.cs @@ -2,11 +2,10 @@ using Bit.Core.Auth.Models.Api.Request.Accounts; using Bit.Core.Entities; using Bit.Core.Enums; -using Bit.Core.Utilities; namespace Bit.Api.Auth.Models.Request.Accounts; -public class SetKeyConnectorKeyRequestModel : IValidatableObject +public class SetKeyConnectorKeyRequestModel { [Required] public string Key { get; set; } @@ -31,9 +30,4 @@ public class SetKeyConnectorKeyRequestModel : IValidatableObject Keys.ToUser(existingUser); return existingUser; } - - public IEnumerable Validate(ValidationContext validationContext) - { - return KdfSettingsValidator.Validate(Kdf, KdfIterations, KdfMemory, KdfParallelism); - } } diff --git a/src/Api/Auth/Models/Request/Accounts/SetPasswordRequestModel.cs b/src/Api/Auth/Models/Request/Accounts/SetPasswordRequestModel.cs index 3dd43aa5e..b07c7ea81 100644 --- a/src/Api/Auth/Models/Request/Accounts/SetPasswordRequestModel.cs +++ b/src/Api/Auth/Models/Request/Accounts/SetPasswordRequestModel.cs @@ -2,11 +2,10 @@ using Bit.Core.Auth.Models.Api.Request.Accounts; using Bit.Core.Entities; using Bit.Core.Enums; -using Bit.Core.Utilities; namespace Bit.Api.Auth.Models.Request.Accounts; -public class SetPasswordRequestModel : IValidatableObject +public class SetPasswordRequestModel { [Required] [StringLength(300)] @@ -35,9 +34,4 @@ public class SetPasswordRequestModel : IValidatableObject Keys?.ToUser(existingUser); return existingUser; } - - public IEnumerable Validate(ValidationContext validationContext) - { - return KdfSettingsValidator.Validate(Kdf, KdfIterations, KdfMemory, KdfParallelism); - } } diff --git a/src/Api/Controllers/AccountsController.cs b/src/Api/Controllers/AccountsController.cs index 7217afd0b..8cffe3f39 100644 --- a/src/Api/Controllers/AccountsController.cs +++ b/src/Api/Controllers/AccountsController.cs @@ -113,7 +113,7 @@ public class AccountsController : Controller kdfInformation = new UserKdfInformation { Kdf = KdfType.PBKDF2_SHA256, - KdfIterations = 100000, + KdfIterations = AuthConstants.PBKDF2_ITERATIONS.Default, }; } return new PreloginResponseModel(kdfInformation); diff --git a/src/Core/Constants.cs b/src/Core/Constants.cs index 492d4fdd9..706d6858a 100644 --- a/src/Core/Constants.cs +++ b/src/Core/Constants.cs @@ -31,6 +31,45 @@ public static class Constants public const string IdentityProvider = "bitwarden"; } +public static class AuthConstants +{ + public static readonly RangeConstant PBKDF2_ITERATIONS = new(600_000, 2_000_000, 600_000); + + public static readonly RangeConstant ARGON2_ITERATIONS = new(2, 10, 3); + public static readonly RangeConstant ARGON2_MEMORY = new(15, 1024, 64); + public static readonly RangeConstant ARGON2_PARALLELISM = new(1, 16, 4); + +} + +public class RangeConstant +{ + public int Default { get; } + public int Min { get; } + public int Max { get; } + + public RangeConstant(int min, int max, int defaultValue) + { + Default = defaultValue; + Min = min; + Max = max; + + if (Min > Max) + { + throw new ArgumentOutOfRangeException($"{Min} is larger than {Max}."); + } + + if (!InsideRange(defaultValue)) + { + throw new ArgumentOutOfRangeException($"{Default} is outside allowed range of {Min}-{Max}."); + } + } + + public bool InsideRange(int number) + { + return Min <= number && number <= Max; + } +} + public static class TokenPurposes { public const string LinkSso = "LinkSso"; diff --git a/src/Core/Utilities/KdfSettingsValidator.cs b/src/Core/Utilities/KdfSettingsValidator.cs index c47431d7d..db7936acf 100644 --- a/src/Core/Utilities/KdfSettingsValidator.cs +++ b/src/Core/Utilities/KdfSettingsValidator.cs @@ -10,23 +10,23 @@ public static class KdfSettingsValidator switch (kdfType) { case KdfType.PBKDF2_SHA256: - if (kdfIterations < 5000 || kdfIterations > 2_000_000) + if (!AuthConstants.PBKDF2_ITERATIONS.InsideRange(kdfIterations)) { - yield return new ValidationResult("KDF iterations must be between 5000 and 2000000."); + yield return new ValidationResult($"KDF iterations must be between {AuthConstants.PBKDF2_ITERATIONS.Min} and {AuthConstants.PBKDF2_ITERATIONS.Max}."); } break; case KdfType.Argon2id: - if (kdfIterations <= 0) + if (!AuthConstants.ARGON2_ITERATIONS.InsideRange(kdfIterations)) { - yield return new ValidationResult("Argon2 iterations must be greater than 0."); + yield return new ValidationResult($"Argon2 iterations must be between {AuthConstants.ARGON2_ITERATIONS.Min} and {AuthConstants.ARGON2_ITERATIONS.Max}."); } - else if (!kdfMemory.HasValue || kdfMemory.Value < 15 || kdfMemory.Value > 1024) + else if (!kdfMemory.HasValue || !AuthConstants.ARGON2_MEMORY.InsideRange(kdfMemory.Value)) { - yield return new ValidationResult("Argon2 memory must be between 15mb and 1024mb."); + yield return new ValidationResult($"Argon2 memory must be between {AuthConstants.ARGON2_MEMORY.Min}mb and {AuthConstants.ARGON2_MEMORY.Max}mb."); } - else if (!kdfParallelism.HasValue || kdfParallelism.Value < 1 || kdfParallelism.Value > 16) + else if (!kdfParallelism.HasValue || !AuthConstants.ARGON2_PARALLELISM.InsideRange(kdfParallelism.Value)) { - yield return new ValidationResult("Argon2 parallelism must be between 1 and 16."); + yield return new ValidationResult($"Argon2 parallelism must be between {AuthConstants.ARGON2_PARALLELISM.Min} and {AuthConstants.ARGON2_PARALLELISM.Max}."); } break; diff --git a/src/Identity/Controllers/AccountsController.cs b/src/Identity/Controllers/AccountsController.cs index 4abbbab48..9469673e1 100644 --- a/src/Identity/Controllers/AccountsController.cs +++ b/src/Identity/Controllers/AccountsController.cs @@ -75,7 +75,7 @@ public class AccountsController : Controller kdfInformation = new UserKdfInformation { Kdf = KdfType.PBKDF2_SHA256, - KdfIterations = 100000, + KdfIterations = AuthConstants.PBKDF2_ITERATIONS.Default, }; } return new PreloginResponseModel(kdfInformation); diff --git a/test/Api.Test/Controllers/AccountsControllerTests.cs b/test/Api.Test/Controllers/AccountsControllerTests.cs index d6e8cacbe..c0c50345a 100644 --- a/test/Api.Test/Controllers/AccountsControllerTests.cs +++ b/test/Api.Test/Controllers/AccountsControllerTests.cs @@ -1,6 +1,7 @@ using System.Security.Claims; using Bit.Api.Auth.Models.Request.Accounts; using Bit.Api.Controllers; +using Bit.Core; using Bit.Core.AdminConsole.Repositories; using Bit.Core.AdminConsole.Services; using Bit.Core.Auth.Models.Api.Request.Accounts; @@ -111,14 +112,14 @@ public class AccountsControllerTests : IDisposable } [Fact] - public async Task PostPrelogin_WhenUserDoesNotExist_ShouldDefaultToSha256And100000Iterations() + public async Task PostPrelogin_WhenUserDoesNotExist_ShouldDefaultToPBKDF() { _userRepository.GetKdfInformationByEmailAsync(Arg.Any()).Returns(Task.FromResult((UserKdfInformation)null)); var response = await _sut.PostPrelogin(new PreloginRequestModel { Email = "user@example.com" }); Assert.Equal(KdfType.PBKDF2_SHA256, response.Kdf); - Assert.Equal(100000, response.KdfIterations); + Assert.Equal(AuthConstants.PBKDF2_ITERATIONS.Default, response.KdfIterations); } [Fact] diff --git a/test/Api.Test/Models/Request/Accounts/KdfRequestModelTests.cs b/test/Api.Test/Models/Request/Accounts/KdfRequestModelTests.cs index 0edd5a96d..612b7ad44 100644 --- a/test/Api.Test/Models/Request/Accounts/KdfRequestModelTests.cs +++ b/test/Api.Test/Models/Request/Accounts/KdfRequestModelTests.cs @@ -9,11 +9,11 @@ public class KdfRequestModelTests { [Theory] [InlineData(KdfType.PBKDF2_SHA256, 1_000_000, null, null)] // Somewhere in the middle - [InlineData(KdfType.PBKDF2_SHA256, 5000, null, null)] // Right on the lower boundary + [InlineData(KdfType.PBKDF2_SHA256, 600_000, null, null)] // Right on the lower boundary [InlineData(KdfType.PBKDF2_SHA256, 2_000_000, null, null)] // Right on the upper boundary - [InlineData(KdfType.Argon2id, 10, 500, 8)] // Somewhere in the middle - [InlineData(KdfType.Argon2id, 1, 15, 1)] // Right on the lower boundary - [InlineData(KdfType.Argon2id, 5000, 1024, 16)] // Right on the upper boundary + [InlineData(KdfType.Argon2id, 5, 500, 8)] // Somewhere in the middle + [InlineData(KdfType.Argon2id, 2, 15, 1)] // Right on the lower boundary + [InlineData(KdfType.Argon2id, 10, 1024, 16)] // Right on the upper boundary public void Validate_IsValid(KdfType kdfType, int? kdfIterations, int? kdfMemory, int? kdfParallelism) { var model = new KdfRequestModel @@ -32,7 +32,7 @@ public class KdfRequestModelTests [Theory] [InlineData(null, 350_000, null, null, 1)] // Although KdfType is nullable, it's marked as [Required] - [InlineData(KdfType.PBKDF2_SHA256, 1000, null, null, 1)] // Too few iterations + [InlineData(KdfType.PBKDF2_SHA256, 500_000, null, null, 1)] // Too few iterations [InlineData(KdfType.PBKDF2_SHA256, 2_000_001, null, null, 1)] // Too many iterations [InlineData(KdfType.Argon2id, 0, 30, 8, 1)] // Iterations must be greater than 0 [InlineData(KdfType.Argon2id, 10, 14, 8, 1)] // Too little memory diff --git a/test/Core.Test/ConstantsTests.cs b/test/Core.Test/ConstantsTests.cs new file mode 100644 index 000000000..5be5bbef1 --- /dev/null +++ b/test/Core.Test/ConstantsTests.cs @@ -0,0 +1,69 @@ +using Xunit; + +namespace Bit.Core.Test; + +public class ConstantsTests +{ + public class RangeConstantTests + { + [Fact] + public void Constructor_WithValidValues_SetsProperties() + { + // Arrange + const int min = 0; + const int max = 10; + const int defaultValue = 5; + + // Act + var rangeConstant = new RangeConstant(min, max, defaultValue); + + // Assert + Assert.Equal(min, rangeConstant.Min); + Assert.Equal(max, rangeConstant.Max); + Assert.Equal(defaultValue, rangeConstant.Default); + } + + [Fact] + public void Constructor_WithInvalidValues_ThrowsArgumentOutOfRangeException() + { + Assert.Throws(() => new RangeConstant(10, 0, 5)); + } + + [Fact] + public void Constructor_WithDefaultValueOutsideRange_ThrowsArgumentOutOfRangeException() + { + Assert.Throws(() => new RangeConstant(0, 10, 20)); + } + + [Theory] + [InlineData(5)] + [InlineData(0)] + [InlineData(10)] + public void InsideRange_WithValidValues_ReturnsTrue(int number) + { + // Arrange + var rangeConstant = new RangeConstant(0, 10, 5); + + // Act + bool result = rangeConstant.InsideRange(number); + + // Assert + Assert.True(result); + } + + [Theory] + [InlineData(-1)] + [InlineData(11)] + public void InsideRange_WithInvalidValues_ReturnsFalse(int number) + { + // Arrange + var rangeConstant = new RangeConstant(0, 10, 5); + + // Act + bool result = rangeConstant.InsideRange(number); + + // Assert + Assert.False(result); + } + } +} diff --git a/test/Identity.Test/Controllers/AccountsControllerTests.cs b/test/Identity.Test/Controllers/AccountsControllerTests.cs index 3d94fd54b..4690583f7 100644 --- a/test/Identity.Test/Controllers/AccountsControllerTests.cs +++ b/test/Identity.Test/Controllers/AccountsControllerTests.cs @@ -1,4 +1,5 @@ -using Bit.Core.Auth.Models.Api.Request.Accounts; +using Bit.Core; +using Bit.Core.Auth.Models.Api.Request.Accounts; using Bit.Core.Auth.Models.Business.Tokenables; using Bit.Core.Auth.Services; using Bit.Core.Entities; @@ -64,14 +65,14 @@ public class AccountsControllerTests : IDisposable } [Fact] - public async Task PostPrelogin_WhenUserDoesNotExist_ShouldDefaultToSha256And100000Iterations() + public async Task PostPrelogin_WhenUserDoesNotExist_ShouldDefaultToPBKDF() { _userRepository.GetKdfInformationByEmailAsync(Arg.Any()).Returns(Task.FromResult(null!)); var response = await _sut.PostPrelogin(new PreloginRequestModel { Email = "user@example.com" }); Assert.Equal(KdfType.PBKDF2_SHA256, response.Kdf); - Assert.Equal(100000, response.KdfIterations); + Assert.Equal(AuthConstants.PBKDF2_ITERATIONS.Default, response.KdfIterations); } [Fact]