yuqi1129 commented on code in PR #11354:
URL: https://github.com/apache/gravitino/pull/11354#discussion_r3354196409
##########
core/src/main/java/org/apache/gravitino/job/JobManager.java:
##########
@@ -830,6 +832,57 @@ static String fetchFileFromUri(String uri, File
stagingDir, int timeoutInMs) {
}
}
+ /**
+ * Resolves the host in the given URI and rejects addresses that should not
be reachable from the
+ * server (loopback, link-local, RFC-1918 private ranges, IPv6 ULA, cloud
metadata endpoints).
+ * This is a defence-in-depth measure against Server-Side Request Forgery
(SSRF).
+ *
+ * <p><b>Note on DNS TOCTOU:</b> This method resolves the hostname once at
validation time. {@code
+ * FileUtils.copyURLToFile()} will re-resolve it when opening the
connection, so a precisely-timed
+ * DNS-rebinding attack could theoretically bypass this check. Complete
protection against DNS
+ * rebinding requires network-level egress controls (e.g. firewall rules) in
addition to this
+ * application-layer validation.
+ */
+ @VisibleForTesting
+ static void validateRemoteUri(URI uri) throws IOException {
+ String host = uri.getHost();
+ if (host == null) {
+ throw new IllegalArgumentException("URI has no host: " + uri);
+ }
+ InetAddress[] addresses = InetAddress.getAllByName(host);
+ for (InetAddress address : addresses) {
+ if (isBlockedAddress(address)) {
+ throw new IllegalArgumentException(
+ String.format(
+ "URI '%s' resolves to blocked address %s, access denied (SSRF
prevention)",
+ uri, address.getHostAddress()));
+ }
+ }
+ }
+
+ private static boolean isBlockedAddress(InetAddress address) {
+ // Covers loopback (127.x.x.x / ::1), link-local (169.254.x.x / fe80::/10
— includes AWS/GCP/
+ // Azure metadata), RFC-1918 private (10.x / 172.16-31.x / 192.168.x),
multicast, unspecified.
+ if (address.isLoopbackAddress()
+ || address.isLinkLocalAddress()
+ || address.isSiteLocalAddress()
+ || address.isMulticastAddress()
+ || address.isAnyLocalAddress()) {
+ return true;
Review Comment:
The main risk is SSRF. `localhost` or a private IP is resolved from the
Gravitino server side, not from the user's machine.
So if a user passes `http://127.0.0.1:xxx` or `http://192.168.x.x`,
Gravitino will try to access services on the Gravitino server itself or inside
the server's internal network. This may expose local admin services, internal
APIs, or cloud metadata endpoints.
I agree that some deployments may want to fetch files from a local HTTP/FTP
server. But I think this should be disabled by default, and only enabled
explicitly by an admin, for example with an allowlist/configuration. If we
reach an agreement on this point, I can do as it is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
