CVE-2011-1088

@ServletSecurity Servlets added via addServlet() should not be processed unless created via craeteServlet. Need to delay scanning until urlPatterns are known git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1077995 13f79535-47bb-0310-9956-ffa450edef68
apache · Mar 4, 2011 · ece65c1 · ece65c1
1 parent 46bfef6
commit ece65c1
Show file tree

Hide file tree

Showing 5 changed files with 139 additions and 15 deletions.
diff --git a/java/org/apache/catalina/Wrapper.java b/java/org/apache/catalina/Wrapper.java
@@ -371,4 +371,19 @@ public void setMultipartConfigElement(
      */
     public void setEnabled(boolean enabled);
 
+    /**
+     * Set the flag that indicates
+     * {@link javax.servlet.annotation.ServletSecurity} annotations must be
+     * scanned when the Servlet is first used.
+     * 
+     * @param b The new value of the flag
+     */
+    public void setServletSecurityAnnotationScanRequired(boolean b);
+
+    /**
+     * Scan for (if necessary) and process (if found) the
+     * {@link javax.servlet.annotation.ServletSecurity} annotations for the
+     * Servlet associated with this wrapper.
+     */
+    public void servletSecurityAnnotationScan() throws ServletException;
 }
diff --git a/java/org/apache/catalina/authenticator/AuthenticatorBase.java b/java/org/apache/catalina/authenticator/AuthenticatorBase.java
@@ -482,9 +482,7 @@ public void invoke(Request request, Response response)
         // The Servlet may specify security constraints through annotations.
         // Ensure that they have been processed before constraints are checked
         Wrapper wrapper = (Wrapper) request.getMappingData().wrapper; 
-        if (wrapper.getServlet() == null) {
-            wrapper.load();
-        }
+        wrapper.servletSecurityAnnotationScan();
 
         Realm realm = this.context.getRealm();
         // Is this request URI subject to a security constraint?

