Restrict URL protocol types loaded by XBeanBrokerFactory (#1910)

This adds a new system property to control which protocol types are
valid for loading resources using the XBeanBrokerFactory. By default
only file and classpath resources can be loaded.

The goal of this is to prevent possible future security issues by
hardening what is allowed to be loaded by default.

Author: cshannon

activemq-spring/src/main/java/org/apache/activemq/spring/Utils.java

@@ -19,6 +19,9 @@
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.net.MalformedURLException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.Set;
 import org.springframework.core.io.ClassPathResource;
 import org.springframework.core.io.FileSystemResource;
 import org.springframework.core.io.Resource;
@@ -27,22 +30,69 @@
 
 public class Utils {
 
+    public static final String FILE_PROTOCOL = "file";
+    public static final String CLASSPATH_PROTOCOL = "classpath";
+
     public static Resource resourceFromString(String uri) throws MalformedURLException {
-        Resource resource;
-        File file = new File(uri);
-        if (file.exists()) {
+        // default allows all
+        return resourceFromString(uri, null);
+    }
+
+    public static Resource resourceFromString(String uri, Set<String> allowedProtocols) throws MalformedURLException {
+        // Empty set means nothing is allowed
+        if (allowedProtocols != null && allowedProtocols.isEmpty()) {
+            throw new IllegalArgumentException("No protocols are allowed for loading resources.");
+        }
+
+        final Resource resource;
+
+        // First, just try and load a local file (if it exists) and if "file"
+        // as part of the allow list. This preserves previous behavior of
+        // always optimistically trying a local file first.
+        if (isAllowFile(allowedProtocols) && new File(uri).exists()) {
             resource = new FileSystemResource(uri);
+        // If file isn't allowed, or if the file can't be found then check
+        // if the string is a valid URL. If it's valid, then we need
+        // to validate if it's allowed before loading the URL.
+        // isUrl() uses URI internally so it won't actually load anything
         } else if (ResourceUtils.isUrl(uri)) {
             try {
+                validateUrlAllowed(uri, allowedProtocols);
                 resource = new UrlResource(ResourceUtils.getURL(uri));
-            } catch (FileNotFoundException e) {
+            } catch (FileNotFoundException | URISyntaxException e) {
                 MalformedURLException malformedURLException = new MalformedURLException(uri);
                 malformedURLException.initCause(e);
-                throw  malformedURLException;
+                throw malformedURLException;
             }
-        } else {
+        // Fallback to trying on the classpath if not a valid Url, and we allow it which
+        // also preserves the previous behavior (if classpath is allowed)
+        } else if (isAllowClasspath(allowedProtocols)){
             resource = new ClassPathResource(uri);
+        // Catch all fail-safe if nothing else matches. This could happen if file is allowed
+        // but not classpath but the file doesn't exist
+        } else {
+            throw new IllegalArgumentException("URL [" + uri + "] can't be found or is not allowed"
+                    + " for loading resources");
         }
         return resource;
     }
+
+    static boolean isAllowFile(Set<String> allowedProtocols) {
+        return allowedProtocols == null || allowedProtocols.contains(FILE_PROTOCOL);
+    }
+
+    static boolean isAllowClasspath(Set<String> allowedProtocols) {
+        return allowedProtocols == null || allowedProtocols.contains(CLASSPATH_PROTOCOL);
+    }
+
+    private static void validateUrlAllowed(String uriString, Set<String> allowedProtocols)
+            throws URISyntaxException {
+        // Use new URI() to get the scheme
+        // This is important because ResourceUtils.getURL() actually searches
+        // the classpath which we don't want to do if not allowed
+        if (allowedProtocols != null && !allowedProtocols.contains(new URI(uriString).getScheme())) {
+            throw new IllegalArgumentException("URL [" + uriString +
+                    "] does not use an allowed protocol for loading URL resources");
+        }
+    }
 }

activemq-spring/src/main/java/org/apache/activemq/xbean/XBeanBrokerFactory.java

@@ -20,7 +20,9 @@
 import java.net.MalformedURLException;
 import java.net.URI;
 
-import org.apache.activemq.broker.BrokerContextAware;
+import java.util.Arrays;
+import java.util.Set;
+import java.util.stream.Collectors;
 import org.apache.activemq.broker.BrokerFactoryHandler;
 import org.apache.activemq.broker.BrokerService;
 import org.apache.activemq.spring.SpringBrokerContext;
@@ -35,20 +37,38 @@
 import org.springframework.beans.FatalBeanException;
 import org.springframework.beans.factory.xml.XmlBeanDefinitionReader;
 import org.springframework.context.ApplicationContext;
-import org.springframework.context.ApplicationContextAware;
 import org.springframework.core.io.Resource;
 
 /**
  * 
  */
 public class XBeanBrokerFactory implements BrokerFactoryHandler {
-    private static final transient Logger LOG = LoggerFactory.getLogger(XBeanBrokerFactory.class);
+    private static final Logger LOG = LoggerFactory.getLogger(XBeanBrokerFactory.class);
+
+    public static final String XBEAN_BROKER_FACTORY_PROTOCOLS_PROP =
+            "org.apache.activemq.xbean.XBEAN_BROKER_FACTORY_PROTOCOLS";
+    public static final String DEFAULT_ALLOWED_PROTOCOLS = Utils.FILE_PROTOCOL + "," + Utils.CLASSPATH_PROTOCOL;
+
+    private final Set<String> allowedProtocols;
 
     static {
         PropertyEditorManager.registerEditor(URI.class, URIEditor.class);
     }
 
+    public XBeanBrokerFactory() {
+        final String allowedProtocols = System.getProperty(XBEAN_BROKER_FACTORY_PROTOCOLS_PROP,
+                DEFAULT_ALLOWED_PROTOCOLS);
+
+        // Asterisk will map to null which will allow all and skip checking
+        // Empty string will map to an empty set and will deny all
+        this.allowedProtocols = !allowedProtocols.equals("*") ?
+                Arrays.stream(allowedProtocols.split("\\s*,\\s*"))
+                .filter(s -> !s.isBlank())
+                .collect(Collectors.toUnmodifiableSet()) : null;
+    }
+
     private boolean validate = true;
+
     public boolean isValidate() {
         return validate;
     }
@@ -75,12 +95,10 @@ public BrokerService createBroker(URI config) throws Exception {
         if (broker == null) {
             // lets try find by type
             String[] names = context.getBeanNamesForType(BrokerService.class);
-            for (int i = 0; i < names.length; i++) {
-                String name = names[i];
-                broker = (BrokerService)context.getBean(name);
-                if (broker != null) {
-                    break;
-                }
+            for (String name : names) {
+                // No need to check for null, this will throw an exception if not found
+                broker = (BrokerService) context.getBean(name);
+                break;
             }
         }
         if (broker == null) {
@@ -98,8 +116,8 @@ public BrokerService createBroker(URI config) throws Exception {
     }
 
     protected ApplicationContext createApplicationContext(String uri) throws MalformedURLException {
-        Resource resource = Utils.resourceFromString(uri);
-        LOG.debug("Using " + resource + " from " + uri);
+        Resource resource = Utils.resourceFromString(uri, allowedProtocols);
+        LOG.debug("Using {} from {}", resource, uri);
         try {
             return new ResourceXmlApplicationContext(resource) {
                 @Override
@@ -108,9 +126,14 @@ protected void initBeanDefinitionReader(XmlBeanDefinitionReader reader) {
                 }
             };
         } catch (FatalBeanException errorToLog) {
-            LOG.error("Failed to load: " + resource + ", reason: " + errorToLog.getLocalizedMessage(), errorToLog);
+            LOG.error("Failed to load: {}, reason: {}", resource, errorToLog.getLocalizedMessage(),
+                    errorToLog);
             throw errorToLog;
         }
     }
 
+    // Package scope for testing
+    Set<String> getAllowedProtocols() {
+        return allowedProtocols;
+    }
 }