github-actions[bot] commented on code in PR #65987:
URL: https://github.com/apache/doris/pull/65987#discussion_r3643471064
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/JdbcResource.java:
##########
@@ -304,28 +307,113 @@ public static String getFullDriverUrl(String driverUrl)
throws IllegalArgumentEx
+ "file://xxx.jar, http://xxx.jar, https://xxx.jar, or
xxx.jar (without prefix).");
}
+ URI uri;
try {
- URI uri = new URI(driverUrl);
- String schema = uri.getScheme();
- checkCloudWhiteList(driverUrl);
- if (schema == null && !driverUrl.startsWith("/")) {
- return checkAndReturnDefaultDriverUrl(driverUrl);
- }
+ uri = new URI(driverUrl);
+ } catch (URISyntaxException e) {
+ // Fail closed: an unparsable URL must never be silently accepted,
otherwise the
+ // allowed-path check below could be bypassed by a malformed URL.
+ LOG.warn("invalid jdbc driver url: {}", driverUrl, e);
+ throw new IllegalArgumentException("Invalid driver URL: " +
driverUrl);
+ }
+
+ String schema = uri.getScheme();
+ checkCloudWhiteList(driverUrl);
+ if (schema == null && !driverUrl.startsWith("/")) {
+ return checkAndReturnDefaultDriverUrl(driverUrl);
Review Comment:
[P1] Enforce the bare-name rule in the shared resolver
The scheme-less branch still accepts encoded separators before it reaches
the new containment check. For example, `%2e%2e%2Fevil.jar` matches
`^[^:/]+\.jar$`; with a custom `jdbc_drivers_dir=/opt/doris/jdbc_drivers`, this
returns `file:///opt/doris/jdbc_drivers/%2e%2e%2Fevil.jar`, which JDK 17
decodes and opens as `/opt/doris/evil.jar`. The SPI JDBC provider happens to
reject `%`, but Iceberg/Paimon JDBC metastore loaders and legacy JDBC consumers
call `getFullDriverUrl` directly, so they bypass that rule. Apply the safe
bare-filename grammar (or decoded containment) here before resolving, and cover
a direct consumer. This is distinct from the existing encoded `file://` thread
because this input returns earlier and never reaches `toLocalPath`.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/JdbcResource.java:
##########
@@ -304,28 +307,113 @@ public static String getFullDriverUrl(String driverUrl)
throws IllegalArgumentEx
+ "file://xxx.jar, http://xxx.jar, https://xxx.jar, or
xxx.jar (without prefix).");
}
+ URI uri;
try {
- URI uri = new URI(driverUrl);
- String schema = uri.getScheme();
- checkCloudWhiteList(driverUrl);
- if (schema == null && !driverUrl.startsWith("/")) {
- return checkAndReturnDefaultDriverUrl(driverUrl);
- }
+ uri = new URI(driverUrl);
+ } catch (URISyntaxException e) {
+ // Fail closed: an unparsable URL must never be silently accepted,
otherwise the
+ // allowed-path check below could be bypassed by a malformed URL.
+ LOG.warn("invalid jdbc driver url: {}", driverUrl, e);
+ throw new IllegalArgumentException("Invalid driver URL: " +
driverUrl);
+ }
+
+ String schema = uri.getScheme();
+ checkCloudWhiteList(driverUrl);
+ if (schema == null && !driverUrl.startsWith("/")) {
+ return checkAndReturnDefaultDriverUrl(driverUrl);
+ }
+
+ // "*" or an empty/blank value means allow all (the documented,
backward-compatible contract).
+ String securePath = Config.jdbc_driver_secure_path;
+ if (securePath == null || securePath.trim().isEmpty() ||
"*".equals(securePath.trim())) {
+ return driverUrl;
+ }
- if ("*".equals(Config.jdbc_driver_secure_path)) {
- return driverUrl;
+ if (!isDriverUrlAllowed(driverUrl, uri)) {
+ throw new IllegalArgumentException("Driver URL does not match any
allowed paths: " + driverUrl);
+ }
+ return driverUrl;
+ }
+
+ /**
+ * Check whether {@code driverUrl} falls under one of the
semicolon-separated prefixes configured in
+ * {@link Config#jdbc_driver_secure_path}. Matching is structural
(component-based) rather than a raw string
+ * prefix, so that neither prefix confusion ({@code /opt/drivers} vs
{@code /opt/drivers-evil}) nor path
+ * traversal ({@code /opt/drivers/../etc}) can slip a driver outside the
allowed location.
+ */
+ private static boolean isDriverUrlAllowed(String driverUrl, URI uri) {
+ String scheme = uri.getScheme();
+ List<String> allowedPaths = new ArrayList<>();
+ for (String p : Config.jdbc_driver_secure_path.split(";")) {
+ String trimmed = p.trim();
+ if (!trimmed.isEmpty()) {
+ allowedPaths.add(trimmed);
}
+ }
+ if ("http".equalsIgnoreCase(scheme) ||
"https".equalsIgnoreCase(scheme)) {
+ URI candidate = uri.normalize();
+ return allowedPaths.stream().anyMatch(allowed ->
remoteUrlMatches(candidate, allowed));
+ }
+ // Only file:// reaches here; bare absolute paths and bare "*.jar" are
handled earlier.
+ Path candidate = toLocalPath(driverUrl).normalize();
+ return allowedPaths.stream()
+ .map(allowed -> toLocalPath(allowed).normalize())
+ .anyMatch(candidate::startsWith);
+ }
- boolean isAllowed =
Arrays.stream(Config.jdbc_driver_secure_path.split(";"))
- .anyMatch(allowedPath ->
driverUrl.startsWith(allowedPath.trim()));
- if (!isAllowed) {
- throw new IllegalArgumentException("Driver URL does not match
any allowed paths: " + driverUrl);
+ /**
+ * Turn a {@code file://} URL or a plain filesystem path into a {@link
Path} for structural comparison.
+ * A {@code file://} URL is decoded exactly once via {@link URI#getPath()}
so that percent-encoded
+ * segments (e.g. {@code %2e%2e}) are resolved into the same
representation the driver-loading
+ * consumers ({@code URL.openStream} / {@code URLClassLoader}) will use;
otherwise an encoded parent
+ * segment would survive normalization and escape the allowed directory.
+ */
+ private static Path toLocalPath(String pathOrUrl) {
+ if (pathOrUrl.startsWith("file:")) {
+ try {
+ String decoded = new URI(pathOrUrl).getPath();
Review Comment:
[P1] Preserve non-path components of file URIs
This conversion validates only `URI.getPath()` but returns the original URL,
so an authority or query can make consumers use a different object. With a
local allowlist, `file://attacker.example/opt/doris/jdbc_drivers/evil.jar`
path-matches, yet JDK 17 opens it through FTP for checksum;
`Util.getInputStreamFromUrl` also skips SSRF checking for every `file://`
string. Separately, with allowed `file:///opt/doris/jdbc_drivers/`, the
candidate `file:///opt/doris/jdbc_drivers?evil.jar` path-matches; checksum
reads the allowed directory, while JDK 17's `FileURLMapper` used by
`URLClassLoader` maps the original URL to the sibling
`/opt/doris/jdbc_drivers?evil.jar`. The old raw-prefix check rejected both
forms. Reject non-local authorities and queries (or preserve and compare them)
so validation, checksum, and classloading address the same object, and add both
negative tests.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
