This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch release17.12
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit fb495637441cfe331943d34ce2d0943bc8c30552
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
Conflicts: ContextFilter.java handled by hand
---
.../org/apache/ofbiz/base/util/CacheFilter.java | 69 +++++++++
.../org/apache/ofbiz/base/util/RequestWrapper.java | 172 +++++++++++++++++++++
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, 253 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..95f87f0
--- /dev/null
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/CacheFilter.java
@@ -0,0 +1,69 @@
+/*******************************************************************************
+ * 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;
+
+ public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain) throws IOException, ServletException {
+ // Read request.getBody() 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);
+ }
+
+ public void init(FilterConfig filterConfiguration) throws ServletException
{
+ setFilterConfig(filterConfiguration);
+ }
+
+ 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..67ac1fe
--- /dev/null
+++
b/framework/base/src/main/java/org/apache/ofbiz/base/util/RequestWrapper.java
@@ -0,0 +1,172 @@
+/*******************************************************************************
+ * 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 arg0) {
+ super(arg0);
+ origRequest = arg0;
+ }
+
+ public BufferedReader getReader() throws IOException {
+
+ getBytes();
+
+ InputStreamReader dave = new InputStreamReader(new
ByteArrayInputStream(reqBytes));
+ BufferedReader br = new BufferedReader(dave);
+ return br;
+
+ }
+
+ 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
+ public boolean isFinished() {
+ // TODO Auto-generated method stub
+ return false;
+ }
+
+ @Override
+ public boolean isReady() {
+ // TODO Auto-generated method stub
+ return false;
+ }
+
+ @Override
+ public void setReadListener(ReadListener listener) {
+ // TODO Auto-generated method stub
+
+ }
+
+ };
+
+ return sis;
+ }
+
+ public byte[] getBytes() throws IOException {
+ if (firstTime) {
+ firstTime = false;
+ // Read the parameters first, because they can get reachless 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 0c6d20c..3fb82fb 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 6edd2aa..fbbfb87 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;
@@ -95,13 +94,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 0f6a3d5..3109316 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>
