CalvinKirs commented on code in PR #65987:
URL: https://github.com/apache/doris/pull/65987#discussion_r3649458218
##########
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()) {
Review Comment:
Addressed in acf842e830. The shared resolver `getFullDriverUrl` no longer
applies the strict bare-name grammar. That grammar now runs only on the
create/alter path (`JdbcDorisConnector.checkDriverUrlSecurityRule`, invoked by
`checkProperties` on CREATE and ALTER, both `!isReplay`).
So an unmodified historical catalog with a previously valid name (e.g.
`legacy+patched.jar`) resolves exactly as before on lazy access / replay; only
newly created or altered catalogs are validated against the stricter grammar.
##########
fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcDorisConnector.java:
##########
@@ -125,11 +128,71 @@ public ConnectorScanPlanProvider getScanPlanProvider() {
return scanPlanProvider;
}
+ // A scheme-less driver_url must be a plain jar file name: letters,
digits, dot, underscore, hyphen.
+ // This intentionally forbids any path separator, so it can never escape
jdbc_drivers_dir.
+ private static final Pattern SAFE_DRIVER_FILE_NAME =
Pattern.compile("^[A-Za-z0-9._-]+\\.jar$");
+
+ /**
+ * Mandatory, non-configurable driver_url security rule. It is invoked from
+ * {@link JdbcConnectorProvider#validateProperties} (and from {@link
#preCreateValidation}),
+ * i.e. from the engine's {@code checkProperties()} hook, which runs only
on the user-facing
+ * CREATE / ALTER CATALOG paths (both guarded by {@code !isReplay}).
Therefore the rule never
+ * runs during metadata/edit-log replay nor at query time, so existing
catalogs are unaffected
+ * and FE startup / follower replay can never be blocked by it.
+ *
+ * <p>The rule cannot be turned off:
+ * <ul>
+ * <li>any {@code ..} path-traversal segment is rejected, for {@code
file://} and {@code http(s)} alike;</li>
+ * <li>a scheme-less driver_url must be a bare jar file name matching
{@code [A-Za-z0-9._-]+.jar}
+ * (no directories, no special characters), which is then resolved
under {@code jdbc_drivers_dir}.</li>
+ * </ul>
+ * Whether a remote/absolute URL is allowed at all remains governed by the
fe.conf-only
+ * {@code jdbc_driver_secure_path} / {@code jdbc_driver_url_white_list}
configs; this rule only
+ * forbids traversal and enforces the bare-name charset.
+ *
+ * <p>Throws {@link IllegalArgumentException} so the engine wraps it into
a {@code DdlException}
+ * (and, on ALTER, triggers the property rollback).
+ */
+ public static void checkDriverUrlSecurityRule(String driverUrl) {
+ if (driverUrl == null || driverUrl.isEmpty()) {
+ return;
+ }
+ // Check traversal on the decoded path so percent-encoded segments
(e.g. %2e%2e) — which the
+ // driver-loading consumers decode — cannot slip a ".." past this rule.
+ String pathToCheck = driverUrl;
+ if (driverUrl.contains("://")) {
Review Comment:
`checkDriverUrlSecurityRule` is a cheap, no-I/O syntactic guard (rejects
`..` and non-bare scheme-less names), so it isn't the right place for an SSRF
check.
The SSRF checker is already applied where the driver bytes are actually
fetched: at CREATE, `getFullDriverUrl` is followed by checksum computation,
which reads the jar through `Util.getInputStreamFromUrl` — that wraps
`SecurityChecker.startSSRFChecking()/stopSSRFChecking()` (the impl configured
via `security_checker_class_name`) around the whole read. So a create-time
`http(s)` `driver_url` that resolves to an internal address is already blocked
by the configured checker. Bare-name/local drivers never issue a remote
request. The runtime `URLClassLoader` only ever loads a URL that already passed
that create-time, SSRF-checked fetch, so it doesn't need a second wrap.
If you'd prefer the checker also invoked explicitly at this syntactic layer,
I can add it — but as far as I can tell the effective coverage is already
there. Let me know your preference.
--
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]
