Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/Core/Settings/GlobalSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,6 @@ public virtual string MailTemplateDirectory
public virtual int SendAccessTokenLifetimeInMinutes { get; set; } = 5;
public virtual bool EnableEmailVerification { get; set; }
public virtual string KdfDefaultHashKey { get; set; }
/// <summary>
/// This Hash Key is used to prevent enumeration attacks against the Send Access feature.
/// </summary>
public virtual string SendDefaultHashKey { get; set; }
Comment on lines -99 to -102

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ : We need to communicate to SRE that this value is no longer needed in our upper environs.

We want to make sure we're keeping our settings files lean so we will want to remove this setting.

public virtual string PricingUri { get; set; }
public virtual Fido2Settings Fido2 { get; set; } = new Fido2Settings();
public virtual ICommunicationSettings Communication { get; set; } = new CommunicationSettings();
Expand Down
12 changes: 2 additions & 10 deletions src/Core/Tools/Models/Data/SendAuthenticationTypes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,6 @@ namespace Bit.Core.Tools.Models.Data;
/// </example>
public abstract record SendAuthenticationMethod;

/// <summary>
/// Never issue a send claim.
/// </summary>
/// <remarks>
/// This claim is issued when a send does not exist or when a send
/// has exceeded its max access attempts.
/// </remarks>
public record NeverAuthenticate : SendAuthenticationMethod;

/// <summary>
/// Create a send claim automatically.
/// </summary>
Expand All @@ -50,6 +41,7 @@ public record ResourcePassword(string Hash) : SendAuthenticationMethod;
public record EmailOtp(string[] emails) : SendAuthenticationMethod;

