This is an automated email from the ASF dual-hosted git repository.
ashishvijaywargiya pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/trunk by this push:
new b2cf9fd373 Improved webtools data file viewer URL handling (#1707)
b2cf9fd373 is described below
commit b2cf9fd373ee182f35579e50e29e60776750046c
Author: Ashish Vijaywargiya <[email protected]>
AuthorDate: Mon Aug 24 00:33:05 2026 +0530
Improved webtools data file viewer URL handling (#1707)
Gate the fetch behind DATAFILE_MAINT, restrict fetched URLs to
http/https and public addresses, disable redirect following, and stop
echoing fetched content into exception messages.
Thank you Krishna Uprit for your help.
Co-authored-by: Krishnauprit18 <[email protected]>
---
.../java/org/apache/ofbiz/base/util/UtilURL.java | 148 +++++++++++++++++++++
.../org/apache/ofbiz/base/util/UtilURLTests.java | 86 ++++++++++++
.../java/org/apache/ofbiz/datafile/Record.java | 19 +--
.../org/apache/ofbiz/datafile/RecordIterator.java | 12 +-
framework/security/config/security.properties | 6 +
.../ofbiz/webtools/datafile/ViewDataFile.groovy | 120 +++++++++--------
6 files changed, 327 insertions(+), 64 deletions(-)
diff --git
a/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilURL.java
b/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilURL.java
index afb18ea321..9144168208 100644
--- a/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilURL.java
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilURL.java
@@ -19,10 +19,15 @@
package org.apache.ofbiz.base.util;
import java.io.File;
+import java.net.Inet4Address;
+import java.net.Inet6Address;
+import java.net.InetAddress;
import java.net.MalformedURLException;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
+import java.net.UnknownHostException;
+import java.util.Locale;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
@@ -156,6 +161,149 @@ public final class UtilURL {
return url;
}
+ /**
+ * Same as {@link #fromUrlString(String)}, except the result is rejected
unless it is safe to
+ * dereference: an http or https URL whose host does not resolve to a
loopback, link-local,
+ * private, or otherwise reserved address. Use this instead of {@link
#fromUrlString(String)}
+ * whenever the URL comes from an untrusted source (e.g. a request
parameter) and will be
+ * fetched by the server, to avoid server-side request forgery against the
local file system,
+ * loopback services, and the internal network.
+ *
+ * <p>An optional host allow-list can be configured with the
+ * {@code webtools.datafile.url.allowed.hosts} property in
security.properties: a
+ * comma-separated list of hostnames/domains. When set, only URLs whose
host matches an entry
+ * (or is a subdomain of one) are allowed, in addition to the address
checks above.
+ *
+ * @throws GeneralException if the URL is present but not allowed
+ */
+ public static URL fromCheckedUrlString(String urlString) throws
GeneralException {
+ if (urlString == null) {
+ return null;
+ }
+ URL url = fromUrlString(urlString);
+ if (url == null) {
+ return null;
+ }
+ checkUrlResourceAllowed(url);
+ return url;
+ }
+
+ /**
+ * Throws {@link GeneralException} unless {@code url} is an http or https
URL whose host
+ * resolves only to publicly routable addresses (and, when configured, is
in the
+ * {@code webtools.datafile.url.allowed.hosts} allow-list). See {@link
#fromCheckedUrlString}.
+ */
+ public static void checkUrlResourceAllowed(URL url) throws
GeneralException {
+ String protocol = url.getProtocol();
+ if (!"http".equalsIgnoreCase(protocol) &&
!"https".equalsIgnoreCase(protocol)) {
+ throw new GeneralException("URL only supports http/https
protocols; rejected: " + protocol);
+ }
+ String host = url.getHost();
+ if (UtilValidate.isEmpty(host)) {
+ throw new GeneralException("URL has no host component");
+ }
+
+ String allowedHostsStr = UtilProperties.getPropertyValue("security",
"webtools.datafile.url.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 host is not in the allowed
list: " + host);
+ }
+ }
+
+ InetAddress[] addresses;
+ try {
+ addresses = InetAddress.getAllByName(host);
+ } catch (UnknownHostException e) {
+ throw new GeneralException("URL host cannot be resolved: " + host);
+ }
+ if (addresses.length == 0) {
+ throw new GeneralException("URL 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, or otherwise reserved IP range (IPv4 and IPv6).
+ */
+ private static void checkNotPrivateOrReservedAddress(InetAddress addr)
throws GeneralException {
+ if (addr.isLoopbackAddress()) {
+ throw new GeneralException("URL target resolves to a loopback
address: " + addr.getHostAddress());
+ }
+ if (addr.isLinkLocalAddress()) {
+ throw new GeneralException("URL target resolves to a link-local
address: " + addr.getHostAddress());
+ }
+ if (addr.isSiteLocalAddress()) {
+ throw new GeneralException("URL target resolves to a private
(site-local) address: " + addr.getHostAddress());
+ }
+ if (addr.isAnyLocalAddress()) {
+ throw new GeneralException("URL target resolves to a wildcard
address: " + addr.getHostAddress());
+ }
+ if (addr.isMulticastAddress()) {
+ throw new GeneralException("URL 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 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 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 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 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 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 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 target contains an invalid
IPv4-mapped IPv6 address");
+ }
+ }
+ }
+ }
+
public static URL fromOfbizHomePath(String filename) {
String ofbizHome = System.getProperty("ofbiz.home");
if (ofbizHome == null) {
diff --git
a/framework/base/src/test/java/org/apache/ofbiz/base/util/UtilURLTests.java
b/framework/base/src/test/java/org/apache/ofbiz/base/util/UtilURLTests.java
new file mode 100644
index 0000000000..ab7414f5a5
--- /dev/null
+++ b/framework/base/src/test/java/org/apache/ofbiz/base/util/UtilURLTests.java
@@ -0,0 +1,86 @@
+/*******************************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+
*******************************************************************************/
+package org.apache.ofbiz.base.util;
+
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.net.URL;
+
+import org.junit.jupiter.api.Test;
+
+public class UtilURLTests {
+
+ @Test
+ public void rejectsFileScheme() {
+ GeneralException e = assertThrows(GeneralException.class, () ->
+ UtilURL.fromCheckedUrlString("file:///etc/passwd"));
+ assertTrue(e.getMessage().contains("http/https"));
+ }
+
+ @Test
+ public void rejectsLoopbackAddress() {
+ GeneralException e = assertThrows(GeneralException.class, () ->
+
UtilURL.fromCheckedUrlString("http://127.0.0.1:8080/internal";));
+ assertTrue(e.getMessage().contains("loopback"));
+ }
+
+ @Test
+ public void rejectsLoopbackHostname() {
+ GeneralException e = assertThrows(GeneralException.class, () ->
+
UtilURL.fromCheckedUrlString("http://localhost:8080/internal";));
+ assertTrue(e.getMessage().contains("loopback"));
+ }
+
+ @Test
+ public void rejectsLinkLocalCloudMetadataAddress() {
+ GeneralException e = assertThrows(GeneralException.class, () ->
+
UtilURL.fromCheckedUrlString("http://169.254.169.254/latest/meta-data/";));
+ assertTrue(e.getMessage().contains("link-local"));
+ }
+
+ @Test
+ public void rejectsPrivateAddress() {
+ GeneralException e = assertThrows(GeneralException.class, () ->
+ UtilURL.fromCheckedUrlString("http://10.0.0.5/internal";));
+ assertTrue(e.getMessage().contains("private"));
+ }
+
+ @Test
+ public void rejectsFtpScheme() {
+ assertThrows(GeneralException.class, () ->
UtilURL.fromCheckedUrlString("ftp://example.com/file";));
+ }
+
+ @Test
+ public void allowsPubliclyRoutableHttpsHost() throws Exception {
+ URL url =
UtilURL.fromCheckedUrlString("https://93.184.216.34/index.html";);
+ assertTrue(url != null && "93.184.216.34".equals(url.getHost()));
+ }
+
+ @Test
+ public void returnsNullForNullInput() throws Exception {
+ assertNull(UtilURL.fromCheckedUrlString(null));
+ }
+
+ @Test
+ public void returnsNullForUnparsableInput() throws Exception {
+ assertNull(UtilURL.fromCheckedUrlString("not a url at all ::"));
+ }
+}
diff --git
a/framework/datafile/src/main/java/org/apache/ofbiz/datafile/Record.java
b/framework/datafile/src/main/java/org/apache/ofbiz/datafile/Record.java
index 20bbba49ec..8c7ab88861 100644
--- a/framework/datafile/src/main/java/org/apache/ofbiz/datafile/Record.java
+++ b/framework/datafile/src/main/java/org/apache/ofbiz/datafile/Record.java
@@ -604,13 +604,15 @@ public class Record implements Serializable {
try {
record.setString(modelField.getName(), strVal);
} catch (java.text.ParseException e) {
+ // Note: deliberately not including strVal here; it is
unvalidated content read from
+ // the target file/URL and this message can reach the
requesting user and the log.
throw new DataFileException(
- "Could not parse field " + modelField.getName() + ",
format string \"" + modelField.getFormat() + "\" with value " + strVal
- + " on line " + lineNum, e);
+ "Could not parse field " + modelField.getName() + ",
format string \"" + modelField.getFormat()
+ + "\" on line " + lineNum, e);
} catch (java.lang.NumberFormatException e) {
throw new DataFileException(
- "Number not valid for field " + modelField.getName() +
", format string \"" + modelField.getFormat() + "\" with value "
- + strVal + " on line " + lineNum, e);
+ "Number not valid for field " + modelField.getName() +
", format string \"" + modelField.getFormat()
+ + "\" on line " + lineNum, e);
}
}
return record;
@@ -674,13 +676,14 @@ public class Record implements Serializable {
}
record.setString(modelField.getName(), strVal);
} catch (java.text.ParseException e) {
+ // Note: deliberately not including strVal here; see
createRecord above.
throw new DataFileException(
- "Could not parse field " + modelField.getName() + ",
format string \"" + modelField.getFormat() + "\" with value " + strVal
- + " on line " + lineNum, e);
+ "Could not parse field " + modelField.getName() + ",
format string \"" + modelField.getFormat()
+ + "\" on line " + lineNum, e);
} catch (java.lang.NumberFormatException e) {
throw new DataFileException(
- "Number not valid for field " + modelField.getName() +
", format string \"" + modelField.getFormat() + "\" with value "
- + strVal + " on line " + lineNum, e);
+ "Number not valid for field " + modelField.getName() +
", format string \"" + modelField.getFormat()
+ + "\" on line " + lineNum, e);
}
}
return record;
diff --git
a/framework/datafile/src/main/java/org/apache/ofbiz/datafile/RecordIterator.java
b/framework/datafile/src/main/java/org/apache/ofbiz/datafile/RecordIterator.java
index a1ec2cb3d3..46bc8a3b68 100644
---
a/framework/datafile/src/main/java/org/apache/ofbiz/datafile/RecordIterator.java
+++
b/framework/datafile/src/main/java/org/apache/ofbiz/datafile/RecordIterator.java
@@ -22,7 +22,9 @@ import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
+import java.net.HttpURLConnection;
import java.net.URL;
+import java.net.URLConnection;
import java.nio.charset.Charset;
import java.util.Stack;
@@ -53,7 +55,15 @@ public class RecordIterator {
InputStream urlStream = null;
try {
- urlStream = fileUrl.openStream();
+ // Don't follow redirects: the caller may have already validated
fileUrl's host as safe
+ // to fetch from (see UtilURL.fromCheckedUrlString), and a
redirect could point
+ // anywhere, bypassing that check.
+ URLConnection connection = fileUrl.openConnection();
+ if (connection instanceof HttpURLConnection) {
+ HttpURLConnection httpConnection = (HttpURLConnection)
connection;
+ httpConnection.setInstanceFollowRedirects(false);
+ }
+ urlStream = connection.getInputStream();
} catch (IOException e) {
throw new DataFileException("Error open URL: " +
fileUrl.toString(), e);
}
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index 234c06531b..2b202d7fca 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -198,6 +198,12 @@ 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
+# -- Allowed hosts for URLs fetched by the webtools data file viewer/loader
(comma-separated host
+# -- names). Both exact matches and subdomain matches are supported, as above.
Leave empty to skip
+# -- host filtering and apply only the scheme (http/https only) and IP-range
checks; loopback,
+# -- link-local, private and other reserved addresses are always blocked
regardless of this setting.
+webtools.datafile.url.allowed.hosts=
+
# -- 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.
diff --git
a/framework/webtools/src/main/groovy/org/apache/ofbiz/webtools/datafile/ViewDataFile.groovy
b/framework/webtools/src/main/groovy/org/apache/ofbiz/webtools/datafile/ViewDataFile.groovy
index ed9b28709e..258f46b793 100644
---
a/framework/webtools/src/main/groovy/org/apache/ofbiz/webtools/datafile/ViewDataFile.groovy
+++
b/framework/webtools/src/main/groovy/org/apache/ofbiz/webtools/datafile/ViewDataFile.groovy
@@ -19,6 +19,7 @@
package org.apache.ofbiz.webtools.datafile
import org.apache.ofbiz.base.util.Debug
+import org.apache.ofbiz.base.util.GeneralException
import org.apache.ofbiz.base.util.UtilProperties
import org.apache.ofbiz.base.util.UtilURL
import org.apache.ofbiz.datafile.DataFile
@@ -28,78 +29,87 @@ import org.apache.ofbiz.datafile.ModelDataFileReader
uiLabelMap = UtilProperties.getResourceBundleMap('WebtoolsUiLabels', locale)
messages = []
-dataFileSave = request.getParameter('DATAFILE_SAVE')
+// This screen builds URLs from raw request parameters and dereferences them;
that fetch must
+// never happen for a principal who lacks DATAFILE_MAINT, so the whole thing
is gated here,
+// ahead of any parameter handling, rather than left to the screen/template
permission checks
+// that only gate rendering after the fetch already ran.
+if (security.hasPermission('DATAFILE_MAINT', session)) {
+ dataFileSave = request.getParameter('DATAFILE_SAVE')
-entityXmlFileSave = request.getParameter('ENTITYXML_FILE_SAVE')
+ entityXmlFileSave = request.getParameter('ENTITYXML_FILE_SAVE')
-dataFileLoc = request.getParameter('DATAFILE_LOCATION')
-definitionLoc = request.getParameter('DEFINITION_LOCATION')
-definitionName = request.getParameter('DEFINITION_NAME')
-dataFileIsUrl = null != request.getParameter('DATAFILE_IS_URL')
-definitionIsUrl = null != request.getParameter('DEFINITION_IS_URL')
+ dataFileLoc = request.getParameter('DATAFILE_LOCATION')
+ definitionLoc = request.getParameter('DEFINITION_LOCATION')
+ definitionName = request.getParameter('DEFINITION_NAME')
+ dataFileIsUrl = null != request.getParameter('DATAFILE_IS_URL')
+ definitionIsUrl = null != request.getParameter('DEFINITION_IS_URL')
-try {
- dataFileUrl = dataFileIsUrl ? UtilURL.fromUrlString(dataFileLoc) :
UtilURL.fromFilename(dataFileLoc)
-}
-catch (java.net.MalformedURLException e) {
- messages.add(e.getMessage())
-}
-
-try {
- definitionUrl = definitionIsUrl ? UtilURL.fromUrlString(definitionLoc) :
UtilURL.fromFilename(definitionLoc)
-}
-catch (java.net.MalformedURLException e) {
- messages.add(e.getMessage())
-}
-
-definitionNames = null
-if (definitionUrl) {
+ dataFileUrl = null
try {
- ModelDataFileReader reader =
ModelDataFileReader.getModelDataFileReader(definitionUrl)
- if (reader) {
- definitionNames =
((Collection)reader.getDataFileNames()).iterator()
- context.put('definitionNames', definitionNames)
- }
+ dataFileUrl = dataFileIsUrl ?
UtilURL.fromCheckedUrlString(dataFileLoc) : UtilURL.fromFilename(dataFileLoc)
}
- catch (Exception e) {
+ catch (java.net.MalformedURLException | GeneralException e) {
messages.add(e.getMessage())
}
-}
-dataFile = null
-if (dataFileUrl && definitionUrl && definitionNames) {
+ definitionUrl = null
try {
- dataFile = DataFile.readFile(dataFileUrl, definitionUrl,
definitionName)
- context.put('dataFile', dataFile)
+ definitionUrl = definitionIsUrl ?
UtilURL.fromCheckedUrlString(definitionLoc) :
UtilURL.fromFilename(definitionLoc)
}
- catch (Exception e) {
- messages.add(e.toString()); Debug.log(e)
+ catch (java.net.MalformedURLException | GeneralException e) {
+ messages.add(e.getMessage())
}
-}
-if (dataFile) {
- modelDataFile = dataFile.getModelDataFile()
- context.put('modelDataFile', modelDataFile)
-}
+ definitionNames = null
+ if (definitionUrl) {
+ try {
+ ModelDataFileReader reader =
ModelDataFileReader.getModelDataFileReader(definitionUrl)
+ if (reader) {
+ definitionNames =
((Collection)reader.getDataFileNames()).iterator()
+ context.put('definitionNames', definitionNames)
+ }
+ }
+ catch (Exception e) {
+ messages.add(e.getMessage())
+ }
+ }
-if (dataFile && dataFileSave) {
- try {
- dataFile.writeDataFile(dataFileSave)
- messages.add(uiLabelMap.WebtoolsDataFileSavedTo + dataFileSave)
+ dataFile = null
+ if (dataFileUrl && definitionUrl && definitionNames) {
+ try {
+ dataFile = DataFile.readFile(dataFileUrl, definitionUrl,
definitionName)
+ context.put('dataFile', dataFile)
+ }
+ catch (Exception e) {
+ messages.add(e.getMessage())
+ Debug.logError(e, 'Error reading data file', 'ViewDataFile.groovy')
+ }
}
- catch (Exception e) {
- messages.add(e.getMessage())
+
+ if (dataFile) {
+ modelDataFile = dataFile.getModelDataFile()
+ context.put('modelDataFile', modelDataFile)
}
-}
-if (dataFile && entityXmlFileSave) {
- try {
- //dataFile.writeDataFile(entityXmlFileSave)
- DataFile2EntityXml.writeToEntityXml(entityXmlFileSave, dataFile)
- messages.add(uiLabelMap.WebtoolsDataEntityFileSavedTo +
entityXmlFileSave)
+ if (dataFile && dataFileSave) {
+ try {
+ dataFile.writeDataFile(dataFileSave)
+ messages.add(uiLabelMap.WebtoolsDataFileSavedTo + dataFileSave)
+ }
+ catch (Exception e) {
+ messages.add(e.getMessage())
+ }
}
- catch (Exception e) {
- messages.add(e.getMessage())
+
+ if (dataFile && entityXmlFileSave) {
+ try {
+ //dataFile.writeDataFile(entityXmlFileSave)
+ DataFile2EntityXml.writeToEntityXml(entityXmlFileSave, dataFile)
+ messages.add(uiLabelMap.WebtoolsDataEntityFileSavedTo +
entityXmlFileSave)
+ }
+ catch (Exception e) {
+ messages.add(e.getMessage())
+ }
}
}
context.messages = messages
