github-actions[bot] commented on code in PR #65987:
URL: https://github.com/apache/doris/pull/65987#discussion_r3650312226
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/JdbcResource.java:
##########
@@ -304,28 +307,128 @@ 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("/")) {
+ // A scheme-less driver_url is a plain jar file name resolved
under jdbc_drivers_dir. This
+ // shared resolver is also on the lazy load path of pre-existing
catalogs (Iceberg/Paimon/
+ // legacy JDBC consumers call it directly, with no create/alter or
replay context), so it
+ // deliberately applies no new restriction here: an unmodified
historical catalog must keep
+ // resolving exactly as before. The mandatory bare-name grammar is
enforced only when a
+ // catalog is created or altered, in
JdbcDorisConnector.checkDriverUrlSecurityRule.
+ 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.
+ // A local file URL must carry no authority, query or fragment.
Otherwise validation (which
+ // looks only at URI.getPath()) and the consumers (URLClassLoader /
checksum, which act on the
+ // whole original URL) would address different objects — e.g.
"file://attacker/dir/x.jar" is
+ // fetched from a remote authority, and "file:///dir/x.jar?evil" maps
to a sibling file.
+ String authority = uri.getRawAuthority();
+ if ((authority != null && !authority.isEmpty())
+ || uri.getRawQuery() != null || uri.getRawFragment() != null) {
+ return false;
+ }
+ 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();
+ if (decoded != null && !decoded.isEmpty()) {
+ return Paths.get(decoded);
+ }
+ } catch (URISyntaxException ignored) {
+ // fall through to literal stripping below
}
- return driverUrl;
+ int sep = pathOrUrl.indexOf("//");
+ return Paths.get(sep >= 0 ? pathOrUrl.substring(sep + 2) :
pathOrUrl.substring("file:".length()));
+ }
+ return Paths.get(pathOrUrl);
+ }
+
+ /**
+ * Structural match for remote (http/https) driver URLs: scheme, host and
port must be equal, and the
+ * candidate path must sit under the allowed path (component-based). A
bare path prefix (no scheme) can
+ * never authorize a remote URL.
+ */
+ private static boolean remoteUrlMatches(URI candidate, String allowedPath)
{
+ URI base;
+ try {
+ base = new URI(allowedPath).normalize();
} catch (URISyntaxException e) {
- LOG.warn("invalid jdbc driver url: " + driverUrl);
- return driverUrl;
+ return false;
}
+ if (base.getScheme() == null) {
+ return false;
+ }
+ // Scheme/host/port and the path prefix must match, and the
resource-selecting components
+ // (user-info and query) that the checksum/classloader consumers act
on must match exactly too,
+ // otherwise e.g. ".../download?id=approved" would authorize
".../download?id=evil".
+ return base.getScheme().equalsIgnoreCase(candidate.getScheme())
+ && base.getHost() != null &&
base.getHost().equalsIgnoreCase(candidate.getHost())
Review Comment:
[P1] Preserve resolvable authorities that `URI.getHost()` cannot parse
On JDK 17, `new URI("http://jdbc_repo.internal/drivers/x.jar";).getHost()`
returns `null` even though `URI.toURL()` retains that host, and both
`HttpURLConnection` and `URLClassLoader` can fetch it when the internal name
resolves. Thus even when `jdbc_driver_secure_path` and `driver_url` use the
identical authority, this guard returns false. The old raw-prefix check
accepted that persisted pair, so CREATE and lazy Iceberg/Paimon/legacy
initialization can fail after upgrade. Please compare a safely decomposed
authority representation that the URL consumers accept—without a blind
raw-authority fallback—while retaining user-info and effective-port checks,
with CREATE and replayed direct-consumer tests for an underscore-bearing host.
--
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]
