From 1fc46a31975706afdf3fb984646e4e905bc9da80 Mon Sep 17 00:00:00 2001 From: Nik Gilmore Date: Wed, 25 Mar 2026 13:29:00 -0700 Subject: [PATCH] Add SSRF protections to ChangePasswordUriSErvice --- src/Icons/Models/IconUri.cs | 2 +- .../Services/ChangePasswordUriService.cs | 96 ++++++++- src/Icons/Util/ServiceCollectionExtension.cs | 5 +- .../Services/ChangePasswordUriServiceTests.cs | 202 ++++++++++++++++-- 4 files changed, 272 insertions(+), 33 deletions(-) diff --git a/src/Icons/Models/IconUri.cs b/src/Icons/Models/IconUri.cs index 143bc26f72..e436d40066 100644 --- a/src/Icons/Models/IconUri.cs +++ b/src/Icons/Models/IconUri.cs @@ -39,7 +39,7 @@ public class IconUri } /// - /// Represents an ip-validated Uri for use in grabbing an icon. + /// Represents a URI validated against SSRF: resolved to a public IP and bound to it. /// /// /// diff --git a/src/Icons/Services/ChangePasswordUriService.cs b/src/Icons/Services/ChangePasswordUriService.cs index 6f2b73efff..63dc1ddb5b 100644 --- a/src/Icons/Services/ChangePasswordUriService.cs +++ b/src/Icons/Services/ChangePasswordUriService.cs @@ -1,12 +1,27 @@ -namespace Bit.Icons.Services; +using System.Net; +using Bit.Icons.Models; + +namespace Bit.Icons.Services; public class ChangePasswordUriService : IChangePasswordUriService { + private const int _maxRedirects = 2; + + private static readonly HttpStatusCode[] _redirectStatusCodes = + [ + HttpStatusCode.Redirect, + HttpStatusCode.MovedPermanently, + HttpStatusCode.RedirectKeepVerb, + HttpStatusCode.SeeOther + ]; + private readonly HttpClient _httpClient; + private readonly IUriService _uriService; - public ChangePasswordUriService(IHttpClientFactory httpClientFactory) + public ChangePasswordUriService(IHttpClientFactory httpClientFactory, IUriService uriService) { _httpClient = httpClientFactory.CreateClient("ChangePasswordUri"); + _uriService = uriService; } /// @@ -24,7 +39,6 @@ public class ChangePasswordUriService : IChangePasswordUriService var hasReliableStatusCode = await HasReliableHttpStatusCode(domain); var wellKnownChangePasswordUrl = await GetWellKnownChangePasswordUrl(domain); - if (hasReliableStatusCode && wellKnownChangePasswordUrl != null) { return wellKnownChangePasswordUrl; @@ -49,10 +63,16 @@ public class ChangePasswordUriService : IChangePasswordUriService Path = "/.well-known/resource-that-should-not-exist-whose-status-code-should-not-be-200" }; - var request = new HttpRequestMessage(HttpMethod.Get, url.ToString()); + var response = await SendSafeRequestAsync(url.Uri); + if (response == null) + { + return false; + } - var response = await _httpClient.SendAsync(request); - return !response.IsSuccessStatusCode; + using (response) + { + return !response.IsSuccessStatusCode; + } } catch { @@ -76,14 +96,72 @@ public class ChangePasswordUriService : IChangePasswordUriService Path = "/.well-known/change-password" }; - var request = new HttpRequestMessage(HttpMethod.Get, url.ToString()); + var response = await SendSafeRequestAsync(url.Uri); + if (response == null) + { + return null; + } - var response = await _httpClient.SendAsync(request); - return response.IsSuccessStatusCode ? url.ToString() : null; + using (response) + { + return response.IsSuccessStatusCode ? url.ToString() : null; + } } catch { return null; } } + + /// + /// Sends an HTTP GET request with SSRF protections: validates the target IP is not internal, + /// binds the request to the resolved IP to prevent DNS rebinding, and manually follows redirects + /// with validation at each hop. + /// + /// The HTTP response, or null if the URI fails SSRF validation. + private async Task SendSafeRequestAsync(Uri uri) + { + if (!_uriService.TryGetUri(uri, out var iconUri) || !iconUri!.IsValid) + { + return null; + } + + return await SendWithRedirectsAsync(iconUri, 0); + } + + private async Task SendWithRedirectsAsync(IconUri iconUri, int redirectCount) + { + HttpResponseMessage response; + try + { + using var message = new HttpRequestMessage(HttpMethod.Get, iconUri.InnerUri); + message.Headers.Host = iconUri.Host; + response = await _httpClient.SendAsync(message); + } + catch + { + return null; + } + + if (response.IsSuccessStatusCode || !_redirectStatusCodes.Contains(response.StatusCode)) + { + return response; + } + + // Handle redirect with SSRF validation + using (response) + { + if (redirectCount >= _maxRedirects || response.Headers.Location == null) + { + return null; + } + + if (!_uriService.TryGetRedirect(response, iconUri, out var redirectIconUri) || !redirectIconUri!.IsValid) + { + return null; + } + + return await SendWithRedirectsAsync(redirectIconUri, redirectCount + 1); + } + } } diff --git a/src/Icons/Util/ServiceCollectionExtension.cs b/src/Icons/Util/ServiceCollectionExtension.cs index 3bd3537198..9d774a139e 100644 --- a/src/Icons/Util/ServiceCollectionExtension.cs +++ b/src/Icons/Util/ServiceCollectionExtension.cs @@ -29,8 +29,8 @@ public static class ServiceCollectionExtension AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate, }); - // The CreatePasswordUri handler wants similar headers as Icons to portray coming from a browser but - // needs to follow redirects to get the final URL. + // The ChangePasswordUri handler wants similar headers as Icons to portray coming from a browser. + // Redirects are handled manually with SSRF validation at each hop. services.AddHttpClient("ChangePasswordUri", client => { client.Timeout = TimeSpan.FromSeconds(20); @@ -44,6 +44,7 @@ public static class ServiceCollectionExtension client.DefaultRequestHeaders.Add("Pragma", "no-cache"); }).ConfigurePrimaryHttpMessageHandler(() => new HttpClientHandler { + AllowAutoRedirect = false, AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate, }); } diff --git a/test/Icons.Test/Services/ChangePasswordUriServiceTests.cs b/test/Icons.Test/Services/ChangePasswordUriServiceTests.cs index 53b883733b..f4f4bf0092 100644 --- a/test/Icons.Test/Services/ChangePasswordUriServiceTests.cs +++ b/test/Icons.Test/Services/ChangePasswordUriServiceTests.cs @@ -1,4 +1,5 @@ using System.Net; +using Bit.Icons.Models; using Bit.Icons.Services; using Bit.Test.Common.MockedHttpClient; using NSubstitute; @@ -8,32 +9,123 @@ namespace Bit.Icons.Test.Services; public class ChangePasswordUriServiceTests : ServiceTestBase { + private static readonly IPAddress _publicIp = IPAddress.Parse("93.184.216.34"); + private static readonly IPAddress _loopbackIp = IPAddress.Parse("127.0.0.1"); + + /// + /// A fake IUriService that resolves all URIs to the given IP address. + /// + private class FakeUriService : IUriService + { + private readonly IPAddress _ip; + private readonly bool _shouldSucceed; + + public FakeUriService(IPAddress ip, bool shouldSucceed = true) + { + _ip = ip; + _shouldSucceed = shouldSucceed; + } + + public bool TryGetUri(string stringUri, out IconUri? iconUri) + { + if (!_shouldSucceed || !Uri.TryCreate(stringUri, UriKind.Absolute, out var uri)) + { + iconUri = null; + return false; + } + iconUri = new IconUri(uri, _ip); + return true; + } + + public bool TryGetUri(Uri uri, out IconUri? iconUri) + { + if (!_shouldSucceed) + { + iconUri = null; + return false; + } + iconUri = new IconUri(uri, _ip); + return true; + } + + public bool TryGetRedirect(HttpResponseMessage response, IconUri originalUri, out IconUri? iconUri) + { + iconUri = null; + return false; + } + } + + /// + /// A fake IUriService that succeeds for initial requests but returns an internal IP for redirects. + /// + private class FakeUriServiceWithInternalRedirect : IUriService + { + private readonly IPAddress _initialIp; + private readonly IPAddress _redirectIp; + + public FakeUriServiceWithInternalRedirect(IPAddress initialIp, IPAddress redirectIp) + { + _initialIp = initialIp; + _redirectIp = redirectIp; + } + + public bool TryGetUri(string stringUri, out IconUri? iconUri) + { + if (!Uri.TryCreate(stringUri, UriKind.Absolute, out var uri)) + { + iconUri = null; + return false; + } + iconUri = new IconUri(uri, _initialIp); + return true; + } + + public bool TryGetUri(Uri uri, out IconUri? iconUri) + { + iconUri = new IconUri(uri, _initialIp); + return true; + } + + public bool TryGetRedirect(HttpResponseMessage response, IconUri originalUri, out IconUri? iconUri) + { + if (response.Headers.Location == null) + { + iconUri = null; + return false; + } + + var redirectUri = response.Headers.Location.IsAbsoluteUri + ? response.Headers.Location + : new Uri(originalUri.InnerUri, response.Headers.Location); + + iconUri = new IconUri(redirectUri, _redirectIp); + return true; + } + } + [Theory] [InlineData("https://example.com", "https://example.com:443/.well-known/change-password")] public async Task GetChangePasswordUri_WhenBothChecksPass_ReturnsWellKnownUrl(string domain, string expectedUrl) { // Arrange var mockedHandler = new MockedHttpMessageHandler(); + var uriService = new FakeUriService(_publicIp); - var nonExistentUrl = $"{domain}/.well-known/resource-that-should-not-exist-whose-status-code-should-not-be-200"; - var changePasswordUrl = $"{domain}/.well-known/change-password"; - - // Mock the response for the resource-that-should-not-exist request (returns 404) + // Match requests by path since the host will be the resolved IP mockedHandler - .When(nonExistentUrl) + .When(r => r.RequestUri!.AbsolutePath.Contains("resource-that-should-not-exist")) .RespondWith(HttpStatusCode.NotFound) .WithContent(new StringContent("Not found")); - // Mock the response for the change-password request (returns 200) mockedHandler - .When(changePasswordUrl) + .When(r => r.RequestUri!.AbsolutePath == "/.well-known/change-password") .RespondWith(HttpStatusCode.OK) .WithContent(new StringContent("Ok")); var mockHttpFactory = Substitute.For(); mockHttpFactory.CreateClient("ChangePasswordUri").Returns(mockedHandler.ToHttpClient()); - var service = new ChangePasswordUriService(mockHttpFactory); + var service = new ChangePasswordUriService(mockHttpFactory, uriService); var result = await service.GetChangePasswordUri(domain); @@ -44,23 +136,23 @@ public class ChangePasswordUriServiceTests : ServiceTestBase(); var mockedHandler = new MockedHttpMessageHandler(); + var uriService = new FakeUriService(_publicIp); mockedHandler - .When(HttpMethod.Get, $"{domain}/.well-known/resource-that-should-not-exist-whose-status-code-should-not-be-200") + .When(r => r.RequestUri!.AbsolutePath.Contains("resource-that-should-not-exist")) .RespondWith(HttpStatusCode.OK) .WithContent(new StringContent("Ok")); mockedHandler - .When(HttpMethod.Get, $"{domain}/.well-known/change-password") + .When(r => r.RequestUri!.AbsolutePath == "/.well-known/change-password") .RespondWith(HttpStatusCode.OK) .WithContent(new StringContent("Ok")); - var httpClient = mockedHandler.ToHttpClient(); - mockHttpFactory.CreateClient("ChangePasswordUri").Returns(httpClient); + var mockHttpFactory = Substitute.For(); + mockHttpFactory.CreateClient("ChangePasswordUri").Returns(mockedHandler.ToHttpClient()); - var service = new ChangePasswordUriService(mockHttpFactory); + var service = new ChangePasswordUriService(mockHttpFactory, uriService); var result = await service.GetChangePasswordUri(domain); @@ -71,23 +163,23 @@ public class ChangePasswordUriServiceTests : ServiceTestBase(); var mockedHandler = new MockedHttpMessageHandler(); + var uriService = new FakeUriService(_publicIp); mockedHandler - .When(HttpMethod.Get, $"{domain}/.well-known/resource-that-should-not-exist-whose-status-code-should-not-be-200") + .When(r => r.RequestUri!.AbsolutePath.Contains("resource-that-should-not-exist")) .RespondWith(HttpStatusCode.NotFound) .WithContent(new StringContent("Not found")); mockedHandler - .When(HttpMethod.Get, $"{domain}/.well-known/change-password") + .When(r => r.RequestUri!.AbsolutePath == "/.well-known/change-password") .RespondWith(HttpStatusCode.NotFound) .WithContent(new StringContent("Not found")); - var httpClient = mockedHandler.ToHttpClient(); - mockHttpFactory.CreateClient("ChangePasswordUri").Returns(httpClient); + var mockHttpFactory = Substitute.For(); + mockHttpFactory.CreateClient("ChangePasswordUri").Returns(mockedHandler.ToHttpClient()); - var service = new ChangePasswordUriService(mockHttpFactory); + var service = new ChangePasswordUriService(mockHttpFactory, uriService); var result = await service.GetChangePasswordUri(domain); @@ -99,10 +191,78 @@ public class ChangePasswordUriServiceTests : ServiceTestBase(); - var service = new ChangePasswordUriService(mockHttpFactory); + var uriService = new FakeUriService(_publicIp); + var service = new ChangePasswordUriService(mockHttpFactory, uriService); var result = await service.GetChangePasswordUri(domain); Assert.Null(result); } + + [Fact] + public async Task GetChangePasswordUri_WhenDomainResolvesToInternalIp_ReturnsNull() + { + // UriService returns an IconUri with a loopback IP, which makes IsValid return false + var uriService = new FakeUriService(_loopbackIp); + + var mockedHandler = new MockedHttpMessageHandler(); + var mockHttpFactory = Substitute.For(); + mockHttpFactory.CreateClient("ChangePasswordUri").Returns(mockedHandler.ToHttpClient()); + + var service = new ChangePasswordUriService(mockHttpFactory, uriService); + + var result = await service.GetChangePasswordUri("https://evil.com"); + + // No HTTP requests should have been made + Assert.Empty(mockedHandler.CapturedRequests); + Assert.Null(result); + } + + [Fact] + public async Task GetChangePasswordUri_WhenDnsResolutionFails_ReturnsNull() + { + var uriService = new FakeUriService(_publicIp, shouldSucceed: false); + + var mockedHandler = new MockedHttpMessageHandler(); + var mockHttpFactory = Substitute.For(); + mockHttpFactory.CreateClient("ChangePasswordUri").Returns(mockedHandler.ToHttpClient()); + + var service = new ChangePasswordUriService(mockHttpFactory, uriService); + + var result = await service.GetChangePasswordUri("https://nonexistent.invalid"); + + Assert.Empty(mockedHandler.CapturedRequests); + Assert.Null(result); + } + + [Fact] + public async Task GetChangePasswordUri_WhenRedirectTargetsInternalIp_ReturnsNull() + { + // Initial URI resolves to a public IP, but redirects resolve to a loopback IP + var uriService = new FakeUriServiceWithInternalRedirect(_publicIp, _loopbackIp); + + var mockedHandler = new MockedHttpMessageHandler(); + + // Both endpoints redirect (simulating attacker redirect to localhost) + mockedHandler + .When(r => r.RequestUri!.AbsolutePath.Contains("resource-that-should-not-exist")) + .RespondWith(HttpStatusCode.Redirect) + .WithHeader("Location", "http://localhost:5000/some-path") + .WithContent(new StringContent("")); + + mockedHandler + .When(r => r.RequestUri!.AbsolutePath == "/.well-known/change-password") + .RespondWith(HttpStatusCode.Redirect) + .WithHeader("Location", "http://localhost:5000/version") + .WithContent(new StringContent("")); + + var mockHttpFactory = Substitute.For(); + mockHttpFactory.CreateClient("ChangePasswordUri").Returns(mockedHandler.ToHttpClient()); + + var service = new ChangePasswordUriService(mockHttpFactory, uriService); + + var result = await service.GetChangePasswordUri("https://attacker.com"); + + Assert.Null(result); + } }