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 4afb9c9ab4 Restrict widget resource locations to component:// or an
explicit allowlist (#1650)
4afb9c9ab4 is described below
commit 4afb9c9ab4d9fe7777c00f45eb07c961bcbc357a
Author: Ashish Vijaywargiya <[email protected]>
AuthorDate: Fri Aug 14 18:57:56 2026 +0530
Restrict widget resource locations to component:// or an explicit allowlist
(#1650)
Add WidgetSecureLocation as a single gatekeeper for screen, form, grid,
menu, and tree resource locations. Reject any file: scheme location
regardless of letter case, reject '..' traversal inside component://
locations, and deny non-component locations by default unless allowed
via the new security.allowFilePaths pattern.
Also stop anonymous JSON request bodies from overriding request
attributes that already exist as trusted ServletContext attributes,
which is how a request-controlled value could shadow a webapp's
configured decorator location.
Adds unit tests for the new checks.
Thank you Krishna Uprit(@Krishnauprit18) and Nicolas Malin(@nmalin) for
your help.
PR from @nmalin - https://github.com/apache/ofbiz-framework/pull/1552
PR from @Krishnauprit18 -
https://github.com/apache/ofbiz-framework/pull/1586
---
.../commonext/widget/ofbizsetup/ProfileScreens.xml | 4 +-
.../org/apache/ofbiz/base/util/UtilValidate.java | 26 +++---
.../apache/ofbiz/base/util/UtilValidateTests.java | 17 ++++
framework/security/config/security.properties | 9 ++
.../java/org/apache/ofbiz/webapp/WebAppUtil.java | 10 +++
.../org/apache/ofbiz/webapp/WebAppUtilTests.java | 97 +++++++++++++++++++++
.../org/apache/ofbiz/widget/model/FormFactory.java | 19 ++++-
.../org/apache/ofbiz/widget/model/GridFactory.java | 19 ++++-
.../org/apache/ofbiz/widget/model/MenuFactory.java | 17 +++-
.../apache/ofbiz/widget/model/ScreenFactory.java | 2 +-
.../org/apache/ofbiz/widget/model/TreeFactory.java | 9 +-
.../ofbiz/widget/model/WidgetSecureLocation.java | 61 ++++++++++++--
.../widget/model/WidgetSecureLocationTests.java | 98 ++++++++++++++++++++++
13 files changed, 353 insertions(+), 35 deletions(-)
diff --git a/applications/commonext/widget/ofbizsetup/ProfileScreens.xml
b/applications/commonext/widget/ofbizsetup/ProfileScreens.xml
index a2a5ae7232..e0269bd4db 100644
--- a/applications/commonext/widget/ofbizsetup/ProfileScreens.xml
+++ b/applications/commonext/widget/ofbizsetup/ProfileScreens.xml
@@ -90,8 +90,8 @@
<set field="helpAnchor"
value="_help_for_view_organization_profile"/>
</actions>
<widgets>
- <include-screen name="Party"
location="applications/party/widget/partymgr/ProfileScreens.xml"/>
- <include-screen name="Contact"
location="applications/party/widget/partymgr/ProfileScreens.xml"/>
+ <include-screen name="Party"
location="component://party/widget/partymgr/ProfileScreens.xml"/>
+ <include-screen name="Contact"
location="component://party/widget/partymgr/ProfileScreens.xml"/>
</widgets>
</section>
</screen>
diff --git
a/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilValidate.java
b/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilValidate.java
index 4a9b3d96b8..5aa77244f6 100644
--- a/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilValidate.java
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilValidate.java
@@ -18,11 +18,11 @@
*******************************************************************************/
package org.apache.ofbiz.base.util;
-import java.nio.file.Path;
import java.nio.file.Paths;
import java.sql.Timestamp;
import java.util.Collection;
import java.util.Map;
+import java.util.regex.Pattern;
import org.apache.commons.validator.routines.EmailValidator;
import org.apache.commons.validator.routines.UrlValidator;
@@ -155,9 +155,6 @@ public final class UtilValidate {
public static final String CONTIGUOUS_US_STATE_CODES =
"AL|AZ|AR|CA|CO|CT|DE|DC|FL|GA|ID|IL|IN|IA|KS|KY|LA|ME|MD|MA|MI|MN|MS|MO|MT|NE|NV|NH|"
+ "NJ|NM|NY|NC|ND|OH|OK|OR|PA|RI|SC|SD|TN|TX|UT|VT|VA|WA|WV|WI|WY";
- /** Paths from which loading files should be prevented */
- public static final String[] BLOCKED_PATHS = {"proc/self/fd"};
-
/** Check whether an object is empty, will see if it is a String, Map,
Collection, etc. */
public static boolean isEmpty(Object o) {
return ObjectType.isEmpty(o);
@@ -662,22 +659,23 @@ public final class UtilValidate {
}
/**
- * isBlockedPath takes a String representing a filePath, normalizes it and
checks it against a Blacklist
+ * isAllowedPath takes a String representing a non-component widget
resource path, normalizes it and
+ * checks it against the administrator-configured
<code>security.allowFilePaths</code> regular
+ * expression. Unset or blank configuration denies every path (secure by
default): an administrator
+ * must explicitly opt in to loading widget resources from outside a
<code>component://</code> location.
* @param rawPathString
- * @return true if its a blocked path, false otherwise or if it is empty
+ * @return true if it's an allowed path, false otherwise (including when
unconfigured)
*/
- public static boolean isBlockedPath(String rawPathString) {
+ public static boolean isAllowedPath(String rawPathString) {
if (UtilValidate.isEmpty(rawPathString)) {
return false;
}
- Path normalized = Paths.get(rawPathString).normalize();
- String normalizedPath = normalized.toString();
- for (String blocked : BLOCKED_PATHS) {
- if (normalizedPath.contains(blocked)) {
- return true;
- }
+ String allowFilePaths = UtilProperties.getPropertyValue("security",
"allowFilePaths", "");
+ if (UtilValidate.isEmpty(allowFilePaths)) {
+ return false;
}
- return false;
+ String normalizedPath =
Paths.get(rawPathString).normalize().toString();
+ return
Pattern.compile(allowFilePaths).matcher(normalizedPath).matches();
}
/** isYear returns true if string s is a valid
diff --git
a/framework/base/src/test/java/org/apache/ofbiz/base/util/UtilValidateTests.java
b/framework/base/src/test/java/org/apache/ofbiz/base/util/UtilValidateTests.java
index ba26572a77..1f20f35f2c 100644
---
a/framework/base/src/test/java/org/apache/ofbiz/base/util/UtilValidateTests.java
+++
b/framework/base/src/test/java/org/apache/ofbiz/base/util/UtilValidateTests.java
@@ -43,4 +43,21 @@ public class UtilValidateTests {
assertTrue(UtilValidate.isUrlInString("https://foo/bar";));
assertTrue(UtilValidate.isUrlInString("component://foo/bar?param=http://moo/far";));
}
+
+ @Test
+ public void testIsAllowedPathDefaultDenyWhenUnconfigured() throws
Exception {
+ UtilProperties.setPropertyValueInMemory("security", "allowFilePaths",
"");
+
assertFalse(UtilValidate.isAllowedPath("/opt/ofbiz/templates/foo.ftl"));
+ assertFalse(UtilValidate.isAllowedPath("/dev/fd/292"));
+ assertFalse(UtilValidate.isAllowedPath(""));
+ }
+
+ @Test
+ public void testIsAllowedPathHonorsConfiguredPattern() throws Exception {
+ UtilProperties.setPropertyValueInMemory("security", "allowFilePaths",
"/opt/ofbiz/templates/.*");
+ assertTrue(UtilValidate.isAllowedPath("/opt/ofbiz/templates/foo.ftl"));
+ assertFalse(UtilValidate.isAllowedPath("/etc/passwd"));
+ // restore default-deny for other tests sharing the in-memory
properties cache
+ UtilProperties.setPropertyValueInMemory("security", "allowFilePaths",
"");
+ }
}
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index 3231aa6de5..234c06531b 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -315,6 +315,15 @@
deniedFileExtensions=html,htm,php,php1,php2,hph3,php4,php5,php6,php7,phps,asp,as
#-- As it name says, allowAllUploads opens all possibilities
allowAllUploads=
+#--
+#-- Widget resources (screens, forms, grids, menus, trees) are only loaded
from a component://
+#-- location by default. allowFilePaths is a regular expression an
administrator can set to also
+#-- allow loading widget resources from bare filesystem paths outside any
component, matched via
+#-- UtilValidate::isAllowedPath. Left blank (the default), every non-component
location is denied.
+#-- A file: URI (in any letter case, e.g. file:/some/path) is never allowed
here, regardless of
+#-- this setting: see WidgetSecureLocation.
+allowFilePaths=
+
#--
#-- Default characters that are allowed in file names and file extensions to
guarantee safeness
#-- Uncomment to change. Note that allowing all characters is at risk.
diff --git
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/WebAppUtil.java
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/WebAppUtil.java
index ebcd9d41b3..3b25858934 100644
--- a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/WebAppUtil.java
+++ b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/WebAppUtil.java
@@ -159,8 +159,18 @@ public final class WebAppUtil {
Debug.logWarning(ioe, MODULE);
}
if (requestBodyMap != null) {
+ ServletContext servletContext = request.getServletContext();
Set<String> parameterNames = requestBodyMap.keySet();
for (String parameterName: parameterNames) {
+ // A request body is anonymous, attacker-controlled input.
Never let it shadow a name
+ // the webapp already exposes as a trusted, application-owned
ServletContext attribute
+ // (e.g. mainDecoratorLocation, set from web.xml at filter
init) - doing so let an
+ // unauthenticated JSON request redirect trusted widget/screen
locations.
+ if (servletContext.getAttribute(parameterName) != null) {
+ Debug.logWarning("Ignoring request body attribute [%s]: it
shadows an existing"
+ + " ServletContext attribute of the same name",
MODULE, parameterName);
+ continue;
+ }
request.setAttribute(parameterName,
requestBodyMap.get(parameterName));
}
}
diff --git
a/framework/webapp/src/test/java/org/apache/ofbiz/webapp/WebAppUtilTests.java
b/framework/webapp/src/test/java/org/apache/ofbiz/webapp/WebAppUtilTests.java
new file mode 100644
index 0000000000..d5ae92adf2
--- /dev/null
+++
b/framework/webapp/src/test/java/org/apache/ofbiz/webapp/WebAppUtilTests.java
@@ -0,0 +1,97 @@
+/*******************************************************************************
+ * 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.webapp;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.when;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+
+import jakarta.servlet.ReadListener;
+import jakarta.servlet.ServletContext;
+import jakarta.servlet.ServletInputStream;
+import jakarta.servlet.http.HttpServletRequest;
+
+import org.junit.jupiter.api.Test;
+
+/** Covers the JSON-request-body attribute merge that fed the reported
+ * anonymous mainDecoratorLocation override (login-page widget-injection RCE):
+ * a request body must never be able to shadow a name the webapp already
+ * exposes as a trusted, application-owned ServletContext attribute. */
+public class WebAppUtilTests {
+
+ private static ServletInputStream inputStreamOf(String content) {
+ ByteArrayInputStream bytes = new
ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8));
+ return new ServletInputStream() {
+ @Override
+ public boolean isFinished() {
+ return bytes.available() == 0;
+ }
+
+ @Override
+ public boolean isReady() {
+ return true;
+ }
+
+ @Override
+ public void setReadListener(ReadListener readListener) {
+ }
+
+ @Override
+ public int read() {
+ return bytes.read();
+ }
+ };
+ }
+
+ @Test
+ public void doesNotOverrideAnExistingServletContextAttribute() throws
IOException {
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ ServletContext servletContext = mock(ServletContext.class);
+ when(request.getServletContext()).thenReturn(servletContext);
+ when(request.getContentType()).thenReturn("application/json");
+ when(request.getInputStream()).thenReturn(
+
inputStreamOf("{\"mainDecoratorLocation\":\"file:/dev/fd/292\"}"));
+ when(servletContext.getAttribute("mainDecoratorLocation"))
+
.thenReturn("component://order/widget/ordermgr/CommonScreens.xml");
+
+ WebAppUtil.setAttributesFromRequestBody(request);
+
+ verify(request, never()).setAttribute("mainDecoratorLocation",
"file:/dev/fd/292");
+ }
+
+ @Test
+ public void stillSetsAttributesThatDoNotShadowContextConfig() throws
IOException {
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ ServletContext servletContext = mock(ServletContext.class);
+ when(request.getServletContext()).thenReturn(servletContext);
+ when(request.getContentType()).thenReturn("application/json");
+ when(request.getInputStream()).thenReturn(
+ inputStreamOf("{\"searchString\":\"widgets\"}"));
+ when(servletContext.getAttribute("searchString")).thenReturn(null);
+
+ WebAppUtil.setAttributesFromRequestBody(request);
+
+ verify(request).setAttribute("searchString", "widgets");
+ }
+}
diff --git
a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/FormFactory.java
b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/FormFactory.java
index 41b185ec16..9f7617809d 100644
---
a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/FormFactory.java
+++
b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/FormFactory.java
@@ -28,6 +28,7 @@ import jakarta.servlet.http.HttpServletRequest;
import javax.xml.parsers.ParserConfigurationException;
import org.apache.ofbiz.base.location.FlexibleLocation;
+import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.UtilHttp;
import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.base.util.UtilXml;
@@ -71,7 +72,13 @@ public class FormFactory {
String cacheKey = sb.toString();
ModelForm modelForm = FORM_LOCATION_CACHE.get(cacheKey);
if (modelForm == null) {
- URL formFileUrl = FlexibleLocation.resolveLocation(resourceName);
+ String sanitizedLocation =
WidgetSecureLocation.sanitize(resourceName);
+ if (sanitizedLocation == null) {
+ Debug.logWarning("The location of form [%s] isn't an allowed
path. Abort rendering. Raw location [%s]",
+ MODULE, formName, resourceName);
+ throw new IllegalArgumentException("Abort form rendering due
to unallowed form location");
+ }
+ URL formFileUrl =
FlexibleLocation.resolveLocation(sanitizedLocation);
if (formFileUrl == null ||
UtilValidate.isUrlInStringAndDoesNotStartByComponentProtocol(formFileUrl.toString()))
{
throw new IllegalArgumentException("Could not resolve location
to URL: " + resourceName);
}
@@ -104,11 +111,17 @@ public class FormFactory {
if (modelForm == null) {
Delegator delegator = (Delegator)
request.getAttribute("delegator");
LocalDispatcher dispatcher = (LocalDispatcher)
request.getAttribute("dispatcher");
- URL formFileUrl =
request.getServletContext().getResource(resourceName);
+ String sanitizedLocation =
WidgetSecureLocation.sanitize(resourceName);
+ if (sanitizedLocation == null) {
+ Debug.logWarning("The location of form [%s] isn't an allowed
path. Abort rendering. Raw location [%s]",
+ MODULE, formName, resourceName);
+ throw new IllegalArgumentException("Abort form rendering due
to unallowed form location");
+ }
+ URL formFileUrl =
request.getServletContext().getResource(sanitizedLocation);
Document formFileDoc = UtilXml.readXmlDocument(formFileUrl, true,
true);
Element formElement =
UtilXml.firstChildElement(formFileDoc.getDocumentElement(), "form", "name",
formName);
modelForm = createModelForm(formElement,
delegator.getModelReader(), visualTheme, dispatcher.getDispatchContext(),
- resourceName, formName);
+ sanitizedLocation, formName);
modelForm = FORM_WEBAPP_CACHE.putIfAbsentAndGet(cacheKey,
modelForm);
}
if (modelForm == null) {
diff --git
a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/GridFactory.java
b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/GridFactory.java
index f7907b310d..20a6603f7f 100644
---
a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/GridFactory.java
+++
b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/GridFactory.java
@@ -29,6 +29,7 @@ import jakarta.servlet.http.HttpServletRequest;
import javax.xml.parsers.ParserConfigurationException;
import org.apache.ofbiz.base.location.FlexibleLocation;
+import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.UtilHttp;
import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.base.util.UtilXml;
@@ -73,7 +74,13 @@ public class GridFactory {
String cacheKey = sb.toString();
ModelGrid modelGrid = GRID_LOCATION_CACHE.get(cacheKey);
if (modelGrid == null) {
- URL gridFileUrl = FlexibleLocation.resolveLocation(resourceName);
+ String sanitizedLocation =
WidgetSecureLocation.sanitize(resourceName);
+ if (sanitizedLocation == null) {
+ Debug.logWarning("The location of grid [%s] isn't an allowed
path. Abort rendering. Raw location [%s]",
+ MODULE, gridName, resourceName);
+ throw new IllegalArgumentException("Abort grid rendering due
to unallowed grid location");
+ }
+ URL gridFileUrl =
FlexibleLocation.resolveLocation(sanitizedLocation);
if (gridFileUrl == null ||
UtilValidate.isUrlInStringAndDoesNotStartByComponentProtocol(gridFileUrl.toString()))
{
throw new IllegalArgumentException("Could not resolve location
to URL: " + resourceName);
}
@@ -108,11 +115,17 @@ public class GridFactory {
ServletContext servletContext = request.getServletContext();
Delegator delegator = (Delegator)
request.getAttribute("delegator");
LocalDispatcher dispatcher = (LocalDispatcher)
request.getAttribute("dispatcher");
- URL gridFileUrl = servletContext.getResource(resourceName);
+ String sanitizedLocation =
WidgetSecureLocation.sanitize(resourceName);
+ if (sanitizedLocation == null) {
+ Debug.logWarning("The location of grid [%s] isn't an allowed
path. Abort rendering. Raw location [%s]",
+ MODULE, gridName, resourceName);
+ throw new IllegalArgumentException("Abort grid rendering due
to unallowed grid location");
+ }
+ URL gridFileUrl = servletContext.getResource(sanitizedLocation);
Document gridFileDoc = UtilXml.readXmlDocument(gridFileUrl, true,
true);
Element gridElement =
UtilXml.firstChildElement(gridFileDoc.getDocumentElement(), "grid", "name",
gridName);
modelGrid = createModelGrid(gridElement,
delegator.getModelReader(), visualTheme,
- dispatcher.getDispatchContext(), resourceName, gridName);
+ dispatcher.getDispatchContext(), sanitizedLocation,
gridName);
modelGrid = GRID_WEBAPP_CACHE.putIfAbsentAndGet(cacheKey,
modelGrid);
}
if (modelGrid == null) {
diff --git
a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/MenuFactory.java
b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/MenuFactory.java
index afbfebde0a..171c26f354 100644
---
a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/MenuFactory.java
+++
b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/MenuFactory.java
@@ -28,6 +28,7 @@ import jakarta.servlet.http.HttpServletRequest;
import javax.xml.parsers.ParserConfigurationException;
import org.apache.ofbiz.base.location.FlexibleLocation;
+import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.UtilHttp;
import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.base.util.UtilXml;
@@ -65,7 +66,13 @@ public class MenuFactory {
if (modelMenuMap == null) {
ServletContext servletContext = request.getServletContext();
- URL menuFileUrl = servletContext.getResource(resourceName);
+ String sanitizedLocation =
WidgetSecureLocation.sanitize(resourceName);
+ if (sanitizedLocation == null) {
+ Debug.logWarning("The location of menu [%s] isn't an allowed
path. Abort rendering. Raw location [%s]",
+ MODULE, menuName, resourceName);
+ throw new IllegalArgumentException("Abort menu rendering due
to unallowed menu location");
+ }
+ URL menuFileUrl = servletContext.getResource(sanitizedLocation);
Document menuFileDoc = UtilXml.readXmlDocument(menuFileUrl, true,
true);
modelMenuMap = readMenuDocument(menuFileDoc, location,
visualTheme);
MENU_WEBAPP_CACHE.putIfAbsent(cacheKey, modelMenuMap);
@@ -106,7 +113,13 @@ public class MenuFactory {
String keyName = resourceName + "::" + visualTheme.getVisualThemeId();
Map<String, ModelMenu> modelMenuMap = MENU_LOCATION_CACHE.get(keyName);
if (modelMenuMap == null) {
- URL menuFileUrl = FlexibleLocation.resolveLocation(resourceName);
+ String sanitizedLocation =
WidgetSecureLocation.sanitize(resourceName);
+ if (sanitizedLocation == null) {
+ Debug.logWarning("The location of menu [%s] isn't an allowed
path. Abort rendering. Raw location [%s]",
+ MODULE, menuName, resourceName);
+ throw new IllegalArgumentException("Abort menu rendering due
to unallowed menu location");
+ }
+ URL menuFileUrl =
FlexibleLocation.resolveLocation(sanitizedLocation);
if (menuFileUrl == null ||
UtilValidate.isUrlInStringAndDoesNotStartByComponentProtocol(menuFileUrl.toString()))
{
throw new IllegalArgumentException("Could not resolve location
to URL: " + resourceName);
}
diff --git
a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ScreenFactory.java
b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ScreenFactory.java
index 739816fb44..bff5cd2ad2 100644
---
a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ScreenFactory.java
+++
b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ScreenFactory.java
@@ -200,7 +200,7 @@ public class ScreenFactory {
if (UtilValidate.isNotEmpty(location)) {
String sanitizedLocation = WidgetSecureLocation.sanitize(location);
if (sanitizedLocation == null) {
- Debug.logWarning("The location of screen [%s] isn't an allowed
Path. Abort rendering. Raw location [%s]", MODULE, name, location);
+ Debug.logWarning("The location of screen [%s] isn't an allowed
path. Abort rendering. Raw location [%s]", MODULE, name, location);
throw new IllegalArgumentException("Abort screen rendering due
to unallowed screen location");
}
try {
diff --git
a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/TreeFactory.java
b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/TreeFactory.java
index c32e381741..c58d531dfa 100644
---
a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/TreeFactory.java
+++
b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/TreeFactory.java
@@ -26,6 +26,7 @@ import java.util.Map;
import javax.xml.parsers.ParserConfigurationException;
import org.apache.ofbiz.base.location.FlexibleLocation;
+import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.base.util.UtilXml;
import org.apache.ofbiz.base.util.cache.UtilCache;
@@ -50,7 +51,13 @@ public class TreeFactory {
throws IOException, SAXException, ParserConfigurationException {
Map<String, ModelTree> modelTreeMap =
TREE_LOCATION_CACHE.get(resourceName);
if (modelTreeMap == null) {
- URL treeFileUrl = FlexibleLocation.resolveLocation(resourceName);
+ String sanitizedLocation =
WidgetSecureLocation.sanitize(resourceName);
+ if (sanitizedLocation == null) {
+ Debug.logWarning("The location of tree [%s] isn't an allowed
path. Abort rendering. Raw location [%s]",
+ MODULE, treeName, resourceName);
+ throw new IllegalArgumentException("Abort tree rendering due
to unallowed tree location");
+ }
+ URL treeFileUrl =
FlexibleLocation.resolveLocation(sanitizedLocation);
if (treeFileUrl == null ||
UtilValidate.isUrlInStringAndDoesNotStartByComponentProtocol(treeFileUrl.toString()))
{
throw new IllegalArgumentException("Could not resolve location
to URL: " + resourceName);
}
diff --git
a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/WidgetSecureLocation.java
b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/WidgetSecureLocation.java
index 8c87b7b640..16a6e2f85e 100644
---
a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/WidgetSecureLocation.java
+++
b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/WidgetSecureLocation.java
@@ -19,29 +19,72 @@
package org.apache.ofbiz.widget.model;
import java.nio.file.Paths;
+
import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.UtilValidate;
+/**
+ * Central gatekeeper for every widget XML resource location (screen, form,
grid, menu, tree) before
+ * it is resolved and parsed. A widget location can be influenced by request
context (for example a
+ * decorator's {@code location} attribute is commonly a Flexible String
Expression such as
+ * {@code ${parameters.mainDecoratorLocation}}), so this class treats every
location as untrusted and
+ * only allows it through when it is either:
+ * <ul>
+ * <li>a {@code component://} location with no {@code ..} traversal segment,
or</li>
+ * <li>a bare filesystem path matched by the administrator-configured
+ * {@code security.allowFilePaths} allowlist (denied by default).</li>
+ * </ul>
+ * Any {@code file:} scheme location - single-slash or otherwise, in any
letter case - is rejected
+ * outright at the protocol layer: it is never a legitimate widget location,
and on Linux it is the
+ * carrier used to reach live process-descriptor aliases such as {@code
file:/dev/fd/N} or
+ * {@code file:/proc/thread-self/fd/N}.
+ */
public final class WidgetSecureLocation {
private static final String MODULE = WidgetSecureLocation.class.getName();
- private static final String COMPO_TYPE = "component://";
+ private static final String COMPONENT_PROTOCOL = "component://";
+
+ private WidgetSecureLocation() { }
+ /**
+ * Sanitizes a widget resource location.
+ * @param location the raw, potentially untrusted location
+ * @return the (possibly normalized) location if it is allowed, or {@code
null} if it must be rejected
+ */
public static String sanitize(String location) {
- if (UtilValidate.isEmpty(location)
- ||
UtilValidate.isUrlInStringAndDoesNotStartByComponentProtocol(location)
- || location.startsWith("file:/")) {
+ if (UtilValidate.isEmpty(location)) {
+ Debug.logWarning("Unable to sanitize an empty widget location",
MODULE);
+ return null;
+ }
+ if (isFileScheme(location) ||
UtilValidate.isUrlInStringAndDoesNotStartByComponentProtocol(location)) {
Debug.logWarning(String.format("Unable to sanitize location:
[%s]", location), MODULE);
return null;
}
- if (location.startsWith(COMPO_TYPE) && location.length() > 12) {
- if (location.indexOf("..") > 0) {
- Debug.logWarning(String.format("For security reason traversal
sequence '..' is not allowed: [%s]", location), MODULE);
+ if (location.startsWith(COMPONENT_PROTOCOL)) {
+ String componentRelativePath =
location.substring(COMPONENT_PROTOCOL.length());
+ if (componentRelativePath.contains("..")) {
+ Debug.logWarning(String.format("Traversal sequence '..' is not
allowed in location: [%s]", location), MODULE);
return null;
}
- return COMPO_TYPE + Paths.get(location.substring(12)).normalize();
+ return COMPONENT_PROTOCOL +
Paths.get(componentRelativePath).normalize();
}
+ if (UtilValidate.isAllowedPath(location)) {
+ return location;
+ }
+ Debug.logWarning(String.format("Location isn't on the configured
allowed file path: [%s]", location), MODULE);
+ return null;
+ }
- return location.startsWith(COMPO_TYPE) ? location : null;
+ /**
+ * Detects the {@code file:} URI scheme regardless of case or slash count,
e.g. {@code file:/},
+ * {@code File:/}, {@code FILE://}. Java's URL/{@code FlexibleLocation}
scheme resolution is
+ * case-insensitive, so this check must be too.
+ */
+ private static boolean isFileScheme(String location) {
+ int colonIndex = location.indexOf(':');
+ if (colonIndex < 0) {
+ return false;
+ }
+ return "file".equalsIgnoreCase(location.substring(0, colonIndex));
}
}
diff --git
a/framework/widget/src/test/java/org/apache/ofbiz/widget/model/WidgetSecureLocationTests.java
b/framework/widget/src/test/java/org/apache/ofbiz/widget/model/WidgetSecureLocationTests.java
new file mode 100644
index 0000000000..b608514dd1
--- /dev/null
+++
b/framework/widget/src/test/java/org/apache/ofbiz/widget/model/WidgetSecureLocationTests.java
@@ -0,0 +1,98 @@
+/*
+ * 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.widget.model;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import org.apache.ofbiz.base.util.UtilProperties;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+public final class WidgetSecureLocationTests {
+
+ @AfterEach
+ public void resetAllowFilePaths() {
+ UtilProperties.setPropertyValueInMemory("security", "allowFilePaths",
"");
+ }
+
+ @Test
+ public void allowsComponentLocation() {
+ assertEquals("component://common/widget/CommonScreens.xml",
+
WidgetSecureLocation.sanitize("component://common/widget/CommonScreens.xml"));
+ }
+
+ @Test
+ public void rejectsTraversalInComponentLocation() {
+
assertNull(WidgetSecureLocation.sanitize("component://common/widget/../../../etc/passwd"));
+ }
+
+ @Test
+ public void rejectsEmptyOrNullLocation() {
+ assertNull(WidgetSecureLocation.sanitize(null));
+ assertNull(WidgetSecureLocation.sanitize(""));
+ }
+
+ @Test
+ public void rejectsNonComponentUrl() {
+
assertNull(WidgetSecureLocation.sanitize("http://evil.example/widget.xml";));
+ }
+
+ // The single-slash `file:` scheme is the exact carrier used in the
reported
+ // pre-auth RCE chain (file:/dev/fd/N, file:/proc/thread-self/fd/N, ...).
+ // It must be rejected at the protocol layer, independent of any configured
+ // allowFilePaths pattern, and independent of scheme case.
+ @Test
+ public void rejectsFileSchemeDescriptorPaths() {
+ assertNull(WidgetSecureLocation.sanitize("file:/dev/fd/292"));
+ assertNull(WidgetSecureLocation.sanitize("file:/proc/self/fd/292"));
+
assertNull(WidgetSecureLocation.sanitize("file:/proc/thread-self/fd/292"));
+ assertNull(WidgetSecureLocation.sanitize("file:/proc/%73elf/fd/292"));
+ }
+
+ @Test
+ public void rejectsFileSchemeRegardlessOfCase() {
+ // Java's URL/FlexibleLocation scheme resolution is case-insensitive,
so the
+ // literal-lowercase check alone (as first proposed in PR #1586) can
be bypassed.
+ assertNull(WidgetSecureLocation.sanitize("File:/dev/fd/292"));
+ assertNull(WidgetSecureLocation.sanitize("FILE:/dev/fd/292"));
+ assertNull(WidgetSecureLocation.sanitize("FiLe:/dev/fd/292"));
+ }
+
+ @Test
+ public void rejectsFileSchemeEvenWhenAllowFilePathsIsPermissive() {
+ // Defense-in-depth: an administrator-configured allowFilePaths regex
that is
+ // too broad must not resurrect the file: scheme bypass.
+ UtilProperties.setPropertyValueInMemory("security", "allowFilePaths",
".*");
+ assertNull(WidgetSecureLocation.sanitize("file:/dev/fd/292"));
+ assertNull(WidgetSecureLocation.sanitize("FILE:/dev/fd/292"));
+ }
+
+ @Test
+ public void deniesBarePathByDefault() {
+ UtilProperties.setPropertyValueInMemory("security", "allowFilePaths",
"");
+
assertNull(WidgetSecureLocation.sanitize("/opt/ofbiz/templates/foo.ftl"));
+ }
+
+ @Test
+ public void allowsBarePathOnceConfigured() {
+ UtilProperties.setPropertyValueInMemory("security", "allowFilePaths",
"/opt/ofbiz/templates/.*");
+ assertEquals("/opt/ofbiz/templates/foo.ftl",
WidgetSecureLocation.sanitize("/opt/ofbiz/templates/foo.ftl"));
+ }
+}
