CalvinKirs commented on code in PR #65987:
URL: https://github.com/apache/doris/pull/65987#discussion_r3649461518
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/JdbcResource.java:
##########
@@ -297,35 +301,142 @@ private static void checkCloudWhiteList(String
driverUrl) throws IllegalArgument
}
}
+ // A scheme-less driver_url must be a plain jar file name: letters,
digits, dot, underscore, hyphen.
+ // No path separator (encoded or not) and no other characters, so it can
never escape jdbc_drivers_dir.
+ private static final Pattern SAFE_BARE_JAR_NAME =
Pattern.compile("^[A-Za-z0-9._-]+\\.jar$");
+
public static String getFullDriverUrl(String driverUrl) throws
IllegalArgumentException {
if (!(driverUrl.startsWith("file://") ||
driverUrl.startsWith("http://";)
|| driverUrl.startsWith("https://";) ||
driverUrl.matches("^[^:/]+\\.jar$"))) {
throw new IllegalArgumentException("Invalid driver URL format.
Supported formats are: "
+ "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("/")) {
+ // Enforce the safe bare-name grammar in this shared resolver,
before resolving under
+ // jdbc_drivers_dir. Otherwise an encoded separator (e.g.
"%2e%2e%2Fevil.jar") passes the
+ // loose format check above and, once resolved and decoded by the
loader, escapes the
+ // directory. The connector rule catches "%" too, but
Iceberg/Paimon/legacy JDBC consumers
+ // call getFullDriverUrl directly and bypass that rule, so it must
be enforced here.
+ if (!SAFE_BARE_JAR_NAME.matcher(driverUrl).matches()) {
+ throw new IllegalArgumentException(
+ "Invalid driver_url: a driver file name must match
[A-Za-z0-9._-]+.jar: " + driverUrl);
}
+ 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())
Review Comment:
This branch only runs when an operator sets a concrete
`jdbc_driver_secure_path`; the default (`*` or empty) means allow-all and is
unaffected, so historical deployments that don't opt into a secure path see no
change.
For deployments that do opt in: `file://localhost/...` is an unusual
spelling for a local file, and we reject non-empty file authorities so that
validation and the loader address the same object. Operators configuring a
secure path should use `file:///...`. We're treating the `localhost` authority
as out of scope by design here.
--
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]
