This is an automated email from the ASF dual-hosted git repository. jleroux pushed a commit to branch release18.12 in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit a1a24bd9100ccd16732a92eed61e4f7c05d90ca7 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 handled by hand CacheFilter.java RequestWrapper.java --- .../org/apache/ofbiz/base/util/CacheFilter.java | 58 +++++++++++++++++++--- .../org/apache/ofbiz/base/util/RequestWrapper.java | 32 ++++++++---- 2 files changed, 74 insertions(+), 16 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 index 95f87f0..de15e3f 100644 --- 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 @@ -33,21 +33,67 @@ 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 { - // 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; + // 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); } 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 index 67ac1fe..70ca6af 100644 --- 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 @@ -42,11 +42,14 @@ public class RequestWrapper extends HttpServletRequestWrapper { private boolean firstTime = true; private Map<String, String[]> parameterMap = null; - public RequestWrapper(HttpServletRequest arg0) { - super(arg0); - origRequest = arg0; + 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(); @@ -54,9 +57,11 @@ public class RequestWrapper extends HttpServletRequestWrapper { 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(); @@ -72,7 +77,6 @@ public class RequestWrapper extends HttpServletRequestWrapper { } else { b = -1; } - return b; } @@ -89,22 +93,27 @@ public class RequestWrapper extends HttpServletRequestWrapper { return len; } + /** + * Needed by Servlet 3.1 No worry this is not used in OFBiz code + */ @Override public boolean isFinished() { - // TODO Auto-generated method stub return false; } + /** + * Needed by Servlet 3.1 No worry this is not used in OFBiz code + */ @Override public boolean isReady() { - // TODO Auto-generated method stub return false; } + /** + * Needed by Servlet 3.1 No worry this is not used in OFBiz code + */ @Override public void setReadListener(ReadListener listener) { - // TODO Auto-generated method stub - } }; @@ -112,10 +121,13 @@ public class RequestWrapper extends HttpServletRequestWrapper { 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 get reachless after the inputStream is read. + // 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) {