This is an automated email from the ASF dual-hosted git repository.
jleroux 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 a5bdcc6 Fixed: post-auth Remote Code Execution Vulnerability
(OFBIZ-12332)
a5bdcc6 is described below
commit a5bdcc6f9ea59d5d614f64832d5b6acec8e81e97
Author: Jacques Le Roux <jacques.le.r...@les7arts.com>
AuthorDate: Sat Oct 9 19:25:33 2021 +0200
Fixed: post-auth Remote Code Execution Vulnerability (OFBIZ-12332)
This definitely solves all issues by introducing a CacheFilter and
RequestWrapper classes inspired by several works found on the Net.
Also moves the change introduced before in ContextFilter to CacheFilter.
The basic problem is that you only can use once
ServletRequest::getInputStream or the ServletRequest::getReader
Also not both, even once, ie they can be seen as same from this POV.
The integration tests all pass.
Also replace the checked String "</serializable>" by "</serializable" as,
like
reported by Jie Zhu, the former could be bypassed by using "</serializable
>"
Thanks: Jie Zhu for report
---
.../org/apache/ofbiz/base/util/CacheFilter.java | 115 +++++++++++++
.../org/apache/ofbiz/base/util/RequestWrapper.java | 184 +++++++++++++++++++++
framework/service/testdef/servicetests.xml | 7 +-
.../apache/ofbiz/webapp/control/ContextFilter.java | 8 -
framework/webtools/webapp/webtools/WEB-INF/web.xml | 9 +
5 files changed, 311 insertions(+), 12 deletions(-)
diff --git
a/framework/base/src/main/java/org/apache/ofbiz/base/util/CacheFilter.java
b/framework/base/src/main/java/org/apache/ofbiz/base/util/CacheFilter.java
new file mode 100644
index 0000000..de15e3f
--- /dev/null
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/CacheFilter.java
@@ -0,0 +1,115 @@
+/*******************************************************************************
+ * 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 java.io.IOException;
+import java.util.stream.Collectors;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+
+public class CacheFilter implements Filter {
+
+ private FilterConfig filterConfig = null;
+
+ /**
+ * The <code>doFilter</code> method of the Filter is called by the
container each time a request/response pair is passed through the chain due to
+ * a client request for a resource at the end of the chain. The
FilterChain passed in to this method allows the Filter to pass on the request
and
+ * response to the next entity in the chain.
+ * <p>
+ * A typical implementation of this method would follow the following
pattern:- <br>
+ * 1. Examine the request<br>
+ * 2. Optionally wrap the request object with a custom implementation to
filter content or headers for input filtering <br>
+ * 3. Optionally wrap the response object with a custom implementation to
filter content or headers for output filtering <br>
+ * 4. a) <strong>Either</strong> invoke the next entity in the chain using
the FilterChain object (<code>chain.doFilter()</code>), <br>
+ * 4. b) <strong>or</strong> not pass on the request/response pair to the
next entity in the filter chain to block the request processing<br>
+ * 5. Directly set headers on the response after invocation of the next
entity in the filter chain.
+ * @param request The request to process
+ * @param response The response associated with the request
+ * @param chain Provides access to the next filter in the chain for this
filter to pass the request and response to for further processing
+ * @throws IOException if an I/O error occurs during this filter's
processing of the request
+ * @throws ServletException if the processing fails for any other reason
+ */
+ public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain) throws IOException, ServletException {
+ // Get the request URI without the webapp mount point.
+ String context = ((HttpServletRequest) request).getContextPath();
+ String uriWithContext = ((HttpServletRequest) request).getRequestURI();
+ String uri = uriWithContext.substring(context.length());
+
+ if ("xmlrpc".equals(uri.toLowerCase())) {
+ // Read request.getReader() as many time you need
+ request = new RequestWrapper((HttpServletRequest) request);
+ String body =
request.getReader().lines().collect(Collectors.joining());
+ if (body.contains("</serializable")) {
+ Debug.logError("Content not authorised for security reason",
"CacheFilter"); // Cf. OFBIZ-12332
+ return;
+ }
+ }
+ chain.doFilter(request, response);
+ }
+
+ /**
+ * Called by the web container to indicate to a filter that it is being
placed into service. The servlet container calls the init method exactly
+ * once after instantiating the filter. The init method must complete
successfully before the filter is asked to do any filtering work.
+ * <p>
+ * The web container cannot place the filter into service if the init
method either:
+ * <ul>
+ * <li>Throws a ServletException</li>
+ * <li>Does not return within a time period defined by the web
container</li>
+ * </ul>
+ * The default implementation is a NO-OP.
+ * @param filterConfig The configuration information associated with the
filter instance being initialised
+ * @throws ServletException if the initialisation fails
+ */
+ public void init(FilterConfig filterConfiguration) throws ServletException
{
+ setFilterConfig(filterConfiguration);
+ }
+
+ /**
+ * Called by the web container to indicate to a filter that it is being
taken out of service. This method is only called once all threads within
+ * the filter's doFilter method have exited or after a timeout period has
passed. After the web container calls this method, it will not call the
+ * doFilter method again on this instance of the filter. <br>
+ * <br>
+ * This method gives the filter an opportunity to clean up any resources
that are being held (for example, memory, file handles, threads) and make
+ * sure that any persistent state is synchronized with the filter's
current state in memory. The default implementation is a NO-OP.
+ */
+ public void destroy() {
+ setFilterConfig(null);
+ }
+
+ /**
+ * @return the filterConfig
+ */
+ public FilterConfig getFilterConfig() {
+ return filterConfig;
+ }
+
+ /**
+ * @param filterConfig the filterConfig to set
+ */
+ public void setFilterConfig(FilterConfig filterConfig) {
+ this.filterConfig = filterConfig;
+ }
+
+}
diff --git
a/framework/base/src/main/java/org/apache/ofbiz/base/util/RequestWrapper.java
b/framework/base/src/main/java/org/apache/ofbiz/base/util/RequestWrapper.java
new file mode 100644
index 0000000..abd954c
--- /dev/null
+++
b/framework/base/src/main/java/org/apache/ofbiz/base/util/RequestWrapper.java
@@ -0,0 +1,184 @@
+/*******************************************************************************
+ * 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 java.io.BufferedReader;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.servlet.ReadListener;
+import javax.servlet.ServletInputStream;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+
+public class RequestWrapper extends HttpServletRequestWrapper {
+
+ private static final int INITIAL_BUFFER_SIZE = 1024;
+ private HttpServletRequest origRequest;
+ private byte[] reqBytes;
+ private boolean firstTime = true;
+ private Map<String, String[]> parameterMap = null;
+
+ public RequestWrapper(HttpServletRequest arg) {
+ super(arg);
+ origRequest = arg;
+ }
+
+ /**
+ * The default behavior of this method is to return getReader() on the
wrapped request object.
+ */
+ public BufferedReader getReader() throws IOException {
+
+ getBytes();
+
+ InputStreamReader dave = new InputStreamReader(new
ByteArrayInputStream(reqBytes));
+ BufferedReader br = new BufferedReader(dave);
+ return br;
+ }
+
+ /**
+ * The default behavior of this method is to return getInputStream() on
the wrapped request object.
+ */
+ public ServletInputStream getInputStream() throws IOException {
+
+ getBytes();
+
+ ServletInputStream sis = new ServletInputStream() {
+ private int numberOfBytesAlreadyRead;
+
+ @Override
+ public int read() throws IOException {
+ byte b;
+ if (reqBytes.length > numberOfBytesAlreadyRead) {
+ b = reqBytes[numberOfBytesAlreadyRead++];
+ } else {
+ b = -1;
+ }
+ return b;
+ }
+
+ @Override
+ public int read(byte[] b, int off, int len) throws IOException {
+ if (len > (reqBytes.length - numberOfBytesAlreadyRead)) {
+ len = reqBytes.length - numberOfBytesAlreadyRead;
+ }
+ if (len <= 0) {
+ return -1;
+ }
+ System.arraycopy(reqBytes, numberOfBytesAlreadyRead, b, off,
len);
+ numberOfBytesAlreadyRead += len;
+ return len;
+ }
+
+ @Override
+ /**
+ * Needed by Servlet 3.1 No worry this is not used in OFBiz code
+ */
+ public boolean isFinished() {
+ return false;
+ }
+
+ /**
+ * Needed by Servlet 3.1 No worry this is not used in OFBiz code
+ */
+ @Override
+ public boolean isReady() {
+ return false;
+ }
+
+ /**
+ * Needed by Servlet 3.1 No worry this is not used in OFBiz code
+ */
+ @Override
+ public void setReadListener(ReadListener listener) {
+ }
+
+ };
+
+ return sis;
+ }
+
+ /**
+ * Returns the bytes taken from the original request getInputStream
+ */
+ public byte[] getBytes() throws IOException {
+ if (firstTime) {
+ firstTime = false;
+ // Read the parameters first, because they can't be reached after
the inputStream is read.
+ getParameterMap();
+ int initialSize = origRequest.getContentLength();
+ if (initialSize < INITIAL_BUFFER_SIZE) {
+ initialSize = INITIAL_BUFFER_SIZE;
+ }
+ ByteArrayOutputStream baos = new
ByteArrayOutputStream(initialSize);
+ byte[] buf = new byte[1024];
+ InputStream is = origRequest.getInputStream();
+ int len = 0;
+ while (len >= 0) {
+ len = is.read(buf);
+ if (len > 0) {
+ baos.write(buf, 0, len);
+ }
+ }
+ reqBytes = baos.toByteArray();
+ }
+ return reqBytes;
+ }
+
+ @Override
+ public String getParameter(String name) {
+ parameterMap = UtilMisc.toMap(getParameterMap());
+ if (parameterMap != null) {
+ String[] a = parameterMap.get(name);
+ if (a == null || a.length == 0) {
+ return null;
+ }
+ return a[0];
+ }
+ return null;
+ }
+
+ @Override
+ public Map<String, String[]> getParameterMap() {
+ if (parameterMap == null) {
+ parameterMap = new HashMap<String, String[]>();
+ parameterMap.putAll(super.getParameterMap());
+ }
+ return parameterMap;
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public Enumeration getParameterNames() {
+ return Collections.enumeration(parameterMap.values());
+ }
+
+ @Override
+ public String[] getParameterValues(String name) {
+ return parameterMap.get(name);
+ }
+
+}
diff --git a/framework/service/testdef/servicetests.xml
b/framework/service/testdef/servicetests.xml
index 6d15539..817c063 100644
--- a/framework/service/testdef/servicetests.xml
+++ b/framework/service/testdef/servicetests.xml
@@ -66,14 +66,13 @@ under the License.
<test-case case-name="service-eca-global-event-exec-assert-data">
<entity-xml action="assert"
entity-xml-url="component://service/testdef/data/ServiceEcaGlobalEventAssertData.xml"/>
</test-case>
-
-<!-- Because of "post-auth Remote Code Execution Vulnerability" (OFBIZ-12332),
Temporarily comments out XMLRPC tests. -->
-<!-- <test-case case-name="service-xml-rpc">
+
+ <test-case case-name="service-xml-rpc">
<junit-test-suite
class-name="org.apache.ofbiz.service.test.XmlRpcTests"/>
</test-case>
<test-case case-name="service-xml-rpc-local-engine">
<service-test service-name="testXmlRpcClientAdd"/>
- </test-case> -->
+ </test-case>
<test-case case-name="load-data-service-permission-tests">
<entity-xml
entity-xml-url="component://service/testdef/data/PermissionServiceTestData.xml"/>
</test-case>
diff --git
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ContextFilter.java
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ContextFilter.java
index 6f3de70..69f89b1 100644
---
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ContextFilter.java
+++
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ContextFilter.java
@@ -20,7 +20,6 @@ package org.apache.ofbiz.webapp.control;
import java.io.IOException;
import java.util.Enumeration;
-import java.util.stream.Collectors;
import javax.servlet.Filter;
import javax.servlet.FilterChain;
@@ -91,13 +90,6 @@ public class ContextFilter implements Filter {
HttpServletRequest httpRequest = (HttpServletRequest) request;
HttpServletResponse httpResponse = (HttpServletResponse) response;
- String body =
request.getReader().lines().collect(Collectors.joining());
- if (body.contains("</serializable>")) {
- Debug.logError("Content not authorised for security reason",
MODULE); // Cf. OFBIZ-12332
- return;
- }
-
-
// ----- Servlet Object Setup -----
// set the ServletContext in the request for future use
diff --git a/framework/webtools/webapp/webtools/WEB-INF/web.xml
b/framework/webtools/webapp/webtools/WEB-INF/web.xml
index 2b7abff..d574622 100644
--- a/framework/webtools/webapp/webtools/WEB-INF/web.xml
+++ b/framework/webtools/webapp/webtools/WEB-INF/web.xml
@@ -59,6 +59,11 @@ under the License.
</init-param>
</filter>
<filter>
+ <display-name>CacheFilter</display-name>
+ <filter-name>CacheFilter</filter-name>
+ <filter-class>org.apache.ofbiz.base.util.CacheFilter</filter-class>
+ </filter>
+ <filter>
<display-name>ContextFilter</display-name>
<filter-name>ContextFilter</filter-name>
<filter-class>org.apache.ofbiz.webapp.control.ContextFilter</filter-class>
@@ -73,6 +78,10 @@ under the License.
<url-pattern>/*</url-pattern>
</filter-mapping>
<filter-mapping>
+ <filter-name>CacheFilter</filter-name>
+ <url-pattern>/*</url-pattern>
+ </filter-mapping>
+ <filter-mapping>
<filter-name>ContextFilter</filter-name>
<url-pattern>/*</url-pattern>
</filter-mapping>