/// <summary>
/// The send exists but cannot be accessed (expired, disabled, max access exceeded, or past deletion date).
/// The send cannot be accessed: it exists but is inaccessible (expired, disabled, max access exceeded,
/// or past deletion date), or no send matches the given id.
/// </summary>
public record SendInaccessible : SendAuthenticationMethod;
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ namespace Bit.Core.Tools.SendFeatures.Queries;
public class SendAuthenticationQuery : ISendAuthenticationQuery
{
private static readonly NotAuthenticated NOT_AUTHENTICATED = new NotAuthenticated();
private static readonly NeverAuthenticate NEVER_AUTHENTICATE = new NeverAuthenticate();
private static readonly SendInaccessible SEND_INACCESSIBLE = new SendInaccessible();

private readonly ISendRepository _sendRepository;
Expand All @@ -37,7 +36,7 @@ public async Task<SendAuthenticationMethod> GetAuthenticationMethod(Guid sendId)

SendAuthenticationMethod method = send switch
{
null => NEVER_AUTHENTICATE,
null => SEND_INACCESSIBLE,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❓ QUESTION: Was the removal of the Send enumeration protection reviewed with the security team?

Context

This change (plus the deletion of SendNeverAuthenticateRequestValidator and GlobalSettings.SendDefaultHashKey) removes a control that was deliberately added to prevent distinguishing a non-existent send_id from one that exists and requires auth. After this PR, an unknown send_id deterministically returns invalid_grant / send_id_invalid, while an existing, accessible, password- or OTP-protected Send returns password_hash_b64_required / email_required β€” so the two states become reliably distinguishable.

The PR description acknowledges the trade-off, and the residual risk looks low given Send IDs are 122 bits of randomness (offline enumeration is infeasible; the practical leak is limited to someone who already holds a link learning whether the Send is still live). Asking mainly so the sign-off is recorded on the PR β€” was AppSec looped in, given this reverses an explicitly documented anti-enumeration measure?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

var s when s.Disabled => SEND_INACCESSIBLE,
var s when s.AccessCount >= s.MaxAccessCount.GetValueOrDefault(int.MaxValue) => SEND_INACCESSIBLE,
var s when s.ExpirationDate.GetValueOrDefault(DateTime.MaxValue) < DateTime.UtcNow => SEND_INACCESSIBLE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,4 @@ public static class OtpEmail
{
public const string Subject = "Your Bitwarden Send verification code is {0}";
}

/// <summary>
/// We use these static strings to help guide the enumeration protection logic.
/// </summary>
public static class EnumerationProtection
{
public const string Guid = "guid";
public const string Password = "password";
public const string Email = "email";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ namespace Bit.Identity.IdentityServer.RequestValidators.SendAccess;

public class SendAccessGrantValidator(
ISendAuthenticationQuery _sendAuthenticationQuery,
ISendAuthenticationMethodValidator<NeverAuthenticate> _sendNeverAuthenticateValidator,
ISendAuthenticationMethodValidator<ResourcePassword> _sendPasswordRequestValidator,
ISendAuthenticationMethodValidator<EmailOtp> _sendEmailOtpRequestValidator) : IExtensionGrantValidator
{
Expand All @@ -37,12 +36,8 @@ public async Task ValidateAsync(ExtensionGrantValidationContext context)

switch (method)
{
case NeverAuthenticate never:
// null send scenario.
context.Result = await _sendNeverAuthenticateValidator.ValidateRequestAsync(context, never, sendIdGuid);
return;
case SendInaccessible:
// send exists but is not accessible (expired, disabled, max access exceeded, or past deletion date).
// send is inaccessible (expired, disabled, max access exceeded, or past deletion date), or does not exist.
context.Result = new GrantValidationResult(
TokenRequestErrors.InvalidGrant,
SendAccessConstants.SendIdGuidValidatorResults.InvalidSendId,
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ Send access tokens contain custom claims specific to the Send the Send grant typ

## Authentication methods

### `NeverAuthenticate`
### `SendInaccessible`

For a Send to be in this state two things can be true:
1. The Send has been modified and no longer allows access.
2. The Send does not exist.
The Send cannot be accessed. This covers a Send that exists but is disabled, expired, past its
deletion date, or has exhausted its max access count, as well as a `send_id` with no matching
Send. All of these return `invalid_grant` with `send_id_invalid`.

### `NotAuthenticated`

Expand Down
1 change: 0 additions & 1 deletion src/Identity/Utilities/ServiceCollectionExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ public static IIdentityServerBuilder AddCustomIdentityServerServices(this IServi
services.AddTransient<ILoginApprovingClientTypes, LoginApprovingClientTypes>();
services.AddTransient<ISendAuthenticationMethodValidator<ResourcePassword>, SendPasswordRequestValidator>();
services.AddTransient<ISendAuthenticationMethodValidator<EmailOtp>, SendEmailOtpRequestValidator>();
services.AddTransient<ISendAuthenticationMethodValidator<NeverAuthenticate>, SendNeverAuthenticateRequestValidator>();

var issuerUri = new Uri(globalSettings.BaseServiceUri.InternalIdentity);
var identityServerBuilder = services
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public async Task GetAuthenticationMethod_WhenRepositoryThrows_PropagatesExcepti

public static IEnumerable<object[]> AuthenticationMethodTestCases()
{
yield return new object[] { null, typeof(NeverAuthenticate) };
yield return new object[] { null, typeof(SendInaccessible) };
yield return new object[] { CreateSend(accessCount: 5, maxAccessCount: 5, emails: null, password: null, AuthType.None), typeof(SendInaccessible) };
yield return new object[] { CreateSend(accessCount: 6, maxAccessCount: 5, emails: null, password: null, AuthType.None), typeof(SendInaccessible) };
yield return new object[] { CreateSend(accessCount: 0, maxAccessCount: 10, emails: "person@company.com", password: null, AuthType.Email), typeof(EmailOtp) };
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
ο»Ώusing Bit.Core.Enums;
using Bit.Core.Tools.Entities;
using Bit.Core.Tools.Enums;
using Bit.Core.Tools.Models.Data;
using Bit.Core.Tools.Repositories;
using Bit.Core.Tools.SendFeatures.Queries.Interfaces;
using Bit.Identity.IdentityServer.Enums;
using Bit.Identity.IdentityServer.RequestValidators.SendAccess;
Expand Down Expand Up @@ -46,31 +49,62 @@ public async Task SendAccessGrant_ValidNotAuthenticatedSend_ReturnsAccessToken()
}

[Fact]
public async Task SendAccessGrant_MissingSendId_ReturnsInvalidRequest()
public async Task SendAccessGrant_ExistingAccessibleSend_ReturnsAccessToken()
{
// Arrange
var client = _factory.CreateClient();
var sendId = Guid.NewGuid();
var send = new Send
{
Id = sendId,
AuthType = AuthType.None,
Disabled = false,
AccessCount = 0,
MaxAccessCount = null,
ExpirationDate = null,
DeletionDate = DateTime.UtcNow.AddDays(1),
};

var requestBody = new FormUrlEncodedContent([
new KeyValuePair<string, string>(OidcConstants.TokenRequest.GrantType, CustomGrantTypes.SendAccess),
new KeyValuePair<string, string>(OidcConstants.TokenRequest.ClientId, BitwardenClient.Send)
]);
var client = _factory.WithWebHostBuilder(builder =>
{
builder.ConfigureServices(services =>
{
// Real SendAuthenticationQuery and SendAccessGrantValidator run unmocked;
// only the repository lookup is substituted.
var sendRepository = Substitute.For<ISendRepository>();
sendRepository.GetByIdAsync(sendId).Returns(send);
services.AddSingleton(sendRepository);
});
}).CreateClient();

var requestBody = SendAccessTestUtilities.CreateTokenRequestBody(sendId);

// Act
var response = await client.PostAsync("/connect/token", requestBody);

// Assert
Assert.True(response.IsSuccessStatusCode);
var content = await response.Content.ReadAsStringAsync();
Assert.Contains(OidcConstants.TokenErrors.InvalidRequest, content);
Assert.Contains($"{SendAccessConstants.TokenRequest.SendId} is required", content);
Assert.Contains(OidcConstants.TokenResponse.AccessToken, content);
}

[Fact]
public async Task SendAccessGrant_EmptySendGuid_ReturnsInvalidGrant()
public async Task SendAccessGrant_DeletedSend_ReturnsInvalidGrant()
{
// Arrange
var sendId = Guid.Empty;
var client = _factory.CreateClient();
var sendId = Guid.NewGuid();

var client = _factory.WithWebHostBuilder(builder =>
{
builder.ConfigureServices(services =>
{
// A deleted (or never-existing) send: the repository returns null, so the real
// SendAuthenticationQuery maps it to SendInaccessible and the real
// SendAccessGrantValidator produces invalid_grant / send_id_invalid.
var sendRepository = Substitute.For<ISendRepository>();
sendRepository.GetByIdAsync(sendId).Returns((Send?)null);
services.AddSingleton(sendRepository);
});
}).CreateClient();

var requestBody = SendAccessTestUtilities.CreateTokenRequestBody(sendId);

Expand All @@ -79,23 +113,36 @@ public async Task SendAccessGrant_EmptySendGuid_ReturnsInvalidGrant()

// Assert
var content = await response.Content.ReadAsStringAsync();
Assert.Contains("invalid_grant", content);
Assert.Contains(OidcConstants.TokenErrors.InvalidGrant, content);
Assert.Contains(SendAccessConstants.SendIdGuidValidatorResults.InvalidSendId, content);
}

[Fact]
public async Task SendAccessGrant_NeverAuthenticateSend_ReturnsInvalidGrant()
public async Task SendAccessGrant_MissingSendId_ReturnsInvalidRequest()
{
// Arrange
var sendId = Guid.NewGuid();
var client = _factory.WithWebHostBuilder(builder =>
{
builder.ConfigureServices(services =>
{
var sendAuthQuery = Substitute.For<ISendAuthenticationQuery>();
sendAuthQuery.GetAuthenticationMethod(sendId).Returns(new NeverAuthenticate());
services.AddSingleton(sendAuthQuery);
});
}).CreateClient();
var client = _factory.CreateClient();

var requestBody = new FormUrlEncodedContent([
new KeyValuePair<string, string>(OidcConstants.TokenRequest.GrantType, CustomGrantTypes.SendAccess),
new KeyValuePair<string, string>(OidcConstants.TokenRequest.ClientId, BitwardenClient.Send)
]);
Comment thread
harr1424 marked this conversation as resolved.
Dismissed

// Act
var response = await client.PostAsync("/connect/token", requestBody);

// Assert
var content = await response.Content.ReadAsStringAsync();
Assert.Contains(OidcConstants.TokenErrors.InvalidRequest, content);
Assert.Contains($"{SendAccessConstants.TokenRequest.SendId} is required", content);
}

[Fact]
public async Task SendAccessGrant_EmptySendGuid_ReturnsInvalidGrant()
{
// Arrange
var sendId = Guid.Empty;
var client = _factory.CreateClient();

var requestBody = SendAccessTestUtilities.CreateTokenRequestBody(sendId);

Expand Down
Loading
Loading