From c23196611f3bdbb1992b0cb9732f08cc5b432793 Mon Sep 17 00:00:00 2001 From: David Indra Date: Mon, 21 Sep 2026 18:18:03 +0000 Subject: [PATCH 1/4] Build emailed account links from a configured base URL Password reset, email confirmation and email change links were built from the incoming request, so a forged Host header produced a genuine email carrying a valid token that pointed at the attacker's origin. Adds AppOptions.PublicBaseUrl and an IPublicUrlProvider that all five emailed link sites now use. It takes precedence over the request-derived base the registration filter passes in. Unset, behaviour is unchanged, and production startup warns when neither PublicBaseUrl nor AllowedHosts is pinned. --- .../Account/Pages/ForgotPassword.razor | 3 +- .../Account/Pages/Manage/Email.razor | 9 +-- .../Pages/ResendEmailConfirmation.razor | 5 +- ControlR.Web.Server/Options/AppOptions.cs | 18 ++++++ .../Services/PublicUrlProvider.cs | 63 +++++++++++++++++++ .../Services/Users/UserCreator.cs | 10 ++- .../WebApplicationBuilderExtensions.cs | 11 ++++ ControlR.Web.Server/appsettings.json | 1 + docker-compose/docker-compose-secrets.yml | 12 ++++ docker-compose/docker-compose.yml | 12 ++++ 10 files changed, 135 insertions(+), 9 deletions(-) create mode 100644 ControlR.Web.Server/Services/PublicUrlProvider.cs diff --git a/ControlR.Web.Server/Components/Account/Pages/ForgotPassword.razor b/ControlR.Web.Server/Components/Account/Pages/ForgotPassword.razor index b3d86ed14..28f59ce9b 100644 --- a/ControlR.Web.Server/Components/Account/Pages/ForgotPassword.razor +++ b/ControlR.Web.Server/Components/Account/Pages/ForgotPassword.razor @@ -6,6 +6,7 @@ @inject NavigationManager NavigationManager @inject IdentityRedirectManager RedirectManager @inject IPasswordManager PasswordManager +@inject IPublicUrlProvider PublicUrlProvider Forgot your password? @@ -55,7 +56,7 @@ else return; } - var callbackUrl = NavigationManager.ToAbsoluteUri("Account/ResetPassword").AbsoluteUri; + var callbackUrl = PublicUrlProvider.GetAbsoluteUri("Account/ResetPassword"); var result = await PasswordManager.ForgotPassword(new InternalDtos.ForgotPasswordRequestDto(Input.Email), callbackUrl); if (!result.IsSuccess) { diff --git a/ControlR.Web.Server/Components/Account/Pages/Manage/Email.razor b/ControlR.Web.Server/Components/Account/Pages/Manage/Email.razor index 1a5c156f8..5464317c3 100644 --- a/ControlR.Web.Server/Components/Account/Pages/Manage/Email.razor +++ b/ControlR.Web.Server/Components/Account/Pages/Manage/Email.razor @@ -5,6 +5,7 @@ @inject IOptionsMonitor AppOptions @inject IdentityRedirectManager RedirectManager @inject NavigationManager NavigationManager +@inject IPublicUrlProvider PublicUrlProvider Manage email @@ -94,8 +95,8 @@ var userId = await UserManager.GetUserIdAsync(_user); var code = await UserManager.GenerateChangeEmailTokenAsync(_user, Input.NewEmail); code = WebEncoders.Base64UrlEncode(Encoding.UTF8.GetBytes(code)); - var callbackUrl = NavigationManager.GetUriWithQueryParameters( - NavigationManager.ToAbsoluteUri("Account/ConfirmEmailChange").AbsoluteUri, + var callbackUrl = PublicUrlProvider.GetAbsoluteUri( + "Account/ConfirmEmailChange", new Dictionary { ["userId"] = userId, ["email"] = Input.NewEmail, ["code"] = code }); await EmailSender.SendConfirmationLinkAsync(_user, Input.NewEmail, HtmlEncoder.Default.Encode(callbackUrl)); @@ -119,8 +120,8 @@ var userId = await UserManager.GetUserIdAsync(_user); var code = await UserManager.GenerateEmailConfirmationTokenAsync(_user); code = WebEncoders.Base64UrlEncode(Encoding.UTF8.GetBytes(code)); - var callbackUrl = NavigationManager.GetUriWithQueryParameters( - NavigationManager.ToAbsoluteUri("Account/ConfirmEmail").AbsoluteUri, + var callbackUrl = PublicUrlProvider.GetAbsoluteUri( + "Account/ConfirmEmail", new Dictionary { ["userId"] = userId, ["code"] = code }); await EmailSender.SendConfirmationLinkAsync(_user, _email, HtmlEncoder.Default.Encode(callbackUrl)); diff --git a/ControlR.Web.Server/Components/Account/Pages/ResendEmailConfirmation.razor b/ControlR.Web.Server/Components/Account/Pages/ResendEmailConfirmation.razor index 9097bd8ff..569e5af39 100644 --- a/ControlR.Web.Server/Components/Account/Pages/ResendEmailConfirmation.razor +++ b/ControlR.Web.Server/Components/Account/Pages/ResendEmailConfirmation.razor @@ -5,6 +5,7 @@ @inject IOptionsMonitor AppOptions @inject NavigationManager NavigationManager @inject IdentityRedirectManager RedirectManager +@inject IPublicUrlProvider PublicUrlProvider Resend email confirmation @@ -68,8 +69,8 @@ else var userId = await UserManager.GetUserIdAsync(user); var code = await UserManager.GenerateEmailConfirmationTokenAsync(user); code = WebEncoders.Base64UrlEncode(Encoding.UTF8.GetBytes(code)); - var callbackUrl = NavigationManager.GetUriWithQueryParameters( - NavigationManager.ToAbsoluteUri("Account/ConfirmEmail").AbsoluteUri, + var callbackUrl = PublicUrlProvider.GetAbsoluteUri( + "Account/ConfirmEmail", new Dictionary { ["userId"] = userId, ["code"] = code }); await EmailSender.SendConfirmationLinkAsync(user, Input.Email, HtmlEncoder.Default.Encode(callbackUrl)); diff --git a/ControlR.Web.Server/Options/AppOptions.cs b/ControlR.Web.Server/Options/AppOptions.cs index ecfed081c..4282e1977 100644 --- a/ControlR.Web.Server/Options/AppOptions.cs +++ b/ControlR.Web.Server/Options/AppOptions.cs @@ -238,6 +238,24 @@ public class AppOptions /// public bool PersistPasskeyLogin { get; init; } + /// + /// The absolute base URL where this server is reachable by its users, e.g. "https://controlr.example.com". + /// + /// + /// + /// Links that are emailed to users (password reset, email confirmation, email change) are built + /// from this value. When it is not set, those links fall back to the host of the incoming request, + /// which an attacker controls via the Host header unless AllowedHosts is pinned or a + /// reverse proxy overwrites it. A forged host produces a genuine email from this server carrying a + /// valid token that points at the attacker's origin. + /// + /// + /// Set this on every internet-facing deployment. Include the scheme and, if non-standard, the port; + /// a trailing slash is optional. + /// + /// + public string? PublicBaseUrl { get; init; } + /// /// Whether users must confirm their email address before being allowed to log in. /// If true, you must also configure SMTP settings below. diff --git a/ControlR.Web.Server/Services/PublicUrlProvider.cs b/ControlR.Web.Server/Services/PublicUrlProvider.cs new file mode 100644 index 000000000..3f8654f38 --- /dev/null +++ b/ControlR.Web.Server/Services/PublicUrlProvider.cs @@ -0,0 +1,63 @@ +using Microsoft.AspNetCore.Components; + +namespace ControlR.Web.Server.Services; + +/// +/// Builds absolute URLs for links that leave the application, such as the callback links embedded in +/// account emails. +/// +/// +/// Emailed links must never be derived from the incoming request. The Host header is +/// attacker-controlled unless AllowedHosts is pinned or a reverse proxy overwrites it, and a +/// forged host turns a password-reset mail into a genuine token delivered to the attacker's origin. +/// Callers get the configured when one is set, and only fall +/// back to the request when it is not. +/// +public interface IPublicUrlProvider +{ + /// + /// Whether an explicit public base URL is configured. When false, generated URLs fall back to the + /// host of the incoming request. + /// + bool HasConfiguredBaseUrl { get; } + + /// + /// Builds an absolute URL for a path relative to the application root. + /// + /// The path relative to the application root, e.g. "Account/ResetPassword". + string GetAbsoluteUri(string relativePath); + + /// + /// Builds an absolute URL for a path relative to the application root, with the supplied query + /// parameters appended. + /// + /// The path relative to the application root, e.g. "Account/ConfirmEmail". + /// The query parameters to append. + string GetAbsoluteUri(string relativePath, IReadOnlyDictionary queryParameters); +} + +public class PublicUrlProvider( + NavigationManager navigationManager, + IOptionsMonitor appOptions) : IPublicUrlProvider +{ + private readonly IOptionsMonitor _appOptions = appOptions; + private readonly NavigationManager _navigationManager = navigationManager; + + public bool HasConfiguredBaseUrl => !string.IsNullOrWhiteSpace(_appOptions.CurrentValue.PublicBaseUrl); + + public string GetAbsoluteUri(string relativePath) + { + var configuredBaseUrl = _appOptions.CurrentValue.PublicBaseUrl; + if (string.IsNullOrWhiteSpace(configuredBaseUrl)) + { + return _navigationManager.ToAbsoluteUri(relativePath.TrimStart('/')).AbsoluteUri; + } + + return $"{configuredBaseUrl.TrimEnd('/')}/{relativePath.TrimStart('/')}"; + } + + public string GetAbsoluteUri(string relativePath, IReadOnlyDictionary queryParameters) + { + return _navigationManager.GetUriWithQueryParameters(GetAbsoluteUri(relativePath), queryParameters); + } +} diff --git a/ControlR.Web.Server/Services/Users/UserCreator.cs b/ControlR.Web.Server/Services/Users/UserCreator.cs index 853b6ac94..4482e4aff 100644 --- a/ControlR.Web.Server/Services/Users/UserCreator.cs +++ b/ControlR.Web.Server/Services/Users/UserCreator.cs @@ -333,9 +333,15 @@ await _assignmentSeeder.SeedAssignments( ["returnUrl"] = returnUrl }; - var callbackUrl = confirmationBaseUrl is not null + // The configured public base URL wins over any caller-supplied base, because callers such as + // the registration endpoint filter derive theirs from the request's Host header. + var effectiveBaseUrl = string.IsNullOrWhiteSpace(_appOptions.CurrentValue.PublicBaseUrl) + ? confirmationBaseUrl + : _appOptions.CurrentValue.PublicBaseUrl; + + var callbackUrl = effectiveBaseUrl is not null ? QueryHelpers.AddQueryString( - $"{confirmationBaseUrl.TrimEnd('/')}/Account/ConfirmEmail", + $"{effectiveBaseUrl.TrimEnd('/')}/Account/ConfirmEmail", queryParams) : _navigationManager.GetUriWithQueryParameters( _navigationManager.ToAbsoluteUri("Account/ConfirmEmail").AbsoluteUri, diff --git a/ControlR.Web.Server/Startup/WebApplicationBuilderExtensions.cs b/ControlR.Web.Server/Startup/WebApplicationBuilderExtensions.cs index 9252920e5..c5df6ad46 100644 --- a/ControlR.Web.Server/Startup/WebApplicationBuilderExtensions.cs +++ b/ControlR.Web.Server/Startup/WebApplicationBuilderExtensions.cs @@ -80,6 +80,16 @@ public static async Task AddControlrServer( .GetSection(AppOptions.SectionKey) .Get() ?? new AppOptions(); + if (builder.Environment.IsProduction() && + string.IsNullOrWhiteSpace(appOptions.PublicBaseUrl) && + builder.Configuration["AllowedHosts"] is null or "" or "*") + { + Console.WriteLine( + "Links emailed to users will be built from the incoming request's Host header, which is " + + "attacker-controlled. Set AppOptions:PublicBaseUrl to this server's public URL, or pin " + + "AllowedHosts to its hostnames."); + } + // Configure logging. builder.Logging.AddConfiguration(builder.Configuration.GetSection("Logging")); builder.Services.AddStarRedactor(); @@ -255,6 +265,7 @@ public static async Task AddControlrServer( builder.Services.AddScoped(services => services.GetRequiredService()); builder.Services.AddScoped(); builder.Services.AddScoped(); + builder.Services.AddScoped(); builder.Services.AddScoped(); builder.Services.AddScoped(); builder.Services.AddScoped(); diff --git a/ControlR.Web.Server/appsettings.json b/ControlR.Web.Server/appsettings.json index 76a267148..017fdbeec 100644 --- a/ControlR.Web.Server/appsettings.json +++ b/ControlR.Web.Server/appsettings.json @@ -72,6 +72,7 @@ "MicrosoftClientId": "", "MicrosoftClientSecret": "", "PersistPasskeyLogin": false, + "PublicBaseUrl": "", "RequireUserEmailConfirmation": false, "RequireUserUniqueEmail": true, "SmtpCheckCertificateRevocation": true, diff --git a/docker-compose/docker-compose-secrets.yml b/docker-compose/docker-compose-secrets.yml index 8dacb376b..451b9cc03 100644 --- a/docker-compose/docker-compose-secrets.yml +++ b/docker-compose/docker-compose-secrets.yml @@ -87,6 +87,18 @@ services: # recommended to balance between allowing legitimate clock skew and limiting replay attacks. ControlR_AppOptions__AgentClockSkewTolerance: "00:01:00" + # The absolute base URL where this server is reachable by its users, e.g. + # "https://controlr.example.com". Links emailed to users (password reset, email + # confirmation, email change) are built from this value. Leave it unset and those links + # fall back to the Host header of the incoming request, which a caller controls: a forged + # host produces a genuine email from this server carrying a valid token that points at the + # attacker's origin. Set this on every internet-facing deployment. + ControlR_AppOptions__PublicBaseUrl: "" + + # The hostnames this server will answer to, semicolon-separated. Pinning this rejects + # requests carrying a forged Host header before they reach the application. "*" allows any. + ControlR_AllowedHosts: "*" + # Allows devices to self-register without requiring an installer key. ControlR_AppOptions__AllowAgentsToSelfBootstrap: false diff --git a/docker-compose/docker-compose.yml b/docker-compose/docker-compose.yml index 90d02bcba..1c13cfc80 100644 --- a/docker-compose/docker-compose.yml +++ b/docker-compose/docker-compose.yml @@ -72,6 +72,18 @@ services: # that auto-disables after the first user is created. ControlR_AppOptions__EnablePublicRegistration: false + # The absolute base URL where this server is reachable by its users, e.g. + # "https://controlr.example.com". Links emailed to users (password reset, email + # confirmation, email change) are built from this value. Leave it unset and those links + # fall back to the Host header of the incoming request, which a caller controls: a forged + # host produces a genuine email from this server carrying a valid token that points at the + # attacker's origin. Set this on every internet-facing deployment. + ControlR_AppOptions__PublicBaseUrl: "" + + # The hostnames this server will answer to, semicolon-separated. Pinning this rejects + # requests carrying a forged Host header before they reach the application. "*" allows any. + ControlR_AllowedHosts: "*" + # Allows devices to self-register without requiring an installer key. ControlR_AppOptions__AllowAgentsToSelfBootstrap: false From 4fed50495d994874a5abeec1bf3f9abcee68ea91 Mon Sep 17 00:00:00 2001 From: David Indra Date: Mon, 21 Sep 2026 18:19:35 +0000 Subject: [PATCH 2/4] Close device takeover on the unauthenticated agent hub AgentHub has no [Authorize], so a device whose stored PublicKey was empty could be claimed by any anonymous caller that knew its id: the deprecated unsigned UpdateDevice accepted it outright, and UpdateDeviceSigned verified the signature against the key the caller supplied in the same message. Either rewrote ConnectionId, which routes viewer commands, and adopted the caller's key permanently. Removes UpdateDevice from the hub and IAgentHub, and restricts a caller- supplied key to bootstrapping a device the server has never seen. Re-keying an existing device now goes through the installer-key registration API. Current agents only call UpdateDeviceSigned; agents predating it will need to re-register. Decommission tests move to the signed path. --- ControlR.Web.Server/Hubs/AgentHub.cs | 110 ++++-------------- .../Hubs/IAgentHub.cs | 1 - .../DecommissionServerTests.cs | 45 ++++++- .../Helpers/ServiceExtensions.cs | 5 +- 4 files changed, 65 insertions(+), 96 deletions(-) diff --git a/ControlR.Web.Server/Hubs/AgentHub.cs b/ControlR.Web.Server/Hubs/AgentHub.cs index 0689891a7..b98f8f501 100644 --- a/ControlR.Web.Server/Hubs/AgentHub.cs +++ b/ControlR.Web.Server/Hubs/AgentHub.cs @@ -236,103 +236,39 @@ await _viewerHub.Clients } } - [Obsolete("This method is deprecated. Please use UpdateDeviceSigned instead.")] - public async Task> UpdateDevice(DeviceUpdateRequestDto agentDto) - { - try - { - var device = await _appDb.Devices.FindAsync(agentDto.Id); - if (device is not null && !string.IsNullOrEmpty(device.PublicKey)) - { - return HubResult.Fail("Device requires signed updates."); - } - - if (_serverOptions.Value.DecommissionServer) - { - return await HandleAgentUpdateForDecommission(agentDto, device); - } - - // Self-bootstrap: only permitted when exactly one tenant exists. - // Multi-tenant deployments must use installer keys with an explicit tenant. - if (_appOptions.Value.AllowAgentsToSelfBootstrap && agentDto.TenantId == Guid.Empty) - { - var tenants = await _appDb.Tenants - .OrderByDescending(x => x.CreatedAt) - .Take(2) - .ToListAsync(); - - if (tenants.Count == 0) - { - return HubResult.Fail("No tenants found."); - } - - if (tenants.Count > 1) - { - return HubResult.Fail( - "Self-bootstrap is only allowed on single-tenant servers. Use an installer key instead."); - } - - // Update the DTO with the assigned TenantId - agentDto = agentDto with { TenantId = tenants[0].Id }; - } - - if (agentDto.TenantId == Guid.Empty) - { - return HubResult.Fail("Invalid tenant ID."); - } - - if (!await _appDb.Tenants.AnyAsync(x => x.Id == agentDto.TenantId)) - { - return HubResult.Fail("Invalid tenant ID."); - } - - var remoteIp = Context.GetHttpContext()?.Connection.RemoteIpAddress; - var connectionContext = new DeviceConnectionContext( - ConnectionId: Context.ConnectionId, - RemoteIpAddress: remoteIp, - LastSeen: _timeProvider.GetLocalNow(), - IsOnline: true - ); - - var updateResult = await UpdateDeviceEntity(agentDto, connectionContext); - - if (!updateResult.IsSuccess) - { - return HubResult.Fail(updateResult.Reason); - } - - var deviceEntity = updateResult.Value; - - var isOutdated = await GetIsAgentOutdated(deviceEntity); - Device = deviceEntity.ToInternalResponseDto(isOutdated); - - await SendDeviceUpdate(deviceEntity, Device); - - return HubResult.Ok(Device); - } - catch (Exception ex) - { - _logger.LogError(ex, "Error while updating device."); - return HubResult.Fail("An error occurred while updating the device."); - } - } - public async Task> UpdateDeviceSigned(SignedDto signedDto) { try { var agentDto = signedDto.Dto; - // Only trust the agent-supplied key when self-bootstrap is enabled. + // A stored key always wins. An agent-supplied one is trusted only to bootstrap a device the + // server has never seen, and only when self-bootstrap is enabled. var device = await _appDb.Devices.FindAsync(agentDto.Id); var storedPublicKey = device?.PublicKey; - if (string.IsNullOrEmpty(storedPublicKey) && !_appOptions.Value.AllowAgentsToSelfBootstrap) + if (string.IsNullOrEmpty(storedPublicKey)) { - _logger.LogWarning( - "Rejecting update from unknown device {DeviceId}. Self-bootstrap is disabled.", - agentDto.Id); - return HubResult.Fail("Unknown device."); + // A caller-supplied key is only ever trusted to bootstrap a device the server has never + // seen. Honouring one for a device that already exists would let any anonymous caller who + // knows its id re-key it and take over its hub connection. Adopting a key for an existing + // device goes through the installer-key-authenticated registration API instead. + if (device is not null) + { + _logger.LogWarning( + "Rejecting update for device {DeviceId}, which has no registered public key. " + + "Re-register the device with an installer key to adopt one.", + agentDto.Id); + return HubResult.Fail("Device has no registered public key."); + } + + if (!_appOptions.Value.AllowAgentsToSelfBootstrap) + { + _logger.LogWarning( + "Rejecting update from unknown device {DeviceId}. Self-bootstrap is disabled.", + agentDto.Id); + return HubResult.Fail("Unknown device."); + } } var publicKeyBase64 = !string.IsNullOrEmpty(storedPublicKey) diff --git a/Libraries/ControlR.Libraries.Api.Contracts/Hubs/IAgentHub.cs b/Libraries/ControlR.Libraries.Api.Contracts/Hubs/IAgentHub.cs index e3b17dc5e..a92d58f89 100644 --- a/Libraries/ControlR.Libraries.Api.Contracts/Hubs/IAgentHub.cs +++ b/Libraries/ControlR.Libraries.Api.Contracts/Hubs/IAgentHub.cs @@ -13,6 +13,5 @@ public interface IAgentHub Task SendFileContentStream(Guid streamId, ChannelReader fileChunks); Task SendSubdirectoriesStream(Guid streamId, ChannelReader subdirectoryChunks); Task SendTerminalOutputToViewer(string viewerConnectionId, TerminalOutputDto outputDto); - Task> UpdateDevice(DeviceUpdateRequestDto agentDto); Task> UpdateDeviceSigned(SignedDto signedDto); } diff --git a/Tests/ControlR.Web.Server.Tests/DecommissionServerTests.cs b/Tests/ControlR.Web.Server.Tests/DecommissionServerTests.cs index ab8aad902..49e6faa31 100644 --- a/Tests/ControlR.Web.Server.Tests/DecommissionServerTests.cs +++ b/Tests/ControlR.Web.Server.Tests/DecommissionServerTests.cs @@ -16,6 +16,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Moq; +using ControlR.Libraries.Shared.Primitives; using ControlR.Libraries.Shared.Services.Encryption; using ControlR.Web.Server.Api.Internal; @@ -23,6 +24,8 @@ namespace ControlR.Web.Server.Tests; public class DecommissionServerTests(ITestOutputHelper testOutput) { + private static readonly string _testPublicKeyBase64 = Convert.ToBase64String(new byte[32]); + private readonly ITestOutputHelper _testOutput = testOutput; [Fact] @@ -87,7 +90,7 @@ public void ServerLifecycleOptions_BindsFromConfiguration() } [Fact] - public async Task UpdateDevice_WhenServerDecommissioned_UninstallsAgentAndDeletesDevice() + public async Task UpdateDeviceSigned_WhenServerDecommissioned_UninstallsAgentAndDeletesDevice() { await using var testApp = await TestAppBuilder.CreateTestApp(_testOutput); using var scope = testApp.Services.CreateScope(); @@ -95,7 +98,7 @@ public async Task UpdateDevice_WhenServerDecommissioned_UninstallsAgentAndDelete var tenant = await services.CreateTestTenant(); var deviceId = Guid.NewGuid(); - _ = await services.CreateTestDevice(tenant.Id, deviceId); + _ = await services.CreateTestDevice(tenant.Id, deviceId, _testPublicKeyBase64); await using var appDb = services.GetRequiredService(); var timeProvider = services.GetRequiredService(); @@ -115,6 +118,15 @@ public async Task UpdateDevice_WhenServerDecommissioned_UninstallsAgentAndDelete var mockHubStreamStore = new Mock(); var mockAgentVersionProvider = new Mock(); var mockKeyProvider = new Mock(); + mockKeyProvider + .Setup(x => x.ValidatePublicKeyBase64(It.IsAny())) + .Returns(Result.Ok(new byte[32])); + mockKeyProvider + .Setup(x => x.Verify(It.IsAny>(), It.IsAny())) + .Returns(true); + mockKeyProvider + .Setup(x => x.VerifyTimestamp(It.IsAny>(), It.IsAny())) + .Returns(true); var mockLogger = new Mock>(); var serverOptions = Microsoft.Extensions.Options.Options.Create( @@ -171,7 +183,13 @@ public async Task UpdateDevice_WhenServerDecommissioned_UninstallsAgentAndDelete ]); // Act - var result = await hub.UpdateDevice(deviceDto); + var signedDto = new SignedDto( + deviceDto, + DateTimeOffset.UtcNow, + new byte[64], + _testPublicKeyBase64); + + var result = await hub.UpdateDeviceSigned(signedDto); // Assert - HubResult indicates failure with the decommissioned message. Assert.False(result.IsSuccess); @@ -194,7 +212,7 @@ public async Task UpdateDevice_WhenServerDecommissioned_UninstallsAgentAndDelete } [Fact] - public async Task UpdateDevice_WhenServerNotDecommissioned_DoesNotCallUninstallAgent() + public async Task UpdateDeviceSigned_WhenServerNotDecommissioned_DoesNotCallUninstallAgent() { await using var testApp = await TestAppBuilder.CreateTestApp(_testOutput); using var scope = testApp.Services.CreateScope(); @@ -202,7 +220,7 @@ public async Task UpdateDevice_WhenServerNotDecommissioned_DoesNotCallUninstallA var tenant = await services.CreateTestTenant(); var deviceId = Guid.NewGuid(); - _ = await services.CreateTestDevice(tenant.Id, deviceId); + _ = await services.CreateTestDevice(tenant.Id, deviceId, _testPublicKeyBase64); await using var appDb = services.GetRequiredService(); var timeProvider = services.GetRequiredService(); @@ -215,6 +233,15 @@ public async Task UpdateDevice_WhenServerNotDecommissioned_DoesNotCallUninstallA var mockHubStreamStore = new Mock(); var mockAgentVersionProvider = new Mock(); var mockKeyProvider = new Mock(); + mockKeyProvider + .Setup(x => x.ValidatePublicKeyBase64(It.IsAny())) + .Returns(Result.Ok(new byte[32])); + mockKeyProvider + .Setup(x => x.Verify(It.IsAny>(), It.IsAny())) + .Returns(true); + mockKeyProvider + .Setup(x => x.VerifyTimestamp(It.IsAny>(), It.IsAny())) + .Returns(true); var mockLogger = new Mock>(); // DecommissionServer is false (the default). @@ -271,7 +298,13 @@ public async Task UpdateDevice_WhenServerNotDecommissioned_DoesNotCallUninstallA ]); // Act - var result = await hub.UpdateDevice(deviceDto); + var signedDto = new SignedDto( + deviceDto, + DateTimeOffset.UtcNow, + new byte[64], + _testPublicKeyBase64); + + var result = await hub.UpdateDeviceSigned(signedDto); // Assert - UninstallAgent was NOT called. mockCaller.Verify( diff --git a/Tests/ControlR.Web.Server.Tests/Helpers/ServiceExtensions.cs b/Tests/ControlR.Web.Server.Tests/Helpers/ServiceExtensions.cs index 35c984a6e..1b116b454 100644 --- a/Tests/ControlR.Web.Server.Tests/Helpers/ServiceExtensions.cs +++ b/Tests/ControlR.Web.Server.Tests/Helpers/ServiceExtensions.cs @@ -128,7 +128,8 @@ public static async Task CreateServerPrincipal(this IServicePro public static async Task CreateTestDevice( this IServiceProvider services, Guid tenantId, - Guid? deviceId = null) + Guid? deviceId = null, + string? publicKeyBase64 = null) { using var scope = services.CreateScope(); var deviceManager = scope.ServiceProvider.GetRequiredService(); @@ -163,7 +164,7 @@ public static async Task CreateTestDevice( IsOnline: true ); - var device = await deviceManager.AddOrUpdate(deviceDto, connectionContext, tagIds: null); + var device = await deviceManager.AddOrUpdate(deviceDto, connectionContext, tagIds: null, publicKeyBase64: publicKeyBase64); return device; } From 02ac81bfd4dcc8f9b4ddf33314e881bb8a0cd7ab Mon Sep 17 00:00:00 2001 From: David Indra Date: Mon, 21 Sep 2026 18:20:07 +0000 Subject: [PATCH 3/4] Apply the self-bootstrap single-tenant check unconditionally The check only ran when the caller left TenantId empty, so naming an existing tenant explicitly skipped it and self-bootstrap accepted a chosen tenant on a multi-tenant server. It now runs whenever self-bootstrap is the authority for the write, and the tenant is taken from the server rather than the caller. --- ControlR.Web.Server/Hubs/AgentHub.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ControlR.Web.Server/Hubs/AgentHub.cs b/ControlR.Web.Server/Hubs/AgentHub.cs index b98f8f501..e316ed553 100644 --- a/ControlR.Web.Server/Hubs/AgentHub.cs +++ b/ControlR.Web.Server/Hubs/AgentHub.cs @@ -317,7 +317,12 @@ await _viewerHub.Clients // Allow agents to self-bootstrap when enabled. Only permitted when exactly one // tenant exists, so there's no ambiguity about where the agent lands. Multi-tenant // deployments must use installer keys, which carry an explicit tenant. - if (_appOptions.Value.AllowAgentsToSelfBootstrap && agentDto.TenantId == Guid.Empty) + // + // The restriction applies whenever self-bootstrap is the authority for this write, not only + // when the caller left the tenant unset. Gating it on an empty TenantId let a caller skip the + // single-tenant check simply by naming an existing tenant, which is the opposite of what the + // restriction is for. The tenant is taken from the server, never from the caller. + if (_appOptions.Value.AllowAgentsToSelfBootstrap && device is null) { var tenants = await _appDb.Tenants .OrderByDescending(x => x.CreatedAt) From 4c86806dc23148f5596b7c9813670eafd5a30580 Mon Sep 17 00:00:00 2001 From: David Indra Date: Mon, 21 Sep 2026 18:21:03 +0000 Subject: [PATCH 4/4] Compare relay access tokens in fixed time SessionSignaler used an ordinary string comparison, which short-circuits on the first differing byte. The responder half of a relay session is unauthenticated, so this token is the only check on that side. Does not address the other two gaps in the finding: the requester still has no authorization policy beyond being authenticated, and the session is still created by whichever peer connects first. --- .../Sessions/SessionSignaler.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Libraries/ControlR.Libraries.WebSocketRelay.Common/Sessions/SessionSignaler.cs b/Libraries/ControlR.Libraries.WebSocketRelay.Common/Sessions/SessionSignaler.cs index 174402c58..aceb4d74e 100644 --- a/Libraries/ControlR.Libraries.WebSocketRelay.Common/Sessions/SessionSignaler.cs +++ b/Libraries/ControlR.Libraries.WebSocketRelay.Common/Sessions/SessionSignaler.cs @@ -1,5 +1,7 @@ using System.Collections.Concurrent; using System.Net.WebSockets; +using System.Security.Cryptography; +using System.Text; namespace ControlR.Libraries.WebSocketRelay.Common.Sessions; @@ -156,7 +158,11 @@ public bool TryAssignRole(Guid peerId, RelayRole role) public bool ValidateToken(string accessToken) { - return accessToken == _accessToken; + // Compared in fixed time. The responder side of a relay session is unauthenticated, so for that + // half this token is the only thing between a caller and the peer on the other end. + return CryptographicOperations.FixedTimeEquals( + Encoding.UTF8.GetBytes(accessToken), + Encoding.UTF8.GetBytes(_accessToken)); } public async Task WaitForPartner(CancellationToken cancellationToken)