From bd179579e6045f6b483e1868f7a0bd305f86f6ae Mon Sep 17 00:00:00 2001 From: Ludy Date: Thu, 16 Oct 2025 23:40:08 +0200 Subject: [PATCH] fix(security): harden URL reachability to block SSRF to private/reserved networks and add unit tests (#4637) # Description of Changes - **What was changed** - Introduced a stricter network safety check in `GeneralUtils.isURLReachable(...)`: - Block resolution to local, private, link-local, multicast, and reserved ranges for both IPv4 and IPv6 (including IPv4-mapped and IPv4-compatible IPv6). - Added a DNS records cap (`MAX_DNS_ADDRESSES = 20`) to mitigate DNS answer explosions and reduce SSRF blast radius. - Treat DNS resolution failures as unsafe (fail closed). - Reject empty/invalid hosts early and disallow non-HTTP(S) protocols. - Ensure `HttpURLConnection` is properly disconnected in a `finally` block to avoid resource leaks. - Added comprehensive unit tests in `GeneralUtilsAdditionalTest` to verify blocking of sensitive ranges (e.g., `127.0.0.1`, `10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`, CGNAT `100.64.0.0/10`, link-local `169.254.0.0/16`, TEST-NETs, multicast `224.0.0.0/4`, IPv6 ULA `fc00::/7`, and IPv4-mapped `::ffff:127.0.0.1`). - Renamed and refactored helper logic to `isDisallowedNetworkLocation(...)` and split out `isSensitiveAddress(...)`, `isPrivateOrReservedIPv4(...)`, `isUniqueLocalIPv6(...)`, and `isIPv4MappedAddress(...)` for clarity and testability. - **Why the change was made** - To prevent Server-Side Request Forgery (SSRF) and related abuses in features that fetch external URLs (e.g., website-to-PDF and similar utilities). - Ensures the application cannot be coerced into contacting internal infrastructure or special-purpose address spaces. - Adds explicit resource cleanup and safer defaults (fail closed) to improve reliability and security. --- ## Checklist ### General - [x] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [x] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/DeveloperGuide.md) (if applicable) - [ ] I have read the [How to add new languages to Stirling-PDF](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md) (if applicable) - [x] I have performed a self-review of my own code - [x] My changes generate no new warnings ### Documentation - [ ] I have updated relevant docs on [Stirling-PDF's doc repo](https://github.com/Stirling-Tools/Stirling-Tools.github.io/blob/main/docs/) (if functionality has heavily changed) - [ ] I have read the section [Add New Translation Tags](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md#add-new-translation-tags) (for new translation tags only) ### UI Changes (if applicable) - [ ] Screenshots or videos demonstrating the UI changes are attached (e.g., as comments or direct attachments in the PR) ### Testing (if applicable) - [ ] I have tested my changes locally. Refer to the [Testing Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/DeveloperGuide.md#6-testing) for more details. --- .../software/common/util/GeneralUtils.java | 214 +++++++++++++++--- .../util/GeneralUtilsAdditionalTest.java | 15 ++ 2 files changed, 201 insertions(+), 28 deletions(-) diff --git a/app/common/src/main/java/stirling/software/common/util/GeneralUtils.java b/app/common/src/main/java/stirling/software/common/util/GeneralUtils.java index 10ac8b595..110b94ecf 100644 --- a/app/common/src/main/java/stirling/software/common/util/GeneralUtils.java +++ b/app/common/src/main/java/stirling/software/common/util/GeneralUtils.java @@ -33,6 +33,9 @@ import stirling.software.common.configuration.InstallationPathConfig; @UtilityClass public class GeneralUtils { + /** Maximum number of resolved DNS addresses allowed for a host before it is considered unsafe. */ + private static final int MAX_DNS_ADDRESSES = 20; + private final Set DEFAULT_VALID_SCRIPTS = Set.of("png_to_webp.py", "split_photos.py"); private final Set DEFAULT_VALID_PIPELINE = Set.of( @@ -153,7 +156,7 @@ public class GeneralUtils { return matcher.find() ? matcher.replaceFirst("") : filename; } - /* + /** * Append suffix to base name with null safety. * * @param baseName the base filename, null becomes "default" @@ -164,7 +167,7 @@ public class GeneralUtils { return (baseName == null ? "default" : baseName) + (suffix != null ? suffix : ""); } - /* + /** * Generate a PDF filename by removing extension from first file and adding suffix. * *

High-level utility method for common PDF naming scenarios. Handles null safety and uses @@ -179,7 +182,7 @@ public class GeneralUtils { return appendSuffix(baseName, suffix); } - /* + /** * Process a list of filenames by removing extensions and adding suffix. * *

Efficiently processes multiple filenames using streaming operations and bulk operations @@ -200,7 +203,7 @@ public class GeneralUtils { .forEach(processor); } - /* + /** * Extract title from filename by removing extension, with fallback handling. * *

Returns "Untitled" for null or empty filenames, otherwise removes the extension using the @@ -268,6 +271,12 @@ public class GeneralUtils { .getResources(pattern); } + /** + * Validates URL syntax and disallows common-infrastructure targets to reduce SSRF risk. + * + * @param urlStr a URL string to validate + * @return {@code true} if the URL is syntactically valid and allowed; {@code false} otherwise + */ public boolean isValidURL(String urlStr) { try { Urls.create( @@ -278,7 +287,7 @@ public class GeneralUtils { } } - /* + /** * Checks if a URL is reachable with proper timeout configuration and error handling. * * @param urlStr the URL string to check @@ -288,15 +297,17 @@ public class GeneralUtils { return isURLReachable(urlStr, 5000, 5000); } - /* - * Checks if a URL is reachable with configurable timeouts. + /** + * Checks whether a URL is reachable using configurable timeouts. Only {@code http} and + * {@code https} protocols are permitted, and local/private/multicast ranges are blocked. * - * @param urlStr the URL string to check + * @param urlStr the URL to probe * @param connectTimeout connection timeout in milliseconds * @param readTimeout read timeout in milliseconds - * @return true if URL is reachable, false otherwise + * @return {@code true} if a HEAD request returns a 2xx or 3xx status; {@code false} otherwise */ public boolean isURLReachable(String urlStr, int connectTimeout, int readTimeout) { + HttpURLConnection connection = null; try { // Parse the URL URL url = URI.create(urlStr).toURL(); @@ -307,14 +318,17 @@ public class GeneralUtils { return false; // Disallow other protocols } - // Check if the host is a local address String host = url.getHost(); - if (isLocalAddress(host)) { - return false; // Exclude local addresses + if (host == null || host.isBlank()) { + return false; + } + + if (isDisallowedNetworkLocation(host)) { + return false; // Exclude local, private or otherwise sensitive addresses } // Check if the URL is reachable - HttpURLConnection connection = (HttpURLConnection) url.openConnection(); + connection = (HttpURLConnection) url.openConnection(); connection.setRequestMethod("HEAD"); connection.setConnectTimeout(connectTimeout); connection.setReadTimeout(readTimeout); @@ -325,29 +339,173 @@ public class GeneralUtils { } catch (Exception e) { log.debug("URL {} is not reachable: {}", urlStr, e.getMessage()); return false; // Return false in case of any exception + } finally { + if (connection != null) { + connection.disconnect(); + } } } - private boolean isLocalAddress(String host) { + /** + * Determines whether the specified host resolves to a disallowed network location, such as + * local, private, multicast, or reserved ranges. Excessive DNS results are also blocked. + * + * @param host the hostname to resolve + * @return {@code true} if the host should be considered unsafe + */ + private boolean isDisallowedNetworkLocation(String host) { + // Resolution is delegated to the JVM/OS resolver which already applies system + // configured query limits and timeouts. We only need the resolved addresses here so + // that we can enforce the MAX_DNS_ADDRESSES limit and perform the sensitive range + // checks below. try { - // Resolve DNS to IP address - InetAddress address = InetAddress.getByName(host); - - // Check for local addresses - return address.isAnyLocalAddress() - || // Matches 0.0.0.0 or similar - address.isLoopbackAddress() - || // Matches 127.0.0.1 or ::1 - address.isSiteLocalAddress() - || // Matches private IPv4 ranges: 192.168.x.x, 10.x.x.x, 172.16.x.x to - // 172.31.x.x - address.getHostAddress() - .startsWith("fe80:"); // Matches link-local IPv6 addresses + InetAddress[] addresses = InetAddress.getAllByName(host); + if (addresses.length > MAX_DNS_ADDRESSES) { + log.debug( + "Blocking URL to host {} due to excessive DNS records (>{})", + host, + MAX_DNS_ADDRESSES); + return true; + } + for (InetAddress address : addresses) { + if (address == null || isSensitiveAddress(address)) { + log.debug("Blocking URL to host {} resolved to {}", host, address); + return true; + } + } + return false; } catch (Exception e) { - return false; // Return false for invalid or unresolved addresses + log.debug("Unable to resolve host {}: {}", host, e.getMessage()); + return true; // Treat resolution issues as unsafe to avoid SSRF } } + /** + * Returns whether the given IP address lies within ranges that should not be contacted by the + * server (loopback, link-local, private, multicast, etc.). IPv6 ULA and IPv4-mapped addresses + * are handled. + * + * @param address the resolved address + * @return {@code true} if the address is considered sensitive + */ + private boolean isSensitiveAddress(InetAddress address) { + if (address.isAnyLocalAddress() + || address.isLoopbackAddress() + || address.isLinkLocalAddress() + || address.isSiteLocalAddress() + || address.isMulticastAddress()) { + return true; + } + + byte[] rawAddress = address.getAddress(); + if (address instanceof Inet4Address) { + return isPrivateOrReservedIPv4(rawAddress); + } + + if (address instanceof Inet6Address inet6Address) { + if (isUniqueLocalIPv6(rawAddress)) { + return true; + } + if (isIPv4MappedAddress(rawAddress) || inet6Address.isIPv4CompatibleAddress()) { + byte[] ipv4 = + Arrays.copyOfRange(rawAddress, rawAddress.length - 4, rawAddress.length); + return isPrivateOrReservedIPv4(ipv4); + } + } + + return false; + } + + /** + * Checks whether an IPv4 address is private or reserved. Any malformed input defaults to + * {@code true} (conservative) to avoid misuse. + * + * @param address 4-byte IPv4 address + * @return {@code true} if private/reserved + */ + private boolean isPrivateOrReservedIPv4(byte[] address) { + // IPv4 addresses must be exactly 4 bytes. Treat null or unexpected lengths as + // sensitive to avoid processing malformed input. + if (address == null || address.length != 4) { + return true; + } + + int first = Byte.toUnsignedInt(address[0]); + int second = Byte.toUnsignedInt(address[1]); + + if (first == 0 || first == 127) { + return true; // 0.0.0.0/8 and 127.0.0.0/8 + } + if (first == 100 && second >= 64 && second <= 127) { + return true; // 100.64.0.0/10 Carrier-grade NAT + } + if (first == 169 && second == 254) { + return true; // 169.254.0.0/16 Link-local + } + if (first == 172 && second >= 16 && second <= 31) { + return true; // 172.16.0.0/12 Private + } + if (first == 192 && second == 0 && Byte.toUnsignedInt(address[2]) == 0) { + return true; // 192.0.0.0/24 IETF Protocol Assignments + } + if (first == 192 && second == 0 && Byte.toUnsignedInt(address[2]) == 2) { + return true; // 192.0.2.0/24 TEST-NET-1 + } + if (first == 192 && second == 168) { + return true; // 192.168.0.0/16 Private + } + if (first == 198 && (second == 18 || second == 19)) { + return true; // 198.18.0.0/15 Benchmark tests + } + if (first == 198 && second == 51 && Byte.toUnsignedInt(address[2]) == 100) { + return true; // 198.51.100.0/24 TEST-NET-2 + } + if (first == 203 && second == 0 && Byte.toUnsignedInt(address[2]) == 113) { + return true; // 203.0.113.0/24 TEST-NET-3 + } + if (first == 10) { + return true; // 10.0.0.0/8 Private + } + if (first >= 224) { + return true; // 224.0.0.0/4 Multicast and 240.0.0.0/4 Reserved for future use + } + return false; + } + + /** + * Checks whether an IPv6 address is a Unique Local Address (ULA, fc00::/7). Any malformed input + * defaults to {@code true} (conservative) to avoid misuse. + * + * @param address 16-byte IPv6 address + * @return {@code true} if ULA + */ + private boolean isUniqueLocalIPv6(byte[] address) { + if (address == null || address.length != 16) { + return true; + } + int first = Byte.toUnsignedInt(address[0]); + return (first & 0xFE) == 0xFC; // fc00::/7 Unique local addresses + } + + /** + * Checks whether an IPv6 address is an IPv4-mapped address (::ffff:0:0/96). Any malformed + * input defaults to {@code false} (conservative) to avoid misuse. + * + * @param address 16-byte IPv6 address + * @return {@code true} if IPv4-mapped + */ + private boolean isIPv4MappedAddress(byte[] address) { + if (address == null || address.length != 16) { + return false; + } + for (int i = 0; i < 10; i++) { + if (address[i] != 0) { + return false; + } + } + return address[10] == (byte) 0xFF && address[11] == (byte) 0xFF; + } + /* * Improved multipart file conversion using the shared helper method. * diff --git a/app/common/src/test/java/stirling/software/common/util/GeneralUtilsAdditionalTest.java b/app/common/src/test/java/stirling/software/common/util/GeneralUtilsAdditionalTest.java index 3ecc6fac5..ac913353b 100644 --- a/app/common/src/test/java/stirling/software/common/util/GeneralUtilsAdditionalTest.java +++ b/app/common/src/test/java/stirling/software/common/util/GeneralUtilsAdditionalTest.java @@ -29,6 +29,21 @@ class GeneralUtilsAdditionalTest { assertTrue(GeneralUtils.isValidURL("https://example.com")); assertFalse(GeneralUtils.isValidURL("htp:/bad")); assertFalse(GeneralUtils.isURLReachable("http://localhost")); + assertFalse(GeneralUtils.isURLReachable("http://0.0.0.0")); + assertFalse(GeneralUtils.isURLReachable("http://192.168.1.1")); + assertFalse(GeneralUtils.isURLReachable("http://169.254.0.1")); + assertFalse(GeneralUtils.isURLReachable("http://172.16.0.1")); + assertFalse(GeneralUtils.isURLReachable("http://192.0.2.1")); + assertFalse(GeneralUtils.isURLReachable("http://192.0.0.0")); + assertFalse(GeneralUtils.isURLReachable("http://192.168.0.0")); + assertFalse(GeneralUtils.isURLReachable("http://198.18.0.1")); + assertFalse(GeneralUtils.isURLReachable("http://198.51.100.0")); + assertFalse(GeneralUtils.isURLReachable("http://203.0.113.0")); + assertFalse(GeneralUtils.isURLReachable("http://10.0.0.0")); + assertFalse(GeneralUtils.isURLReachable("http://100.64.0.1")); + assertFalse(GeneralUtils.isURLReachable("http://224.0.0.0")); + assertFalse(GeneralUtils.isURLReachable("http://[::ffff:127.0.0.1]/")); + assertFalse(GeneralUtils.isURLReachable("http://[fd12:3456:789a::1]/")); assertFalse(GeneralUtils.isURLReachable("ftp://example.com")); assertTrue(GeneralUtils.isValidUUID("123e4567-e89b-12d3-a456-426614174000"));