From f8db37d3f9aa42d47e7c4f4ca839e892d3f97afb Mon Sep 17 00:00:00 2001 From: Joe Milazzo Date: Mon, 31 Oct 2022 09:07:55 -0400 Subject: [PATCH] Security Patches (#1624) * Fixed an issue where migrate-email could be called when an admin already existed * When logging in, ensure there is no bias towards username or password when rejecting * Cleaned up some messaging around anonymous apis to ensure there are no attack vectors. --- API/Controllers/AccountController.cs | 48 +++++++++++++++++++++------- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/API/Controllers/AccountController.cs b/API/Controllers/AccountController.cs index b0d6c43ba1..09e211d519 100644 --- a/API/Controllers/AccountController.cs +++ b/API/Controllers/AccountController.cs @@ -186,7 +186,7 @@ public async Task> Login(LoginDto loginDto) .Include(u => u.UserPreferences) .SingleOrDefaultAsync(x => x.NormalizedUserName == loginDto.Username.ToUpper()); - if (user == null) return Unauthorized("Invalid username"); + if (user == null) return Unauthorized("Your credentials are not correct"); var result = await _signInManager .CheckPasswordSignInAsync(user, loginDto.Password, true); @@ -198,7 +198,7 @@ public async Task> Login(LoginDto loginDto) if (!result.Succeeded) { - return Unauthorized(result.IsNotAllowed ? "You must confirm your email first" : "Your credentials are not correct."); + return Unauthorized(result.IsNotAllowed ? "You must confirm your email first" : "Your credentials are not correct"); } // Update LastActive on account @@ -632,6 +632,11 @@ await _emailService.SendConfirmationEmail(new ConfirmationEmailDto() return BadRequest("There was an error setting up your account. Please check the logs"); } + /// + /// Last step in authentication flow, confirms the email token for email + /// + /// + /// [AllowAnonymous] [HttpPost("confirm-email")] public async Task> ConfirmEmail(ConfirmEmailDto dto) @@ -640,7 +645,8 @@ public async Task> ConfirmEmail(ConfirmEmailDto dto) if (user == null) { - return BadRequest("The email does not match the registered email"); + _logger.LogInformation("confirm-email failed from invalid registered email: {Email}", dto.Email); + return BadRequest("Invalid email confirmation"); } // Validate Password and Username @@ -654,7 +660,11 @@ public async Task> ConfirmEmail(ConfirmEmailDto dto) } - if (!await ConfirmEmailToken(dto.Token, user)) return BadRequest("Invalid Email Token"); + if (!await ConfirmEmailToken(dto.Token, user)) + { + _logger.LogInformation("confirm-email failed from invalid token: {Token}", dto.Token); + return BadRequest("Invalid email confirmation"); + } user.UserName = dto.Username; user.ConfirmationToken = null; @@ -694,11 +704,15 @@ public async Task ConfirmEmailUpdate(ConfirmEmailUpdateDto dto) var user = await _unitOfWork.UserRepository.GetUserByConfirmationToken(dto.Token); if (user == null) { - return BadRequest("Invalid Email Token"); + _logger.LogInformation("confirm-email failed from invalid registered email: {Email}", dto.Email); + return BadRequest("Invalid email confirmation"); } - if (!await ConfirmEmailToken(dto.Token, user)) return BadRequest("Invalid Email Token"); - + if (!await ConfirmEmailToken(dto.Token, user)) + { + _logger.LogInformation("confirm-email failed from invalid token: {Token}", dto.Token); + return BadRequest("Invalid email confirmation"); + } _logger.LogInformation("User is updating email from {OldEmail} to {NewEmail}", user.Email, dto.Email); var result = await _userManager.SetEmailAsync(user, dto.Email); @@ -728,12 +742,16 @@ public async Task> ConfirmForgotPassword(ConfirmPasswordRes var user = await _unitOfWork.UserRepository.GetUserByEmailAsync(dto.Email); if (user == null) { - return BadRequest("Invalid Details"); + return BadRequest("Invalid credentials"); } var result = await _userManager.VerifyUserTokenAsync(user, TokenOptions.DefaultProvider, "ResetPassword", dto.Token); - if (!result) return BadRequest("Unable to reset password, your email token is not correct."); + if (!result) + { + _logger.LogInformation("Unable to reset password, your email token is not correct: {@Dto}", dto); + return BadRequest("Invalid credentials"); + } var errors = await _accountService.ChangeUserPassword(user, dto.Password); return errors.Any() ? BadRequest(errors) : Ok("Password updated"); @@ -801,9 +819,13 @@ public async Task> IsEmailConfirmed() public async Task> ConfirmMigrationEmail(ConfirmMigrationEmailDto dto) { var user = await _unitOfWork.UserRepository.GetUserByEmailAsync(dto.Email); - if (user == null) return BadRequest("This email is not on system"); + if (user == null) return BadRequest("Invalid credentials"); - if (!await ConfirmEmailToken(dto.Token, user)) return BadRequest("Invalid Email Token"); + if (!await ConfirmEmailToken(dto.Token, user)) + { + _logger.LogInformation("confirm-migration-email email token is invalid"); + return BadRequest("Invalid credentials"); + } await _unitOfWork.CommitAsync(); @@ -865,6 +887,10 @@ private string GenerateEmailLink(string token, string routePart, string email, b [HttpPost("migrate-email")] public async Task> MigrateEmail(MigrateUserEmailDto dto) { + // If there is an admin account already, return + var users = await _unitOfWork.UserRepository.GetAdminUsersAsync(); + if (users.Any()) return BadRequest("Admin already exists"); + // Check if there is an existing invite var emailValidationErrors = await _accountService.ValidateEmail(dto.Email); if (emailValidationErrors.Any())