diff --git a/java/org/apache/catalina/core/StandardContext.java b/java/org/apache/catalina/core/StandardContext.java
@@ -837,6 +837,12 @@ public StandardContext() {
 
     private boolean fireRequestListenersOnForwards = false;
 
+    /**
+     * Servlets created via {@link ApplicationContext#createServlet(Class)} for
+     * tracking purposes.
+     */
+    private Set<Servlet> createdServlets = new HashSet<Servlet>();
+
     // ----------------------------------------------------- Context Properties
 
 
@@ -4364,6 +4370,11 @@ public String getRealPath(String path) {
      * @param wrapper   The wrapper for the Servlet that was added
      */
     public ServletRegistration.Dynamic dynamicServletAdded(Wrapper wrapper) {
+        Servlet s = wrapper.getServlet();
+        if (s != null && createdServlets.contains(s)) {
+            // Mark the wrapper to indicate annotations need to be scanned
+            wrapper.setServletSecurityAnnotationScanRequired(true);
+        }
         return new ApplicationServletRegistration(wrapper, this);
     }
 
@@ -4372,7 +4383,7 @@ public ServletRegistration.Dynamic dynamicServletAdded(Wrapper wrapper) {
      * @param servlet
      */
     public void dynamicServletCreated(Servlet servlet) {
-        // NOOP - Hook for JACC implementations
+        createdServlets.add(servlet);
     }
 
 
@@ -5530,6 +5541,8 @@ private void resetContext() throws Exception {
 
         initializers.clear();
 
+        createdServlets.clear();
+
         if(log.isDebugEnabled())
             log.debug("resetContext " + getObjectName());
     }

diff --git a/java/org/apache/catalina/core/StandardWrapper.java b/java/org/apache/catalina/core/StandardWrapper.java
@@ -273,6 +273,8 @@ public StandardWrapper() {
      */
     protected boolean enabled = true;
 
+    protected volatile boolean servletSecurityAnnotationScanRequired = false;
+
     /**
      * Static class array used when the SecurityManager is turned on and 
      * <code>Servlet.init</code> is invoked.
@@ -643,6 +645,15 @@ public void setServlet(Servlet servlet) {
         instance = servlet;
     }
 
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public void setServletSecurityAnnotationScanRequired(boolean b) {
+        this.servletSecurityAnnotationScanRequired = b;
+    }
+
     // --------------------------------------------------------- Public Methods
 
 
@@ -1077,20 +1088,12 @@ public synchronized Servlet loadServlet() throws ServletException {
                 }
             }
 
-            ServletSecurity secAnnotation =
-                servlet.getClass().getAnnotation(ServletSecurity.class);
-            Context ctxt = (Context) getParent();
-            if (secAnnotation != null) {
-                ctxt.addServletSecurity(
-                        new ApplicationServletRegistration(this, ctxt),
-                        new ServletSecurityElement(secAnnotation));
-            }
-
+            processServletSecurityAnnotation(servlet);
 
             // Special handling for ContainerServlet instances
             if ((servlet instanceof ContainerServlet) &&
-                  (isContainerProvidedServlet(servletClass) ||
-                    ctxt.getPrivileged() )) {
+                    (isContainerProvidedServlet(servletClass) ||
+                            ((Context) getParent()).getPrivileged() )) {
                 ((ContainerServlet) servlet).setWrapper(this);
             }
 
@@ -1123,6 +1126,34 @@ public synchronized Servlet loadServlet() throws ServletException {
 
     }
 
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public void servletSecurityAnnotationScan() throws ServletException {
+        if (instance == null) {
+            load();
+        } else {
+            if (servletSecurityAnnotationScanRequired) {
+                processServletSecurityAnnotation(instance);
+            }
+        }
+    }
+
+    private void processServletSecurityAnnotation(Servlet servlet) {
+        // Calling this twice isn't harmful so no syncs
+        servletSecurityAnnotationScanRequired = false;
+
+        ServletSecurity secAnnotation =
+            servlet.getClass().getAnnotation(ServletSecurity.class);
+        Context ctxt = (Context) getParent();
+        if (secAnnotation != null) {
+            ctxt.addServletSecurity(
+                    new ApplicationServletRegistration(this, ctxt),
+                    new ServletSecurityElement(secAnnotation));
+        }
+    }
+
     private synchronized void initServlet(Servlet servlet)
             throws ServletException {
 

diff --git a/test/org/apache/catalina/core/TestStandardWrapper.java b/test/org/apache/catalina/core/TestStandardWrapper.java
@@ -23,8 +23,13 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
+import javax.servlet.Servlet;
+import javax.servlet.ServletContainerInitializer;
+import javax.servlet.ServletContext;
 import javax.servlet.ServletException;
+import javax.servlet.ServletRegistration;
 import javax.servlet.annotation.HttpConstraint;
 import javax.servlet.annotation.HttpMethodConstraint;
 import javax.servlet.annotation.ServletSecurity;
@@ -112,6 +117,43 @@ public void testSecurityAnnotationsMetaDataPriority() throws Exception {
         assertEquals(200, rc);
     }
 
+    public void testSecurityAnnotationsAddServlet1() throws Exception {
+        doTestSecurityAnnotationsAddServlet(false);
+    }
+
+    public void testSecurityAnnotationsAddServlet2() throws Exception {
+        doTestSecurityAnnotationsAddServlet(true);
+    }
+
+    private void doTestSecurityAnnotationsAddServlet(boolean useCreateServlet)
+            throws Exception {
+
+        // Setup Tomcat instance
+        Tomcat tomcat = getTomcatInstance();
+
+        // Must have a real docBase - just use temp
+        Context ctx =
+            tomcat.addContext("", System.getProperty("java.io.tmpdir"));
+
+        Servlet s = new DenyAllServlet();
+        ServletContainerInitializer sci = new SCI(s, useCreateServlet);
+        ctx.addServletContainerInitializer(sci, null);
+
+        tomcat.start();
+
+        ByteChunk bc = new ByteChunk();
+        int rc;
+        rc = getUrl("http://localhost:" + getPort() + "/", bc, null, null);
+
+        if (useCreateServlet) {
+            assertNull(bc.toString());
+            assertEquals(403, rc);
+        } else {
+            assertEquals("OK", bc.toString());
+            assertEquals(200, rc);
+        }
+    }
+
     private void doTest(String servletClassName, boolean usePost,
             boolean useRole, boolean expect200) throws Exception {
 
@@ -217,4 +259,29 @@ public static class RoleAllowServlet extends TestServlet {
     public static class RoleDenyServlet extends TestServlet {
         private static final long serialVersionUID = 1L;
     }
+
+    public static class SCI implements ServletContainerInitializer {
+
+        private Servlet servlet;
+        private boolean createServlet;
+
+        public SCI(Servlet servlet, boolean createServlet) {
+            this.servlet = servlet;
+            this.createServlet = createServlet;
+        }
+
+        @Override
+        public void onStartup(Set<Class<?>> c, ServletContext ctx)
+                throws ServletException {
+            Servlet s;
+
+            if (createServlet) {
+                s = ctx.createServlet(servlet.getClass());
+            } else {
+                s = servlet;
+            }
+            ServletRegistration.Dynamic r = ctx.addServlet("servlet", s);
+            r.addMapping("/");
+        }
+    }
 }