Fix FactoryFinder path resolution in Windows (#1831) (#1837)

This update ensures forward slashes are alow used for classpath
resolution, even on all Windows versions. Also add extra validation
to prevent path separators (slashes) in factory key names. Lastly added
some more tests for validation

This closes #1830

(cherry picked from commit bedb108)

Author: cshannon

activemq-client/src/main/java/org/apache/activemq/util/FactoryFinder.java

@@ -219,7 +219,12 @@ Class<T> getRequiredType() {
         return requiredType;
     }
 
-    private String resolvePath(final String key) throws InstantiationException {
+    String resolvePath(final String key) throws InstantiationException {
+        // Validate the key has no path separators
+        if (containsPathSeparators(key)) {
+            throw new InstantiationException("Provided key may not contain path separators");
+        }
+
         // Normalize the base path with the given key. This
         // will resolve/remove any relative ".." sections of the path.
         // Example: "/dir1/dir2/dir3/../file" becomes "/dir1/dir2/file"
@@ -231,7 +236,13 @@ private String resolvePath(final String key) throws InstantiationException {
             throw new InstantiationException("Provided key escapes the FactoryFinder configured directory");
         }
 
-        return resolvedPath.toString();
+        // replace any backslashes with forward slashes (may happen in some Windows versions)
+        return resolvedPath.toString().replace("\\", "/");
+    }
+
+    public static boolean containsPathSeparators(String str) {
+        // Check for forward and backslash
+        return str.contains("/") || str.contains("\\");
     }
 
     public static String buildAllowedImpls(Class<?>...classes) {

activemq-client/src/test/java/org/apache/activemq/util/FactoryFinderTest.java

@@ -17,18 +17,17 @@
 package org.apache.activemq.util;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import java.io.IOException;
-import java.util.List;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
 import org.apache.activemq.transport.TransportFactory;
 import org.apache.activemq.transport.nio.NIOTransportFactory;
-import org.apache.activemq.transport.tcp.SslTransport;
 import org.apache.activemq.transport.tcp.SslTransportFactory;
 import org.apache.activemq.transport.tcp.TcpTransportFactory;
 import org.apache.activemq.util.FactoryFinder.ObjectFactory;
@@ -50,12 +49,44 @@ public void testPathTraversal() throws Exception {
         try {
             finder.newInstance("../../tcp");
             fail("should have failed instantiation");
+        } catch (InstantiationException e) {
+            assertEquals("Provided key may not contain path separators",
+                    e.getMessage());
+        }
+    }
+
+    // Test path traversal attempts will throw an error
+    @Test
+    public void testPathTraversal2() throws Exception {
+        FactoryFinder<TransportFactory> finder
+                = new FactoryFinder<>(TRANSPORT_FACTORY_PATH, TransportFactory.class, null);
+        assertNull(finder.getAllowedImpls());
+
+        try {
+            finder.newInstance("..");
+            fail("should have failed instantiation");
         } catch (InstantiationException e) {
             assertEquals("Provided key escapes the FactoryFinder configured directory",
                     e.getMessage());
         }
     }
 
+    @Test
+    public void testPathTraversal3() throws Exception {
+        FactoryFinder<TransportFactory> finder
+                = new FactoryFinder<>(TRANSPORT_FACTORY_PATH, TransportFactory.class, null);
+        assertNull(finder.getAllowedImpls());
+
+        // test backslashes as well
+        try {
+            finder.newInstance("..\\..\\tcp");
+            fail("should have failed instantiation");
+        } catch (InstantiationException e) {
+            assertEquals("Provided key may not contain path separators",
+                    e.getMessage());
+        }
+    }
+
     // WireFormatFactory is not assignable to TransportFactory
     // So the constructor should throw an IllegalArgumentException
     @Test(expected = IllegalArgumentException.class)
@@ -169,6 +200,8 @@ public void testAllowedImplsFilterAllowAll() throws Exception {
         assertNull(finder.getAllowedImpls());
         assertNotNull(finder.newInstance("tcp"));
         assertNotNull(finder.newInstance("ssl"));
+        // should not contain backslashes, even on Windows
+        assertFalse(finder.resolvePath("tcp").contains("\\"));;
 
         try {
             // abc is allowed because we are not filtering by allowed impls but
@@ -199,4 +232,17 @@ public void testAllowedImplsTrimWhiteSpace() throws Exception {
         assertNotNull(finder.newInstance("ssl"));
         assertNotNull(finder.newInstance("nio"));
     }
+
+    @Test
+    public void testBackslashReplace() throws Exception {
+        // Forward slashes are usually used but this test uses backslashes to
+        // test that resolvePath() will replace any backslashes
+        String backslashPath = TRANSPORT_FACTORY_PATH.replace("/", "\\");
+        FactoryFinder<TransportFactory> finder = new FactoryFinder<>(backslashPath,
+                TransportFactory.class, null);
+        assertNotNull(finder.newInstance("tcp"));
+        // should not contain backslashes
+        assertFalse(finder.resolvePath("tcp").contains("\\"));
+    }
+
 }

activemq-stomp/src/test/java/org/apache/activemq/transport/stomp/ProtocolConverterTest.java

@@ -159,7 +159,14 @@ public void testPathTraversal() throws Exception {
             converter.loadTranslator("../stomp-test");
             fail("should have failed");
         } catch (InstantiationException e) {
-            assertTrue(e.getMessage().contains("rovided key escapes the FactoryFinder configured directory"));
+            assertTrue(e.getMessage().contains("Provided key may not contain path separators"));
+        }
+
+        try {
+            converter.loadTranslator("..");
+            fail("should have failed");
+        } catch (InstantiationException e) {
+            assertTrue(e.getMessage().contains("Provided key escapes the FactoryFinder configured directory"));
         }
 
         // fallback

activemq-web/src/test/java/org/apache/activemq/web/QueueBrowseServletTest.java

@@ -77,6 +77,22 @@ public void testPathTraversal() throws Exception {
         QueueBrowseServlet servlet = new QueueBrowseServlet();
         // illegal path traversal
         when(request.getParameter("view")).thenReturn("../../simple");
+        try {
+            servlet.getMessageRenderer(request);
+            fail("Should have thrown an exception");
+        } catch (NoSuchViewStyleException e) {
+            Throwable rootCause = e.getRootCause();
+            assertTrue(rootCause instanceof InstantiationException);
+            // view is in allow list but wrong interface type
+            assertEquals(rootCause.getMessage(), "Provided key may not contain path separators");
+        }
+    }
+
+    @Test
+    public void testPathTraversal2() throws Exception {
+        QueueBrowseServlet servlet = new QueueBrowseServlet();
+        // illegal path traversal
+        when(request.getParameter("view")).thenReturn("..");
         try {
             servlet.getMessageRenderer(request);
             fail("Should have thrown an exception");