Harden IMPORT DATABASE source validation and require admin privilege#4422
Conversation
- Require the administrative updateSecurity permission to run IMPORT DATABASE via SQL (no-op in embedded mode without security configured). - Validate the import source in SourceDiscovery: optional block of HTTP(S) URLs that resolve to loopback/link-local/private/wildcard/multicast addresses (arcadedb.server.security.importBlockLocalNetworks, default true) and an optional local-path allow-list for file:// and plain paths (arcadedb.server.security.importAllowedLocalPaths, default unrestricted). - Disable DTD processing and external entities in the XML importer. - Surface security violations as HTTP 403. Adds unit and integration regression tests.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 24 |
🟢 Coverage 88.46% diff coverage · -7.38% coverage variation
Metric Results Coverage variation ✅ -7.38% coverage variation Diff coverage ✅ 88.46% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (9945018) 128194 94278 73.54% Head commit (e1aedc7) 159951 (+31757) 105827 (+11549) 66.16% (-7.38%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#4422) 52 46 88.46% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces security hardening for the IMPORT DATABASE command to mitigate Server-Side Request Forgery (SSRF), arbitrary local file reads, and XML External Entity (XXE) vulnerabilities. It adds configuration options to block local networks and restrict local import paths, enforces administrative privilege checks, and disables DTD processing in the XML importer. The review feedback highlights two security improvements: addressing a gap in IPv6 private address validation where Unique Local Addresses (ULA) are not blocked by isSiteLocalAddress(), and mitigating a potential DNS rebinding (TOCTOU) vulnerability by pinning the resolved IP address during connection establishment.
| static boolean isBlockedAddress(final InetAddress address) { | ||
| return address.isLoopbackAddress() // 127.0.0.0/8, ::1 | ||
| || address.isLinkLocalAddress() // 169.254.0.0/16 (cloud metadata), fe80::/10 | ||
| || address.isSiteLocalAddress() // 10/8, 172.16/12, 192.168/16 | ||
| || address.isAnyLocalAddress() // 0.0.0.0, :: | ||
| || address.isMulticastAddress(); // 224.0.0.0/4, ff00::/8 | ||
| } |
There was a problem hiding this comment.
The isBlockedAddress method uses InetAddress.isSiteLocalAddress() to identify and block private network addresses. However, in Java, isSiteLocalAddress() only recognizes the deprecated IPv6 site-local prefix (fec0::/10) and does not return true for Unique Local Addresses (ULA, fc00::/7 / fd00::/8), which are the modern standard for private IPv6 networks. This allows an attacker to bypass the SSRF protection if the target environment uses IPv6 ULAs.\n\nTo fix this, explicitly check if the address is an Inet6Address and belongs to the fc00::/7 range.
static boolean isBlockedAddress(final InetAddress address) {\n if (address instanceof java.net.Inet6Address) {\n final byte[] bytes = address.getAddress();\n if ((bytes[0] & 0xfe) == 0xfc) {\n return true;\n }\n }\n return address.isLoopbackAddress() // 127.0.0.0/8, ::1\n || address.isLinkLocalAddress() // 169.254.0.0/16 (cloud metadata), fe80::/10\n || address.isSiteLocalAddress() // 10/8, 172.16/12, 192.168/16\n || address.isAnyLocalAddress() // 0.0.0.0, ::\n || address.isMulticastAddress(); // 224.0.0.0/4, ff00::/8\n }| public static void validateRemoteURL(final String url) { | ||
| if (!GlobalConfiguration.SERVER_SECURITY_IMPORT_BLOCK_LOCAL_NETWORKS.getValueAsBoolean()) | ||
| return; | ||
|
|
||
| final int sep = url.lastIndexOf(RESOURCE_SEPARATOR); | ||
| final String urlPath = sep > -1 ? url.substring(0, sep) : url; | ||
|
|
||
| final String host; | ||
| try { | ||
| host = new URL(urlPath).getHost(); | ||
| } catch (final MalformedURLException e) { | ||
| throw new SecurityException("IMPORT DATABASE: malformed URL"); | ||
| } | ||
|
|
||
| if (host == null || host.isEmpty()) | ||
| throw new SecurityException("IMPORT DATABASE: URL is missing a host"); | ||
|
|
||
| final InetAddress[] addresses; | ||
| try { | ||
| addresses = InetAddress.getAllByName(host); | ||
| } catch (final UnknownHostException e) { | ||
| throw new SecurityException("IMPORT DATABASE: cannot resolve host '" + host + "'"); | ||
| } | ||
|
|
||
| for (final InetAddress address : addresses) { | ||
| if (isBlockedAddress(address)) | ||
| throw new SecurityException("IMPORT DATABASE: access to host '" + host + "' (" + address.getHostAddress() | ||
| + ") is blocked because it resolves to a private/local network address. Set '" | ||
| + GlobalConfiguration.SERVER_SECURITY_IMPORT_BLOCK_LOCAL_NETWORKS.getKey() | ||
| + "=false' to allow imports from internal hosts in trusted environments"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The validateRemoteURL method resolves the hostname to validate that it does not point to a private/local network address. However, the actual connection is established later in SourceDiscovery.getSourceFromURL using new URL(urlPath).openConnection(). This creates a Time-of-Check to Time-of-Use (TOCTOU) vulnerability, commonly known as DNS Rebinding.\n\nAn attacker can configure a malicious DNS server to return a safe public IP during the validation phase, and then return a blocked private IP (e.g., 127.0.0.1 or 169.254.169.254) when the connection is opened.\n\nTo fully mitigate this, consider pinning the resolved IP address by using a custom SocketFactory or a custom Dns resolver for the HTTP client, ensuring that the connection is made to the exact IP address that was validated.
Code ReviewSolid security hardening PR. The layered approach (privilege gate + SSRF validation + local path allow-list + XXE hardening) is the right architecture, and the test coverage is good. A few observations below. Potential IssuesDNS rebinding (SSRF residual risk)
IPv6 unique-local addresses ( Java's Overly broad for (Throwable cause = e.getTargetException(); cause != null; cause = cause.getCause())
if (cause instanceof SecurityException se)
throw se;
Minor PointsAll-caps comments ( Missing positive test for admin user - Temp file leak in
What's Done Well
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4422 +/- ##
============================================
+ Coverage 64.45% 64.48% +0.02%
+ Complexity 431 428 -3
============================================
Files 1648 1649 +1
Lines 128194 128245 +51
Branches 27477 27492 +15
============================================
+ Hits 82630 82696 +66
+ Misses 33948 33931 -17
- Partials 11616 11618 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
Hardens the SQL
IMPORT DATABASEstatement and its underlying source discovery.IMPORT DATABASEnow requires the administrativeupdateSecuritypermission. This is a no-op in embedded mode (no security configured), so library/CLI usage is unaffected.SourceDiscovery.getSource(), the single chokepoint for both branches):arcadedb.server.security.importBlockLocalNetworks(defaulttrue). Public hosts are unaffected.file://and plain local paths can be restricted to an allow-list viaarcadedb.server.security.importAllowedLocalPaths(default empty = unrestricted, backward compatible). Canonicalized to defeat..traversal;classpath://resources are always allowed.EXPORT DATABASE/BACKUP DATABASEwere reviewed and left unchanged: they already contain their output paths (reject../separators, force the exports/backup directory).Tests
ImportSecurityValidatorTest(unit, offline) - SSRF host blocking and local-path allow-list.ImportDatabaseSecurityIT(server) - a non-admin user is rejected with HTTP 403.XMLImporterFormatTest.SQLLocalImporterIT,SQLRemoteImporterIT,DatabaseAdminStatementsTest,ServerImportDatabaseIT,CSVImporterIT,JsonLImporterIT.Note
SSRF blocking defaults to on, including embedded mode; environments that legitimately import from internal hosts can set
arcadedb.server.security.importBlockLocalNetworks=false.