1
0
mirror of https://github.com/bitwarden/server.git synced 2025-02-16 01:51:21 +01:00

[PM-3487] prevent account enumeration on auth request endpoint (#3239)

This commit is contained in:
Jake Fink 2023-09-11 10:23:32 -04:00 committed by GitHub
parent 917c657439
commit f909563211
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 26 additions and 17 deletions

View File

@ -82,34 +82,39 @@ public class AuthRequestService : IAuthRequestService
/// </remarks> /// </remarks>
public async Task<AuthRequest> CreateAuthRequestAsync(AuthRequestCreateRequestModel model) public async Task<AuthRequest> CreateAuthRequestAsync(AuthRequestCreateRequestModel model)
{ {
var user = await _userRepository.GetByEmailAsync(model.Email);
if (user == null)
{
throw new NotFoundException();
}
if (!_currentContext.DeviceType.HasValue) if (!_currentContext.DeviceType.HasValue)
{ {
throw new BadRequestException("Device type not provided."); throw new BadRequestException("Device type not provided.");
} }
if (_globalSettings.PasswordlessAuth.KnownDevicesOnly) var userNotFound = false;
var user = await _userRepository.GetByEmailAsync(model.Email);
if (user == null)
{
userNotFound = true;
}
else if (_globalSettings.PasswordlessAuth.KnownDevicesOnly)
{ {
var devices = await _deviceRepository.GetManyByUserIdAsync(user.Id); var devices = await _deviceRepository.GetManyByUserIdAsync(user.Id);
if (devices == null || !devices.Any(d => d.Identifier == model.DeviceIdentifier)) if (devices == null || !devices.Any(d => d.Identifier == model.DeviceIdentifier))
{ {
throw new BadRequestException( userNotFound = true;
"Login with device is only available on devices that have been previously logged in.");
} }
} }
// Anonymous endpoints must not leak that a user exists or not
if (userNotFound)
{
throw new BadRequestException("User or known device not found.");
}
// AdminApproval requests require correlating the user and their organization // AdminApproval requests require correlating the user and their organization
if (model.Type == AuthRequestType.AdminApproval) if (model.Type == AuthRequestType.AdminApproval)
{ {
// TODO: When single org policy is turned on we should query for only a single organization from the current user // TODO: When single org policy is turned on we should query for only a single organization from the current user
// and create only an AuthRequest for that organization and return only that one // and create only an AuthRequest for that organization and return only that one
// This will send out the request to all organizations this user belongs to // This will send out the request to all organizations this user belongs to
var organizationUsers = await _organizationUserRepository.GetManyByUserAsync(_currentContext.UserId!.Value); var organizationUsers = await _organizationUserRepository.GetManyByUserAsync(_currentContext.UserId!.Value);
if (organizationUsers.Count == 0) if (organizationUsers.Count == 0)
@ -173,7 +178,7 @@ public class AuthRequestService : IAuthRequestService
switch (authRequest.Type) switch (authRequest.Type)
{ {
case AuthRequestType.AdminApproval: case AuthRequestType.AdminApproval:
// AdminApproval has a different expiration time, by default is 7 days compared to // AdminApproval has a different expiration time, by default is 7 days compared to
// non-AdminApproval ones having a default of 15 minutes. // non-AdminApproval ones having a default of 15 minutes.
if (IsDateExpired(authRequest.CreationDate, _globalSettings.PasswordlessAuth.AdminRequestExpiration)) if (IsDateExpired(authRequest.CreationDate, _globalSettings.PasswordlessAuth.AdminRequestExpiration))
{ {
@ -213,7 +218,7 @@ public class AuthRequestService : IAuthRequestService
await _authRequestRepository.ReplaceAsync(authRequest); await _authRequestRepository.ReplaceAsync(authRequest);
// We only want to send an approval notification if the request is approved (or null), // We only want to send an approval notification if the request is approved (or null),
// to not leak that it was denied to the originating client if it was originated by a malicious actor. // to not leak that it was denied to the originating client if it was originated by a malicious actor.
if (authRequest.Approved ?? true) if (authRequest.Approved ?? true)
{ {

View File

@ -142,15 +142,19 @@ public class AuthRequestServiceTests
} }
[Theory, BitAutoData] [Theory, BitAutoData]
public async Task CreateAuthRequestAsync_NoUser_ThrowsNotFound( public async Task CreateAuthRequestAsync_NoUser_ThrowsBadRequest(
SutProvider<AuthRequestService> sutProvider, SutProvider<AuthRequestService> sutProvider,
AuthRequestCreateRequestModel createModel) AuthRequestCreateRequestModel createModel)
{ {
sutProvider.GetDependency<ICurrentContext>()
.DeviceType
.Returns(DeviceType.Android);
sutProvider.GetDependency<IUserRepository>() sutProvider.GetDependency<IUserRepository>()
.GetByEmailAsync(createModel.Email) .GetByEmailAsync(createModel.Email)
.Returns((User?)null); .Returns((User?)null);
await Assert.ThrowsAsync<NotFoundException>(() => sutProvider.Sut.CreateAuthRequestAsync(createModel)); await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.CreateAuthRequestAsync(createModel));
} }
[Theory, BitAutoData] [Theory, BitAutoData]
@ -253,7 +257,7 @@ public class AuthRequestServiceTests
/// <summary> /// <summary>
/// Story: If a user happens to exist to more than one organization, we will send the device approval request to /// Story: If a user happens to exist to more than one organization, we will send the device approval request to
/// each of them. /// each of them.
/// </summary> /// </summary>
[Theory, BitAutoData] [Theory, BitAutoData]
public async Task CreateAuthRequestAsync_AdminApproval_CreatesForEachOrganization( public async Task CreateAuthRequestAsync_AdminApproval_CreatesForEachOrganization(
@ -627,8 +631,8 @@ public class AuthRequestServiceTests
} }
/// <summary> /// <summary>
/// Story: An admin approves a request for one of their org users. For auditing purposes we need to /// Story: An admin approves a request for one of their org users. For auditing purposes we need to
/// log an event that correlates the action for who the request was approved for. On approval we also need to /// log an event that correlates the action for who the request was approved for. On approval we also need to
/// push the notification to the user. /// push the notification to the user.
/// </summary> /// </summary>
[Theory, BitAutoData] [Theory, BitAutoData]