From b5a2e4f02e0be7079be508040b299d1d9229b716 Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Wed, 12 Aug 2026 12:15:37 +0530 Subject: [PATCH] Restrict widget resource locations to component:// or an explicit allowlist 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. --- .../widget/ofbizsetup/ProfileScreens.xml | 4 +- .../apache/ofbiz/base/util/UtilValidate.java | 26 +++-- .../ofbiz/base/util/UtilValidateTests.java | 17 ++++ framework/security/config/security.properties | 9 ++ .../org/apache/ofbiz/webapp/WebAppUtil.java | 10 ++ .../apache/ofbiz/webapp/WebAppUtilTests.java | 97 ++++++++++++++++++ .../ofbiz/widget/model/FormFactory.java | 19 +++- .../ofbiz/widget/model/GridFactory.java | 19 +++- .../ofbiz/widget/model/MenuFactory.java | 17 +++- .../ofbiz/widget/model/ScreenFactory.java | 2 +- .../ofbiz/widget/model/TreeFactory.java | 9 +- .../widget/model/WidgetSecureLocation.java | 61 ++++++++++-- .../model/WidgetSecureLocationTests.java | 98 +++++++++++++++++++ 13 files changed, 353 insertions(+), 35 deletions(-) create mode 100644 framework/webapp/src/test/java/org/apache/ofbiz/webapp/WebAppUtilTests.java create mode 100644 framework/widget/src/test/java/org/apache/ofbiz/widget/model/WidgetSecureLocationTests.java diff --git a/applications/commonext/widget/ofbizsetup/ProfileScreens.xml b/applications/commonext/widget/ofbizsetup/ProfileScreens.xml index a2a5ae72321..e0269bd4db6 100644 --- a/applications/commonext/widget/ofbizsetup/ProfileScreens.xml +++ b/applications/commonext/widget/ofbizsetup/ProfileScreens.xml @@ -90,8 +90,8 @@ - - + + 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 4a9b3d96b88..5aa77244f62 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 @@ private 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 static boolean isValidUrl(String s) { } /** - * 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 security.allowFilePaths 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 component:// 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 ba26572a778..1f20f35f2cc 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 void testUrlValidations() throws Exception { 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 3231aa6de57..234c06531b9 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 ebcd9d41b34..3b258589345 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 static void setAttributesFromRequestBody(ServletRequest request) { Debug.logWarning(ioe, MODULE); } if (requestBodyMap != null) { + ServletContext servletContext = request.getServletContext(); Set 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 00000000000..d5ae92adf23 --- /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 41b185ec16f..9f7617809d9 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 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 static ModelForm getFormFromLocation(String resourceName, String formName 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 static ModelForm getFormFromWebappContext(String resourceName, String for 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 f7907b310d4..20a6603f7fd 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 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 static ModelGrid getGridFromLocation(String resourceName, String gridName 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 static ModelGrid getGridFromWebappContext(String resourceName, String gri 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 afbfebde0a4..171c26f354f 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 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 static ModelMenu getMenuFromWebappContext(String resourceName, String men 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 static ModelMenu getMenuFromLocation(String resourceName, String menuName String keyName = resourceName + "::" + visualTheme.getVisualThemeId(); Map 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 739816fb449..bff5cd2ad27 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 static void renderReferencedScreen(String name, String location, ModelScr 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 c32e3817414..c58d531dfaa 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 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 static ModelTree getTreeFromLocation(String resourceName, String treeName throws IOException, SAXException, ParserConfigurationException { Map 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 8c87b7b640f..16a6e2f85e5 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: + *
    + *
  • a {@code component://} location with no {@code ..} traversal segment, or
    • + *
  • a bare filesystem path matched by the administrator-configured + * {@code security.allowFilePaths} allowlist (denied by default).
    • + *
+ * 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 00000000000..b608514dd1b --- /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")); + } +}