CVE-2014-8122

@@ -18,9 +18,6 @@
 
 import static org.jboss.weld.logging.WeldLogger.WELD_PROJECT_CODE;
 
-import javax.enterprise.context.spi.Context;
-import javax.servlet.http.HttpServletRequest;
-
 import org.jboss.logging.Logger;
 import org.jboss.logging.Logger.Level;
 import org.jboss.logging.annotations.Cause;
@@ -60,8 +57,8 @@ public interface ServletLogger extends WeldLogger {
     void webXmlMappingPatternIgnored(String pattern);
 
     @LogMessage(level = Level.WARN)
-    @Message(id = 712, value = "Unable to dissociate context {0} when destroying request {1}", format = Format.MESSAGE_FORMAT)
-    void unableToDissociateContext(Context context, HttpServletRequest request);
+    @Message(id = 712, value = "Unable to dissociate context {0} from the storage {1}", format = Format.MESSAGE_FORMAT)
+    void unableToDissociateContext(Object context, Object storage);
 
     @Message(id = 713, value = "Unable to inject ServletContext. None is associated with {0}, {1}", format = Format.MESSAGE_FORMAT)
     IllegalStateException cannotInjectServletContext(ClassLoader classLoader, ServletContextService service);
@@ -78,4 +75,8 @@ public interface ServletLogger extends WeldLogger {
     @Message(id = 716, value = "Running in Servlet 2.x environment. Asynchronous request support is disabled.")
     void servlet2Environment();
 
+    @LogMessage(level = Level.WARN)
+    @Message(id = 717, value = "Unable to deactivate context {0} when destroying request {1}", format = Format.MESSAGE_FORMAT)
+    void unableToDeactivateContext(Object context, Object request);
+
 }
\ No newline at end of file

@@ -138,30 +138,35 @@ private boolean isContextActivatedInRequest(HttpServletRequest request) {
     }
 
     protected void deactivateConversationContext(HttpServletRequest request) {
-        ConversationContext conversationContext = httpConversationContext();
-        if (conversationContext.isActive()) {
-            // Only deactivate the context if one is already active, otherwise we get Exceptions
-            if (conversationContext instanceof LazyHttpConversationContextImpl) {
-                LazyHttpConversationContextImpl lazyConversationContext = (LazyHttpConversationContextImpl) conversationContext;
-                if (!lazyConversationContext.isInitialized()) {
-                    // if this lazy conversation has not been touched yet, just deactivate it
-                    lazyConversationContext.deactivate();
-                    return;
+        try {
+            ConversationContext conversationContext = httpConversationContext();
+            if (conversationContext.isActive()) {
+                // Only deactivate the context if one is already active, otherwise we get Exceptions
+                if (conversationContext instanceof LazyHttpConversationContextImpl) {
+                    LazyHttpConversationContextImpl lazyConversationContext = (LazyHttpConversationContextImpl) conversationContext;
+                    if (!lazyConversationContext.isInitialized()) {
+                        // if this lazy conversation has not been touched yet, just deactivate it
+                        lazyConversationContext.deactivate();
+                        return;
+                    }
                 }
-            }
-            boolean isTransient = conversationContext.getCurrentConversation().isTransient();
-            if (ConversationLogger.LOG.isTraceEnabled()) {
+                boolean isTransient = conversationContext.getCurrentConversation().isTransient();
+                if (ConversationLogger.LOG.isTraceEnabled()) {
+                    if (isTransient) {
+                        ConversationLogger.LOG.cleaningUpTransientConversation();
+                    } else {
+                        ConversationLogger.LOG.cleaningUpConversation(conversationContext.getCurrentConversation().getId());
+                    }
+                }
+                conversationContext.invalidate();
+                conversationContext.deactivate();
                 if (isTransient) {
-                    ConversationLogger.LOG.cleaningUpTransientConversation();
-                } else {
-                    ConversationLogger.LOG.cleaningUpConversation(conversationContext.getCurrentConversation().getId());
+                    conversationDestroyedEvent.fire(request);
                 }
             }
-            conversationContext.invalidate();
-            conversationContext.deactivate();
-            if (isTransient) {
-                conversationDestroyedEvent.fire(request);
-            }
+        } catch (Exception e) {
+            ServletLogger.LOG.unableToDeactivateContext(httpConversationContext(), request);
+            ServletLogger.LOG.catchingDebug(e);
         }
     }
 

@@ -23,6 +23,8 @@
 
 import org.jboss.weld.Container;
 import org.jboss.weld.bootstrap.api.Service;
+import org.jboss.weld.context.BoundContext;
+import org.jboss.weld.context.ManagedContext;
 import org.jboss.weld.context.cache.RequestScopedCache;
 import org.jboss.weld.context.http.HttpRequestContext;
 import org.jboss.weld.context.http.HttpRequestContextImpl;
@@ -279,24 +281,21 @@ public void requestDestroyed(HttpServletRequest request) {
             if (!servletApi.isAsyncSupported() || !servletApi.isAsyncStarted(request)) {
                 getRequestContext().invalidate();
             }
-            getRequestContext().deactivate();
+
+            safelyDeactivate(getRequestContext(),  request);
             // fire @Destroyed(RequestScoped.class)
             requestDestroyedEvent.fire(request);
-            getSessionContext().deactivate();
+
+            safelyDeactivate(getSessionContext(), request);
             // fire @Destroyed(SessionScoped.class)
             if (!getSessionContext().isValid()) {
                 sessionDestroyedEvent.fire((HttpSession) request.getAttribute(HTTP_SESSION));
             }
         } finally {
-            getRequestContext().dissociate(request);
-
+            safelyDissociate(getRequestContext(), request);
             // WFLY-1533 Underlying HTTP session may be invalid
-            try {
-                getSessionContext().dissociate(request);
-            } catch (Exception e) {
-                ServletLogger.LOG.unableToDissociateContext(getSessionContext(), request);
-                ServletLogger.LOG.catchingDebug(e);
-            }
+            safelyDissociate(getSessionContext(), request);
+
             // Catch block is inside the activator method so that we're able to log the context
             conversationContextActivator.disassociateConversationContext(request);
 
@@ -312,6 +311,10 @@ public void setConversationActivationEnabled(boolean conversationActivationEnabl
         this.conversationActivationEnabled = conversationActivationEnabled;
     }
 
+    @Override
+    public void cleanup() {
+    }
+
     /**
      * Some Servlet containers fire HttpServletListeners for include requests (inner requests caused by calling the include method of RequestDispatcher). This
      * causes problems with context shut down as context manipulation is not reentrant. This method detects if this request is an included request or not.
@@ -338,7 +341,22 @@ private boolean isRequestDestroyed(HttpServletRequest request) {
         return request.getAttribute(REQUEST_DESTROYED) != null;
     }
 
-    @Override
-    public void cleanup() {
+    private <T> void safelyDissociate(BoundContext<T> context, T storage) {
+        try {
+            context.dissociate(storage);
+        } catch(Exception e) {
+            ServletLogger.LOG.unableToDissociateContext(context, storage);
+            ServletLogger.LOG.catchingDebug(e);
+        }
+    }
+
+    private void safelyDeactivate(ManagedContext context, HttpServletRequest request) {
+        try {
+            context.deactivate();
+        } catch(Exception e) {
+            ServletLogger.LOG.unableToDeactivateContext(context, request);
+            ServletLogger.LOG.catchingDebug(e);
+        }
     }
+
 }

@@ -71,6 +71,8 @@ public void invalidate() {
     }
 
     public static void beginRequest() {
+        // if the previous request was not ended properly for some reason, make sure it is ended now
+        endRequest();
         CACHE.set(new LinkedList<RequestScopedItem>());
     }
 
@@ -80,8 +82,8 @@ public static void beginRequest() {
      */
     public static void endRequest() {
         final List<RequestScopedItem> result = CACHE.get();
-        CACHE.remove();
         if (result != null) {
+            CACHE.remove();
             for (final RequestScopedItem item : result) {
                 item.invalidate();
             }

@@ -0,0 +1,33 @@
+/*
+ * JBoss, Home of Professional Open Source
+ * Copyright 2014, Red Hat, Inc., and individual contributors
+ * by the @authors tag. See the copyright.txt in the distribution for a
+ * full listing of individual contributors.
+ *
+ * Licensed 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.jboss.weld.tests.contexts.cache;
+
+import java.io.Serializable;
+
+import javax.enterprise.context.ConversationScoped;
+
+@ConversationScoped
+public class ConversationScopedBean implements Serializable {
+
+    private String value = "foo";
+
+    public String getAndSet(String newValue) {
+        String old = value;
+        value = newValue;
+        return old;
+    }
+}

@@ -0,0 +1,80 @@
+/*
+ * JBoss, Home of Professional Open Source
+ * Copyright 2014, Red Hat, Inc., and individual contributors
+ * by the @authors tag. See the copyright.txt in the distribution for a
+ * full listing of individual contributors.
+ *
+ * Licensed 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.jboss.weld.tests.contexts.cache;
+
+import java.io.IOException;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+import junit.framework.Assert;
+
+import org.jboss.arquillian.container.test.api.Deployment;
+import org.jboss.arquillian.junit.Arquillian;
+import org.jboss.arquillian.test.api.ArquillianResource;
+import org.jboss.shrinkwrap.api.ShrinkWrap;
+import org.jboss.shrinkwrap.api.asset.EmptyAsset;
+import org.jboss.shrinkwrap.api.spec.WebArchive;
+import org.jboss.weld.tests.category.Integration;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
+import com.gargoylesoftware.htmlunit.WebClient;
+
+@RunWith(Arquillian.class)
+@Category(Integration.class)
+public class RequestScopedCacheLeakTest {
+
+    @ArquillianResource
+    private URL contextPath;
+
+    @Deployment(testable = false)
+    public static WebArchive getDeployment() {
+        return ShrinkWrap.create(WebArchive.class).addAsWebInfResource(EmptyAsset.INSTANCE, "beans.xml")
+                .addClasses(SimpleServlet.class, ConversationScopedBean.class);
+    }
+
+    @Test
+    public void test() throws Exception {
+        WebClient webClient = new WebClient();
+        webClient.setThrowExceptionOnFailingStatusCode(false);
+        for (int i = 0; i < 100; i++) {
+            // first, send out a hundred of poisoning requests
+            // each of these should leave a thread in a broken state
+            sendRequest(webClient, i, true);
+        }
+        for (int i = 0; i < 100; i++) {
+            // now send out normal requests to see if they are affected by the thread's broken state
+            String result = sendRequest(webClient, i, false);
+            Assert.assertFalse("Invalid state detected after " + (i + 1) + " requests", result.startsWith("bar"));
+        }
+    }
+
+    private String sendRequest(WebClient webClient, int sequence, boolean poison) throws FailingHttpStatusCodeException, MalformedURLException, IOException {
+        final String path = getPath("getAndSet", sequence, poison);
+        return webClient.getPage(path).getWebResponse().getContentAsString().trim();
+    }
+
+    private String getPath(String test, int sequence, boolean poison) {
+        String path = contextPath + "/servlet?action=" + test + "&sequence=" + sequence;
+        if (poison) {
+            path += "&poison=true";
+        }
+        return path;
+    }
+}

@@ -0,0 +1,55 @@
+/*
+ * JBoss, Home of Professional Open Source
+ * Copyright 2014, Red Hat, Inc., and individual contributors
+ * by the @authors tag. See the copyright.txt in the distribution for a
+ * full listing of individual contributors.
+ *
+ * Licensed 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.jboss.weld.tests.contexts.cache;
+
+import java.io.IOException;
+
+import javax.inject.Inject;
+import javax.servlet.ServletException;
+import javax.servlet.annotation.WebServlet;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import org.jboss.weld.context.beanstore.ConversationNamingScheme;
+
+@WebServlet(value = "/servlet", asyncSupported = true)
+public class SimpleServlet extends HttpServlet {
+
+    @Inject
+    private ConversationScopedBean bean;
+
+    @Override
+    protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
+        String action = req.getParameter("action");
+        String sequence = req.getParameter("sequence");
+        String poison = req.getParameter("poison");
+        if ("getAndSet".equals(action)) {
+            // the value should always be foo
+            String value = bean.getAndSet("bar" + sequence);
+            resp.getWriter().println(value);
+            if (poison != null) {
+                // this is a poisoning request
+                // normal applications should never do something like this
+                // we just do this to cause an exception to be thrown out of ConversationContext.deactivate
+                req.removeAttribute(ConversationNamingScheme.PARAMETER_NAME);
+            }
+        } else {
+            throw new IllegalArgumentException(action);
+        }
+    }
+}