This is an automated email from the ASF dual-hosted git repository. jacopoc pushed a commit to branch release24.09 in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 9734714eb81dc976680178f594994e4885fcd054 Author: Jacopo Cappellato <[email protected]> AuthorDate: Wed Mar 11 08:28:13 2026 +0100 Improved: Enhance data resource validation and permission checks Backported from trunk af8ee514a2 with minor modifications. --- applications/content/servicedef/services_data.xml | 2 + .../ofbiz/content/data/DataResourceWorker.java | 272 ++++++++++++++++++++- framework/security/config/security.properties | 29 +++ 3 files changed, 298 insertions(+), 5 deletions(-) diff --git a/applications/content/servicedef/services_data.xml b/applications/content/servicedef/services_data.xml index c02b4a2123..7140ecfa87 100644 --- a/applications/content/servicedef/services_data.xml +++ b/applications/content/servicedef/services_data.xml @@ -89,6 +89,7 @@ <service name="createDataResourceAndText" engine="java" default-entity-name="DataResource" auth="true" location="org.apache.ofbiz.content.data.DataServices" invoke="createDataResourceAndText"> <description>Create a DataResource and, possibly, ElectronicText or ImageDataResource</description> + <permission-service service-name="genericDataResourcePermission" main-action="CREATE"/> <auto-attributes include="pk" mode="INOUT" optional="true"/> <auto-attributes include="nonpk" mode="IN" optional="true"/> <attribute name="textData" mode="IN" optional="true" type="String" allow-html="safe"/> @@ -99,6 +100,7 @@ <service name="updateDataResourceAndText" engine="java" default-entity-name="DataResource" auth="true" location="org.apache.ofbiz.content.data.DataServices" invoke="updateDataResourceAndText"> <description>Create a DataResource and, possibly, ElectronicText or ImageDataResource</description> + <permission-service service-name="genericDataResourcePermission" main-action="UPDATE"/> <auto-attributes include="pk" mode="IN" optional="true"/> <auto-attributes include="nonpk" mode="IN" optional="true"/> <attribute name="textData" mode="IN" type="String" optional="true" allow-html="safe"/> diff --git a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java index 6e03054547..cc105dd30e 100644 --- a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java +++ b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java @@ -27,13 +27,17 @@ import java.io.InputStream; import java.io.InputStreamReader; import java.io.StringWriter; import java.io.Writer; +import java.net.HttpURLConnection; +import java.net.Inet4Address; +import java.net.Inet6Address; +import java.net.InetAddress; import java.net.URL; import java.net.URLConnection; +import java.net.UnknownHostException; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.StandardOpenOption; -import java.sql.Timestamp; import java.util.Comparator; import java.util.HashMap; import java.util.LinkedList; @@ -53,14 +57,12 @@ import org.apache.commons.fileupload.FileUploadException; import org.apache.commons.fileupload.disk.DiskFileItemFactory; import org.apache.commons.fileupload.servlet.ServletFileUpload; import org.apache.commons.io.IOUtils; +import org.apache.commons.io.input.BoundedInputStream; import org.apache.ofbiz.base.location.FlexibleLocation; import org.apache.ofbiz.base.util.Debug; import org.apache.ofbiz.base.util.FileUtil; import org.apache.ofbiz.base.util.GeneralException; -import org.apache.ofbiz.base.util.StringUtil; -import org.apache.ofbiz.base.util.StringUtil.StringWrapper; import org.apache.ofbiz.base.util.UtilCodec; -import org.apache.ofbiz.base.util.UtilDateTime; import org.apache.ofbiz.base.util.UtilGenerics; import org.apache.ofbiz.base.util.UtilHttp; import org.apache.ofbiz.base.util.UtilIO; @@ -466,6 +468,218 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR return prefix; } + /** + * Checks that the given file is within one of the directories listed in + * {@code content.data.local.file.allowed.paths} (security.properties). + * Use {@code ${ofbiz.home}} as a portable placeholder for the OFBiz home directory. + */ + private static void checkLocalFileAllowList(File file) throws GeneralException { + try { + String canonicalFilePath = file.getCanonicalPath(); + String ofbizHome = System.getProperty("ofbiz.home"); + String allowedPathsStr = UtilProperties.getPropertyValue("security", + "content.data.local.file.allowed.paths", "${ofbiz.home}"); + if (UtilValidate.isNotEmpty(allowedPathsStr)) { + boolean inAllowedPath = false; + for (String allowedPath : allowedPathsStr.split(",")) { + allowedPath = allowedPath.trim().replace("${ofbiz.home}", ofbizHome); + if (UtilValidate.isEmpty(allowedPath)) { + continue; + } + String canonicalAllowedDir = new File(allowedPath).getCanonicalPath(); + if (canonicalFilePath.startsWith(canonicalAllowedDir + File.separator) + || canonicalFilePath.equals(canonicalAllowedDir)) { + inAllowedPath = true; + break; + } + } + if (!inAllowedPath) { + throw new GeneralException("Access to file denied: path is not within an allowed directory"); + } + } + } catch (IOException e) { + throw new GeneralException("Unable to validate file path: " + e.getMessage()); + } + } + + /** + * Checks that the given file is within the OFBiz home directory and within one of the + * subdirectories listed in {@code content.data.ofbiz.file.allowed.paths} (security.properties). + */ + private static void checkOfbizFileAllowList(File file) throws GeneralException { + try { + String canonicalHome = new File(System.getProperty("ofbiz.home")).getCanonicalPath(); + String canonicalFilePath = file.getCanonicalPath(); + if (!canonicalFilePath.startsWith(canonicalHome + File.separator)) { + throw new GeneralException("Access to file denied: path resolves outside of the OFBiz home directory"); + } + String allowedPathsStr = UtilProperties.getPropertyValue("security", + "content.data.ofbiz.file.allowed.paths", "applications/,themes/,plugins/,runtime/"); + if (UtilValidate.isNotEmpty(allowedPathsStr)) { + boolean inAllowedPath = false; + for (String relPath : allowedPathsStr.split(",")) { + relPath = relPath.trim().replaceAll("^/+", ""); + if (UtilValidate.isEmpty(relPath)) { + continue; + } + String canonicalAllowedDir = new File(canonicalHome, relPath).getCanonicalPath(); + if (canonicalFilePath.startsWith(canonicalAllowedDir + File.separator) + || canonicalFilePath.equals(canonicalAllowedDir)) { + inAllowedPath = true; + break; + } + } + if (!inAllowedPath) { + throw new GeneralException("Access to file denied: path is not within an allowed directory"); + } + } + } catch (IOException e) { + throw new GeneralException("Unable to validate file path: " + e.getMessage()); + } + } + + /** + * Checks that the given file is within the provided context root directory. + */ + private static void checkContextFileBoundary(File file, String contextRoot) throws GeneralException { + try { + String canonicalAllowed = new File(contextRoot).getCanonicalPath(); + String canonicalFilePath = file.getCanonicalPath(); + if (!canonicalFilePath.startsWith(canonicalAllowed + File.separator)) { + throw new GeneralException("Access to file denied: path resolves outside of the allowed directory"); + } + } catch (IOException e) { + throw new GeneralException("Unable to validate file path: " + e.getMessage()); + } + } + + /** + * Validates a URL for the URL_RESOURCE data type against SSRF (Server-Side Request Forgery) + * attacks. Enforces: + * <ul> + * <li>Protocol restricted to http/https only</li> + * <li>Host must match the configured allow-list when + * {@code content.data.url.resource.allowed.hosts} (security.properties) is non-empty; + * both exact and subdomain matches are supported</li> + * <li>All resolved IP addresses must not be private, loopback, link-local, multicast, + * or otherwise reserved (mitigates DNS-rebinding)</li> + * </ul> + */ + private static void checkUrlResourceAllowed(URL url) throws GeneralException { + // 1. Protocol: only http and https are permitted + String protocol = url.getProtocol(); + if (!"http".equalsIgnoreCase(protocol) && !"https".equalsIgnoreCase(protocol)) { + throw new GeneralException("URL_RESOURCE only supports http/https protocols; rejected: " + protocol); + } + String host = url.getHost(); + if (UtilValidate.isEmpty(host)) { + throw new GeneralException("URL_RESOURCE URL has no host component"); + } + + // 2. Allow-list: if configured, the host must match one of the entries + String allowedHostsStr = UtilProperties.getPropertyValue("security", + "content.data.url.resource.allowed.hosts", ""); + if (UtilValidate.isNotEmpty(allowedHostsStr)) { + String lcHost = host.toLowerCase(Locale.ROOT); + boolean hostAllowed = false; + for (String entry : allowedHostsStr.split(",")) { + String allowedEntry = entry.trim().toLowerCase(Locale.ROOT); + if (UtilValidate.isEmpty(allowedEntry)) { + continue; + } + if (lcHost.equals(allowedEntry) || lcHost.endsWith("." + allowedEntry)) { + hostAllowed = true; + break; + } + } + if (!hostAllowed) { + throw new GeneralException("URL_RESOURCE host is not in the allowed list: " + host); + } + } + + // 3. DNS resolution: block private/reserved IP ranges (SSRF / DNS-rebinding mitigation) + InetAddress[] addresses; + try { + addresses = InetAddress.getAllByName(host); + } catch (UnknownHostException e) { + throw new GeneralException("URL_RESOURCE host cannot be resolved: " + host); + } + if (addresses == null || addresses.length == 0) { + throw new GeneralException("URL_RESOURCE host resolved to no addresses: " + host); + } + for (InetAddress addr : addresses) { + checkNotPrivateOrReservedAddress(addr); + } + } + + /** + * Throws {@link GeneralException} if {@code addr} belongs to a private, loopback, + * link-local, multicast, or otherwise reserved IP range (IPv4 and IPv6). + */ + private static void checkNotPrivateOrReservedAddress(InetAddress addr) throws GeneralException { + if (addr.isLoopbackAddress()) { + throw new GeneralException("URL_RESOURCE target resolves to a loopback address: " + addr.getHostAddress()); + } + if (addr.isLinkLocalAddress()) { + throw new GeneralException("URL_RESOURCE target resolves to a link-local address: " + addr.getHostAddress()); + } + if (addr.isSiteLocalAddress()) { + throw new GeneralException("URL_RESOURCE target resolves to a private (site-local) address: " + addr.getHostAddress()); + } + if (addr.isAnyLocalAddress()) { + throw new GeneralException("URL_RESOURCE target resolves to a wildcard address: " + addr.getHostAddress()); + } + if (addr.isMulticastAddress()) { + throw new GeneralException("URL_RESOURCE target resolves to a multicast address: " + addr.getHostAddress()); + } + byte[] b = addr.getAddress(); + if (addr instanceof Inet4Address) { + int i0 = b[0] & 0xFF; + int i1 = b[1] & 0xFF; + // 0.0.0.0/8 – "this" network (RFC 1122) + if (i0 == 0) { + throw new GeneralException("URL_RESOURCE target resolves to a reserved network address (0.0.0.0/8): " + addr.getHostAddress()); + } + // 100.64.0.0/10 – shared address space / CGNAT (RFC 6598) + if (i0 == 100 && i1 >= 64 && i1 <= 127) { + throw new GeneralException("URL_RESOURCE target resolves to a shared address space (CGNAT, 100.64.0.0/10): " + addr.getHostAddress()); + } + // 192.0.0.0/24 – IETF protocol assignments (RFC 6890) + if (i0 == 192 && i1 == 0 && (b[2] & 0xFF) == 0) { + throw new GeneralException("URL_RESOURCE target resolves to an IETF reserved address (192.0.0.0/24): " + addr.getHostAddress()); + } + // 198.18.0.0/15 – network benchmarking (RFC 2544) + if (i0 == 198 && (i1 == 18 || i1 == 19)) { + throw new GeneralException("URL_RESOURCE target resolves to a benchmarking address (198.18.0.0/15): " + addr.getHostAddress()); + } + // 240.0.0.0/4 – reserved for future use (RFC 1112) + if ((i0 & 0xF0) == 240) { + throw new GeneralException("URL_RESOURCE target resolves to a reserved address (240.0.0.0/4): " + addr.getHostAddress()); + } + } else if (addr instanceof Inet6Address) { + // fc00::/7 – Unique Local Addresses (ULA), private IPv6 (RFC 4193) + if ((b[0] & 0xFE) == 0xFC) { + throw new GeneralException("URL_RESOURCE target resolves to a unique-local (private) IPv6 address: " + addr.getHostAddress()); + } + // ::ffff:0:0/96 – IPv4-mapped IPv6; re-validate the embedded IPv4 address + boolean isIpv4Mapped = true; + for (int i = 0; i < 10; i++) { + if (b[i] != 0) { + isIpv4Mapped = false; + break; + } + } + if (isIpv4Mapped && (b[10] & 0xFF) == 0xFF && (b[11] & 0xFF) == 0xFF) { + try { + checkNotPrivateOrReservedAddress( + InetAddress.getByAddress(new byte[]{b[12], b[13], b[14], b[15]})); + } catch (UnknownHostException e) { + throw new GeneralException("URL_RESOURCE target contains an invalid IPv4-mapped IPv6 address"); + } + } + } + } + public static File getContentFile(String dataResourceTypeId, String objectInfo, String contextRoot) throws GeneralException, FileNotFoundException { File file = null; @@ -478,6 +692,7 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR if (!file.isAbsolute()) { throw new GeneralException("File (" + objectInfo + ") is not absolute"); } + checkLocalFileAllowList(file); } else if ("OFBIZ_FILE".equals(dataResourceTypeId) || "OFBIZ_FILE_BIN".equals(dataResourceTypeId)) { String prefix = System.getProperty("ofbiz.home"); @@ -489,6 +704,7 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR if (!file.exists()) { throw new FileNotFoundException("No file found: " + (prefix + sep + objectInfo)); } + checkOfbizFileAllowList(file); } else if ("CONTEXT_FILE".equals(dataResourceTypeId) || "CONTEXT_FILE_BIN".equals(dataResourceTypeId)) { if (UtilValidate.isEmpty(contextRoot)) { throw new GeneralException("Cannot find CONTEXT_FILE with an empty context root!"); @@ -502,6 +718,7 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR if (!file.exists()) { throw new FileNotFoundException("No file found: " + (contextRoot + sep + objectInfo)); } + checkContextFileBoundary(file, contextRoot); } return file; @@ -719,6 +936,8 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR // FTL template if ("FTL".equals(dataTemplateTypeId)) { + throw new GeneralException("Error rendering template: FTL template type is no longer supported for data resources."); + /* try { // get the template data for rendering String templateText = getDataResourceText(dataResource, targetMimeTypeId, locale, templateContext, delegator, cache); @@ -754,6 +973,7 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR } catch (TemplateException e) { throw new GeneralException("Error rendering FTL template", e); } + */ } else if ("XSLT".equals(dataTemplateTypeId)) { File targetFileLocation = new File(System.getProperty("ofbiz.home") + "/runtime/tempfiles/docbook.css"); @@ -1036,6 +1256,7 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR if (!file.exists()) { throw new FileNotFoundException("No file found: " + file.getAbsolutePath()); } + checkLocalFileAllowList(file); try (InputStreamReader in = new InputStreamReader(new FileInputStream(file), StandardCharsets.UTF_8)) { UtilIO.copy(in, out); } @@ -1049,6 +1270,7 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR if (!file.exists()) { throw new FileNotFoundException("No file found: " + file.getAbsolutePath()); } + checkOfbizFileAllowList(file); try (InputStreamReader in = new InputStreamReader(new FileInputStream(file), StandardCharsets.UTF_8)) { UtilIO.copy(in, out); } @@ -1062,6 +1284,7 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR if (!file.exists()) { throw new FileNotFoundException("No file found: " + file.getAbsolutePath()); } + checkContextFileBoundary(file, rootDir); try (InputStreamReader in = new InputStreamReader(new FileInputStream(file), StandardCharsets.UTF_8)) { if (Debug.infoOn()) { String enc = in.getEncoding(); @@ -1180,8 +1403,47 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR url = UtilURL.fromUrlString(newUrl); } + // SSRF prevention: validate protocol, optional host allow-list, and resolved IP ranges + checkUrlResourceAllowed(url); + + int connectTimeout = (int) UtilProperties.getPropertyNumber("security", + "content.data.url.resource.connect.timeout", 10000.0); + int readTimeout = (int) UtilProperties.getPropertyNumber("security", + "content.data.url.resource.read.timeout", 30000.0); + long maxResponseSize = (long) UtilProperties.getPropertyNumber("security", + "content.data.url.resource.max.response.size", (double) (10L * 1024 * 1024)); + URLConnection con = url.openConnection(); - return UtilMisc.toMap("stream", con.getInputStream(), "length", (long) con.getContentLength()); + con.setConnectTimeout(connectTimeout); + con.setReadTimeout(readTimeout); + // Disable automatic redirect-following to prevent SSRF bypass via redirect to private addresses + if (con instanceof HttpURLConnection) ((HttpURLConnection) con).setInstanceFollowRedirects(false); + con.connect(); + + // Reject redirects outright; we cannot safely re-validate an arbitrary Location header + if (con instanceof HttpURLConnection) { + HttpURLConnection httpCon = (HttpURLConnection) con; + int responseCode = httpCon.getResponseCode(); + if (responseCode >= 300 && responseCode < 400) { + httpCon.disconnect(); + throw new GeneralException("URL_RESOURCE request returned a redirect (" + responseCode + + "); redirects are not followed for security reasons"); + } + } + + long contentLength = con.getContentLengthLong(); + if (contentLength > maxResponseSize) { + if (con instanceof HttpURLConnection) ((HttpURLConnection) con).disconnect(); + throw new GeneralException("URL_RESOURCE response Content-Length (" + contentLength + + " bytes) exceeds the configured maximum of " + maxResponseSize + " bytes"); + } + + // Wrap with a bounded stream to enforce the size cap regardless of the Content-Length header + InputStream limitedStream = BoundedInputStream.builder() + .setInputStream(con.getInputStream()) + .setMaxCount(maxResponseSize) + .get(); + return UtilMisc.toMap("stream", limitedStream, "length", contentLength); } throw new GeneralException("No objectInfo found for URL_RESOURCE type; cannot stream"); } diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties index 569ceae370..2dc6f73707 100644 --- a/framework/security/config/security.properties +++ b/framework/security/config/security.properties @@ -160,6 +160,35 @@ security.internal.sso.enabled=false # Run './gradlew generateSecretKeys' to generate a cryptographically secure random key. security.token.key= +# -- Allowed subdirectories (relative to ofbiz.home) for the OFBIZ_FILE / OFBIZ_FILE_BIN data resource types. +# -- Comma-separated list, no spaces after commas, no leading slashes. +# -- Only files whose resolved canonical path starts with one of these entries will be served. +# -- Set to empty to allow any path within ofbiz.home (the ofbiz.home containment check still applies). +content.data.ofbiz.file.allowed.paths=applications/,themes/,plugins/,runtime/ + +# -- Allowed directories for the LOCAL_FILE / LOCAL_FILE_BIN data resource types (absolute paths). +# -- Comma-separated, no spaces after commas. Use ${ofbiz.home} as a portable placeholder. +# -- Only files whose resolved canonical path starts with one of these entries will be served. +# -- Set to empty to disable this check (NOT recommended). +content.data.local.file.allowed.paths=${ofbiz.home} + +# -- Allowed hosts for the URL_RESOURCE data resource type (comma-separated host names or host:port values). +# -- Both exact matches and subdomain matches are supported: "example.com" also permits "cdn.example.com". +# -- Leave empty to skip host filtering and apply only IP-range checks (private/loopback/link-local/multicast +# -- addresses are always blocked regardless of this setting). +content.data.url.resource.allowed.hosts= + +# -- Connection timeout in milliseconds for URL_RESOURCE fetches. Default: 10000 (10 s). +content.data.url.resource.connect.timeout=10000 + +# -- Read timeout in milliseconds for URL_RESOURCE fetches. Default: 30000 (30 s). +content.data.url.resource.read.timeout=30000 + +# -- Maximum response body size in bytes for URL_RESOURCE fetches. Default: 10485760 (10 MB). +# -- Responses advertising a larger Content-Length are refused before the connection body is read; +# -- the returned InputStream is additionally capped at this size regardless of the declared length. +content.data.url.resource.max.response.size=10485760 + # -- List of domains or IP addresses to be checked to prevent Host Header Injection, # -- no spaces after commas,no wildcard, can be extended of course... host-headers-allowed=localhost,127.0.0.1,demo-trunk.ofbiz.apache.org,demo-stable.ofbiz.apache.org,demo-next.ofbiz.apache.org,demo-trunk.ofbiz-test.apache.org,demo-stable-test.ofbiz.apache.org,demo-next.ofbiz-test.apache.org
