diff --git a/commafeed-server/doc/commafeed.md b/commafeed-server/doc/commafeed.md index e6a2ca05..c6d8f44c 100644 --- a/commafeed-server/doc/commafeed.md +++ b/commafeed-server/doc/commafeed.md @@ -287,6 +287,29 @@ MemorySize [🛈](#memory-size-note-anchor) `5M` + + + +`commafeed.http-client.block-local-addresses` + +Prevent access to local addresses to mitigate server-side request forgery (SSRF) attacks, which could potentially expose internal +resources. + +You may want to disable this if you subscribe to feeds that are only available on your local network and you trust all users of +your CommaFeed instance. + + + +Environment variable: `COMMAFEED_HTTP_CLIENT_BLOCK_LOCAL_ADDRESSES` + + +boolean + + + +`true` + + diff --git a/commafeed-server/src/main/java/com/commafeed/CommaFeedConfiguration.java b/commafeed-server/src/main/java/com/commafeed/CommaFeedConfiguration.java index 7fafb4fe..99a01216 100644 --- a/commafeed-server/src/main/java/com/commafeed/CommaFeedConfiguration.java +++ b/commafeed-server/src/main/java/com/commafeed/CommaFeedConfiguration.java @@ -138,6 +138,16 @@ public interface CommaFeedConfiguration { @WithDefault("5M") MemorySize maxResponseSize(); + /** + * Prevent access to local addresses to mitigate server-side request forgery (SSRF) attacks, which could potentially expose internal + * resources. + * + * You may want to disable this if you subscribe to feeds that are only available on your local network and you trust all users of + * your CommaFeed instance. + */ + @WithDefault("true") + boolean blockLocalAddresses(); + /** * HTTP client cache configuration */ diff --git a/commafeed-server/src/main/java/com/commafeed/backend/HttpGetter.java b/commafeed-server/src/main/java/com/commafeed/backend/HttpGetter.java index e4df949a..5ac061de 100644 --- a/commafeed-server/src/main/java/com/commafeed/backend/HttpGetter.java +++ b/commafeed-server/src/main/java/com/commafeed/backend/HttpGetter.java @@ -2,7 +2,9 @@ package com.commafeed.backend; import java.io.IOException; import java.io.InputStream; +import java.net.InetAddress; import java.net.URI; +import java.net.UnknownHostException; import java.time.Duration; import java.time.Instant; import java.time.InstantSource; @@ -10,8 +12,11 @@ import java.util.ArrayList; import java.util.List; import java.util.Optional; import java.util.concurrent.ExecutionException; +import java.util.stream.Stream; import org.apache.commons.lang3.StringUtils; +import org.apache.hc.client5.http.DnsResolver; +import org.apache.hc.client5.http.SystemDefaultDnsResolver; import org.apache.hc.client5.http.config.ConnectionConfig; import org.apache.hc.client5.http.config.RequestConfig; import org.apache.hc.client5.http.config.TlsConfig; @@ -66,6 +71,7 @@ public class HttpGetter { private final InstantSource instantSource; private final CloseableHttpClient client; private final Cache cache; + private final DnsResolver dnsResolver = SystemDefaultDnsResolver.INSTANCE; public HttpGetter(CommaFeedConfiguration config, InstantSource instantSource, CommaFeedVersion version, MetricRegistry metrics) { this.config = config; @@ -89,11 +95,20 @@ public class HttpGetter { () -> cache == null ? 0 : cache.asMap().values().stream().mapToInt(e -> e.content != null ? e.content.length : 0).sum()); } - public HttpResult get(String url) throws IOException, NotModifiedException, TooManyRequestsException { + public HttpResult get(String url) + throws IOException, NotModifiedException, TooManyRequestsException, SchemeNotAllowedException, HostNotAllowedException { return get(HttpRequest.builder(url).build()); } - public HttpResult get(HttpRequest request) throws IOException, NotModifiedException, TooManyRequestsException { + public HttpResult get(HttpRequest request) + throws IOException, NotModifiedException, TooManyRequestsException, SchemeNotAllowedException, HostNotAllowedException { + URI uri = URI.create(request.getUrl()); + ensureHttpScheme(uri.getScheme()); + + if (config.httpClient().blockLocalAddresses()) { + ensurePublicAddress(uri.getHost()); + } + final HttpResponse response; if (cache == null) { response = invoke(request); @@ -141,6 +156,28 @@ public class HttpGetter { response.getUrlAfterRedirect(), validFor); } + private void ensureHttpScheme(String scheme) throws SchemeNotAllowedException { + if (!"http".equals(scheme) && !"https".equals(scheme)) { + throw new SchemeNotAllowedException(scheme); + } + } + + private void ensurePublicAddress(String host) throws HostNotAllowedException, UnknownHostException { + if (host == null) { + throw new HostNotAllowedException(null); + } + + InetAddress[] addresses = dnsResolver.resolve(host); + if (Stream.of(addresses).anyMatch(this::isPrivateAddress)) { + throw new HostNotAllowedException(host); + } + } + + private boolean isPrivateAddress(InetAddress address) { + return address.isSiteLocalAddress() || address.isAnyLocalAddress() || address.isLinkLocalAddress() || address.isLoopbackAddress() + || address.isMulticastAddress(); + } + private HttpResponse invoke(HttpRequest request) throws IOException { log.debug("fetching {}", request.getUrl()); @@ -229,7 +266,7 @@ public class HttpGetter { } } - private static PoolingHttpClientConnectionManager newConnectionManager(CommaFeedConfiguration config) { + private PoolingHttpClientConnectionManager newConnectionManager(CommaFeedConfiguration config) { SSLFactory sslFactory = SSLFactory.builder().withUnsafeTrustMaterial().withUnsafeHostnameVerifier().build(); int poolSize = config.feedRefresh().httpThreads(); @@ -243,6 +280,7 @@ public class HttpGetter { .setDefaultTlsConfig(TlsConfig.custom().setHandshakeTimeout(Timeout.of(config.httpClient().sslHandshakeTimeout())).build()) .setMaxConnPerRoute(poolSize) .setMaxConnTotal(poolSize) + .setDnsResolver(dnsResolver) .build(); } @@ -279,6 +317,22 @@ public class HttpGetter { .build(); } + public static class SchemeNotAllowedException extends Exception { + private static final long serialVersionUID = 1L; + + public SchemeNotAllowedException(String scheme) { + super("Scheme not allowed: " + scheme); + } + } + + public static class HostNotAllowedException extends Exception { + private static final long serialVersionUID = 1L; + + public HostNotAllowedException(String host) { + super("Host not allowed: " + host); + } + } + @Getter public static class NotModifiedException extends Exception { private static final long serialVersionUID = 1L; diff --git a/commafeed-server/src/main/java/com/commafeed/backend/favicon/YoutubeFaviconFetcher.java b/commafeed-server/src/main/java/com/commafeed/backend/favicon/YoutubeFaviconFetcher.java index 42cedd85..caa2a1e5 100644 --- a/commafeed-server/src/main/java/com/commafeed/backend/favicon/YoutubeFaviconFetcher.java +++ b/commafeed-server/src/main/java/com/commafeed/backend/favicon/YoutubeFaviconFetcher.java @@ -11,8 +11,10 @@ import org.apache.hc.core5.net.URIBuilder; import com.commafeed.CommaFeedConfiguration; import com.commafeed.backend.HttpGetter; +import com.commafeed.backend.HttpGetter.HostNotAllowedException; import com.commafeed.backend.HttpGetter.HttpResult; import com.commafeed.backend.HttpGetter.NotModifiedException; +import com.commafeed.backend.HttpGetter.SchemeNotAllowedException; import com.commafeed.backend.HttpGetter.TooManyRequestsException; import com.commafeed.backend.model.Feed; import com.fasterxml.jackson.core.JsonPointer; @@ -92,7 +94,8 @@ public class YoutubeFaviconFetcher extends AbstractFaviconFetcher { return new Favicon(bytes, contentType); } - private byte[] fetchForUser(String googleAuthKey, String userId) throws IOException, NotModifiedException, TooManyRequestsException { + private byte[] fetchForUser(String googleAuthKey, String userId) + throws IOException, NotModifiedException, TooManyRequestsException, HostNotAllowedException, SchemeNotAllowedException { URI uri = UriBuilder.fromUri("https://www.googleapis.com/youtube/v3/channels") .queryParam("part", "snippet") .queryParam("key", googleAuthKey) @@ -102,7 +105,7 @@ public class YoutubeFaviconFetcher extends AbstractFaviconFetcher { } private byte[] fetchForChannel(String googleAuthKey, String channelId) - throws IOException, NotModifiedException, TooManyRequestsException { + throws IOException, NotModifiedException, TooManyRequestsException, HostNotAllowedException, SchemeNotAllowedException { URI uri = UriBuilder.fromUri("https://www.googleapis.com/youtube/v3/channels") .queryParam("part", "snippet") .queryParam("key", googleAuthKey) @@ -112,7 +115,7 @@ public class YoutubeFaviconFetcher extends AbstractFaviconFetcher { } private byte[] fetchForPlaylist(String googleAuthKey, String playlistId) - throws IOException, NotModifiedException, TooManyRequestsException { + throws IOException, NotModifiedException, TooManyRequestsException, HostNotAllowedException, SchemeNotAllowedException { URI uri = UriBuilder.fromUri("https://www.googleapis.com/youtube/v3/playlists") .queryParam("part", "snippet") .queryParam("key", googleAuthKey) diff --git a/commafeed-server/src/main/java/com/commafeed/backend/feed/FeedFetcher.java b/commafeed-server/src/main/java/com/commafeed/backend/feed/FeedFetcher.java index 1d88f47a..bb456c23 100644 --- a/commafeed-server/src/main/java/com/commafeed/backend/feed/FeedFetcher.java +++ b/commafeed-server/src/main/java/com/commafeed/backend/feed/FeedFetcher.java @@ -10,9 +10,11 @@ import org.apache.commons.lang3.StringUtils; import com.commafeed.backend.Digests; import com.commafeed.backend.HttpGetter; +import com.commafeed.backend.HttpGetter.HostNotAllowedException; import com.commafeed.backend.HttpGetter.HttpRequest; import com.commafeed.backend.HttpGetter.HttpResult; import com.commafeed.backend.HttpGetter.NotModifiedException; +import com.commafeed.backend.HttpGetter.SchemeNotAllowedException; import com.commafeed.backend.HttpGetter.TooManyRequestsException; import com.commafeed.backend.feed.parser.FeedParser; import com.commafeed.backend.feed.parser.FeedParserResult; @@ -41,8 +43,8 @@ public class FeedFetcher { } public FeedFetcherResult fetch(String feedUrl, boolean extractFeedUrlFromHtml, String lastModified, String eTag, - Instant lastPublishedDate, String lastContentHash) - throws FeedException, IOException, NotModifiedException, TooManyRequestsException { + Instant lastPublishedDate, String lastContentHash) throws FeedException, IOException, NotModifiedException, + TooManyRequestsException, SchemeNotAllowedException, HostNotAllowedException { log.debug("Fetching feed {}", feedUrl); HttpResult result = getter.get(HttpRequest.builder(feedUrl).lastModified(lastModified).eTag(eTag).build()); diff --git a/commafeed-server/src/main/resources/application.properties b/commafeed-server/src/main/resources/application.properties index e6059e45..a90aeff7 100644 --- a/commafeed-server/src/main/resources/application.properties +++ b/commafeed-server/src/main/resources/application.properties @@ -50,6 +50,7 @@ quarkus.native.add-all-charsets=true %test.commafeed.users.allow-registrations=true %test.commafeed.password-recovery-enabled=true %test.commafeed.http-client.cache.enabled=false +%test.commafeed.http-client.block-local-addresses=false %test.commafeed.database.cleanup.entries-max-age=0 %test.commafeed.feed-refresh.force-refresh-cooldown-duration=1m diff --git a/commafeed-server/src/test/java/com/commafeed/backend/HttpGetterTest.java b/commafeed-server/src/test/java/com/commafeed/backend/HttpGetterTest.java index ededc315..a06541cd 100644 --- a/commafeed-server/src/test/java/com/commafeed/backend/HttpGetterTest.java +++ b/commafeed-server/src/test/java/com/commafeed/backend/HttpGetterTest.java @@ -236,7 +236,7 @@ class HttpGetterTest { } @Test - void cacheSubsequentCalls() throws IOException, NotModifiedException, TooManyRequestsException { + void cacheSubsequentCalls() throws Exception { AtomicInteger calls = new AtomicInteger(); this.mockServerClient.when(HttpRequest.request().withMethod("GET")).respond(req -> { @@ -302,17 +302,16 @@ class HttpGetterTest { class Compression { @Test - void deflate() throws IOException, NotModifiedException, TooManyRequestsException { + void deflate() throws Exception { supportsCompression("deflate", DeflaterOutputStream::new); } @Test - void gzip() throws IOException, NotModifiedException, TooManyRequestsException { + void gzip() throws Exception { supportsCompression("gzip", GZIPOutputStream::new); } - void supportsCompression(String encoding, CompressionOutputStreamFunction compressionOutputStreamFunction) - throws IOException, NotModifiedException, TooManyRequestsException { + void supportsCompression(String encoding, CompressionOutputStreamFunction compressionOutputStreamFunction) throws Exception { String body = "my body"; HttpGetterTest.this.mockServerClient.when(HttpRequest.request().withMethod("GET")).respond(req -> { @@ -340,4 +339,64 @@ class HttpGetterTest { } + @Nested + class SchemeNotAllowed { + @Test + void file() { + Assertions.assertThrows(HttpGetter.SchemeNotAllowedException.class, () -> getter.get("file://localhost")); + } + + @Test + void ftp() { + Assertions.assertThrows(HttpGetter.SchemeNotAllowedException.class, () -> getter.get("ftp://localhost")); + } + } + + @Nested + class HostNotAllowed { + + @BeforeEach + void init() { + Mockito.when(config.httpClient().blockLocalAddresses()).thenReturn(true); + getter = new HttpGetter(config, () -> NOW, Mockito.mock(CommaFeedVersion.class), Mockito.mock(MetricRegistry.class)); + } + + @Test + void localhost() { + Assertions.assertThrows(HttpGetter.HostNotAllowedException.class, () -> getter.get("http://localhost")); + Assertions.assertThrows(HttpGetter.HostNotAllowedException.class, () -> getter.get("http://127.0.0.1")); + Assertions.assertThrows(HttpGetter.HostNotAllowedException.class, () -> getter.get("http://2130706433")); + Assertions.assertThrows(HttpGetter.HostNotAllowedException.class, () -> getter.get("http://0x7F.0x00.0x00.0X01")); + } + + @Test + void zero() { + Assertions.assertThrows(HttpGetter.HostNotAllowedException.class, () -> getter.get("http://0.0.0.0")); + } + + @Test + void linkLocal() { + Assertions.assertThrows(HttpGetter.HostNotAllowedException.class, () -> getter.get("http://169.254.12.34")); + Assertions.assertThrows(HttpGetter.HostNotAllowedException.class, () -> getter.get("http://169.254.169.254")); + } + + @Test + void multicast() { + Assertions.assertThrows(HttpGetter.HostNotAllowedException.class, () -> getter.get("http://224.2.3.4")); + Assertions.assertThrows(HttpGetter.HostNotAllowedException.class, () -> getter.get("http://239.255.255.254")); + } + + @Test + void privateIpv4Ranges() { + Assertions.assertThrows(HttpGetter.HostNotAllowedException.class, () -> getter.get("http://10.0.0.1")); + Assertions.assertThrows(HttpGetter.HostNotAllowedException.class, () -> getter.get("http://172.16.0.1")); + Assertions.assertThrows(HttpGetter.HostNotAllowedException.class, () -> getter.get("http://192.168.0.1")); + } + + @Test + void privateIpv6Ranges() { + Assertions.assertThrows(HttpGetter.HostNotAllowedException.class, () -> getter.get("http://fd12:3456:789a:1::1")); + } + } + } \ No newline at end of file diff --git a/commafeed-server/src/test/java/com/commafeed/e2e/ReadingIT.java b/commafeed-server/src/test/java/com/commafeed/e2e/ReadingIT.java index 02e1568c..f2dc9803 100644 --- a/commafeed-server/src/test/java/com/commafeed/e2e/ReadingIT.java +++ b/commafeed-server/src/test/java/com/commafeed/e2e/ReadingIT.java @@ -3,6 +3,7 @@ package com.commafeed.e2e; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.concurrent.TimeUnit; +import java.util.regex.Pattern; import org.apache.commons.io.IOUtils; import org.apache.hc.core5.http.HttpStatus;