Ensure a configuration is only written to its own file

config/core/src/main/java/org/apache/karaf/config/core/impl/ConfigRepositoryImpl.java

@@ -25,9 +25,11 @@
 import java.net.URISyntaxException;
 import java.net.URL;
 import java.nio.file.Files;
+import java.util.Arrays;
 import java.util.Dictionary;
 import java.util.Enumeration;
 import java.util.Hashtable;
+import java.util.List;
 import java.util.Map;
 
 import org.apache.felix.cm.json.io.Configurations;
@@ -84,7 +86,7 @@ public void update(String pid, Map<String, Object> properties, String suffix) th
                 if (file == null) {
                     file = generateConfigFilename(cfg, suffix);
                 }
-                checkConfigFileLocation(file);
+                checkConfigFile(cfg.getPid(), cfg.getFactoryPid(), file);
                 props.putAll(properties);
                 props.keySet().retainAll(properties.keySet());
                 store(props, file);
@@ -112,16 +114,67 @@ private static File generateConfigFilename(Configuration cfg, String suffix) {
     }
 
     /**
-     * Ensure that the configuration file we are about to write stays within the Karaf etc folder.
+     * Ensure that a configuration is only written to its own file.
      *
      * <p>The target file is derived from caller-controlled input: a {@code felix.fileinstall.filename}
-     * entry in the property map can point to an arbitrary path, and a PID (or factory alias) containing
-     * {@code ..} segments escapes {@code ${karaf.etc}} through {@link #generateConfigFilename}. Without
-     * this check a user allowed to update configurations could overwrite files reserved to more
-     * privileged users (e.g. {@code etc/users.properties} or the command/JMX ACL files).</p>
+     * entry in the property map, the storage type (suffix), the PID or the factory alias. The file has to
+     * be located inside {@code ${karaf.etc}}, use an extension the configuration installers load
+     * ({@code .cfg}, {@code .config} or the JSON one), and be named after the PID (or, for a factory
+     * configuration, after the factory PID), so that it can only hold this configuration.</p>
+     *
+     * <p>Staying inside {@code ${karaf.etc}} is not enough: the files reserved to more privileged users
+     * live there too. Without the name check, a user allowed to update an ordinary configuration could
+     * overwrite {@code etc/users.properties}, or the file of a PID they are not allowed to edit, such as
+     * the command and JMX ACLs.</p>
+     *
+     * @param pid the PID of the configuration, or {@code null} for a factory configuration not created yet.
+     * @param factoryPid the factory PID, or {@code null} if this is not a factory configuration.
+     * @param file the file the configuration is about to be written to.
      */
-    private static void checkConfigFileLocation(File file) throws IOException {
+    static void checkConfigFile(String pid, String factoryPid, File file) throws IOException {
         PathUtils.checkWithin(new File(System.getProperty("karaf.etc")), file);
+        // resolve symbolic links and ".." segments, and the actual case on case-insensitive file systems
+        String name = configName(file.getCanonicalFile().getName());
+        boolean ownFile = name != null
+                && (name.equals(pid)
+                    || (factoryPid != null && name.length() > factoryPid.length() + 1 && name.startsWith(factoryPid + "-")));
+        if (!ownFile) {
+            throw new IOException("For security reasons, configuration "
+                    + (pid != null ? pid : factoryPid) + " can't be stored in " + file
+                    + ": the file has to be named after the " + (pid != null ? "PID" : "factory PID")
+                    + " and use one of the " + configExtensions() + " extensions");
+        }
+    }
+
+    /**
+     * Ensure that a storage type (suffix) is one of the configuration file extensions.
+     */
+    private static void checkSuffix(String suffix) throws IOException {
+        if (!configExtensions().contains(suffix)) {
+            throw new IOException("Unsupported configuration type " + suffix
+                    + ", it has to be one of " + configExtensions());
+        }
+    }
+
+    /**
+     * The file extensions loaded as configurations: the JSON one first, as it can contain the others.
+     */
+    private static List<String> configExtensions() {
+        return Arrays.asList(JsonConfigInstaller.getExtension(), "cfg", "config");
+    }
+
+    /**
+     * Return the configuration name (PID, or factory PID and alias) of a file name, or {@code null}
+     * if the file is not a configuration file.
+     */
+    private static String configName(String fileName) {
+        for (String extension : configExtensions()) {
+            String ending = "." + extension;
+            if (fileName.endsWith(ending) && fileName.length() > ending.length()) {
+                return fileName.substring(0, fileName.length() - ending.length());
+            }
+        }
+        return null;
     }
 
     /* (non-Javadoc)
@@ -219,10 +272,11 @@ public String createFactoryConfiguration(String factoryPid, String alias, Map<St
 
     @Override
     public String createFactoryConfiguration(String factoryPid, String alias, Map<String, Object> properties, String suffix) throws IOException {
+        checkSuffix(suffix);
         File file = null;
         if (alias != null && !"".equals(alias.trim())) {
             file = new File(new File(System.getProperty("karaf.etc")), factoryPid + "-" + alias + "." + suffix);
-            checkConfigFileLocation(file);
+            checkConfigFile(null, factoryPid, file);
         }
         Configuration config = configAdmin.createFactoryConfiguration(factoryPid, "?");
         TypedProperties props = new TypedProperties();

config/core/src/main/java/org/apache/karaf/config/core/impl/JsonConfigInstaller.java

@@ -61,7 +61,7 @@ public boolean canHandle(File artifact) {
         return artifact.getName().endsWith("." + getExtension());
     }
 
-    private String getExtension() {
+    static String getExtension() {
         String extension = (System.getenv(EXT_ENV_VAR) != null) ? System.getenv(EXT_ENV_VAR) : null;
         extension = (System.getProperty(EXT_SYS_PROP) != null) ? System.getProperty(EXT_SYS_PROP) : extension;
         if (extension == null) {

config/core/src/test/java/org/apache/karaf/config/core/impl/ConfigRepositoryImplContainmentTest.java

@@ -13,20 +13,25 @@
  */
 package org.apache.karaf.config.core.impl;
 
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.easymock.EasyMock.anyObject;
 import static org.easymock.EasyMock.createMock;
 import static org.easymock.EasyMock.expect;
 import static org.easymock.EasyMock.expectLastCall;
 import static org.easymock.EasyMock.replay;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeTrue;
 
 import java.io.File;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Hashtable;
+import java.util.stream.Stream;
 
 import org.apache.felix.utils.properties.TypedProperties;
 import org.junit.After;
@@ -36,12 +41,16 @@
 import org.osgi.service.cm.ConfigurationAdmin;
 
 /**
- * Verifies that {@link ConfigRepositoryImpl} refuses to persist a configuration outside of
- * {@code ${karaf.etc}}, whether the escape is attempted through a crafted PID or through a
- * caller-supplied {@code felix.fileinstall.filename} property.
+ * Verifies that {@link ConfigRepositoryImpl} only persists a configuration to its own file: inside
+ * {@code ${karaf.etc}}, with a configuration extension, and named after the PID. The target is
+ * attempted through a crafted PID, factory alias, storage type (suffix) or caller-supplied
+ * {@code felix.fileinstall.filename} property.
  */
 public class ConfigRepositoryImplContainmentTest {
 
+    private static final String USERS = "karaf = karaf,_g_:admingroup\n_g_\\:admingroup = group,admin,manager,viewer\n";
+    private static final String JMX_ACL = "update(java.lang.String,java.util.Map)[/jmx[.]acl.*/,/.*/] = admin\n";
+
     private Path etc;
     private Path outside;
     private String previousEtc;
@@ -134,4 +143,113 @@ public void regularUpdateStillWritesInsideEtc() throws Exception {
         repository(pid).update(pid, properties);
         assertTrue("the cfg file must land in etc", Files.exists(etc.resolve("my.regular.pid.cfg")));
     }
+
+    @Test
+    public void fileinstallFilenameToReservedEtcFileIsRejected() throws Exception {
+        Path users = Files.write(etc.resolve("users.properties"), USERS.getBytes(UTF_8));
+        String pid = "my.new.pid";
+        TypedProperties properties = new TypedProperties();
+        properties.put("felix.fileinstall.filename", users.toUri().toString());
+        properties.put("intruder", "intruder,_g_:admingroup");
+        ConfigRepositoryImpl repository = repository(pid);
+        assertThrows(IOException.class, () -> repository.update(pid, properties));
+        assertEquals("users.properties must be left untouched", USERS, new String(Files.readAllBytes(users), UTF_8));
+    }
+
+    @Test
+    public void fileinstallFilenameToAnotherPidFileIsRejected() throws Exception {
+        Path acl = Files.write(etc.resolve("jmx.acl.org.apache.karaf.config.cfg"), JMX_ACL.getBytes(UTF_8));
+        String pid = "my.new.pid";
+        TypedProperties properties = new TypedProperties();
+        properties.put("felix.fileinstall.filename", acl.toUri().toString());
+        properties.put("update(java.lang.String,java.util.Map)", "manager");
+        ConfigRepositoryImpl repository = repository(pid);
+        assertThrows(IOException.class, () -> repository.update(pid, properties));
+        assertEquals("the ACL file must be left untouched", JMX_ACL, new String(Files.readAllBytes(acl), UTF_8));
+    }
+
+    @Test
+    public void suffixNamingReservedEtcFileIsRejected() throws Exception {
+        Path users = Files.write(etc.resolve("users.properties"), USERS.getBytes(UTF_8));
+        String pid = "users";
+        TypedProperties properties = new TypedProperties();
+        properties.put("intruder", "intruder,_g_:admingroup");
+        ConfigRepositoryImpl repository = repository(pid);
+        assertThrows(IOException.class, () -> repository.update(pid, properties, "properties"));
+        assertEquals("users.properties must be left untouched", USERS, new String(Files.readAllBytes(users), UTF_8));
+    }
+
+    @Test
+    public void suffixNamingAnotherPidFileIsRejected() throws Exception {
+        Path acl = Files.write(etc.resolve("jmx.acl.org.apache.karaf.config.cfg"), JMX_ACL.getBytes(UTF_8));
+        String pid = "jmx";
+        TypedProperties properties = new TypedProperties();
+        properties.put("update(java.lang.String,java.util.Map)", "manager");
+        ConfigRepositoryImpl repository = repository(pid);
+        assertThrows(IOException.class, () -> repository.update(pid, properties, "acl.org.apache.karaf.config.cfg"));
+        assertEquals("the ACL file must be left untouched", JMX_ACL, new String(Files.readAllBytes(acl), UTF_8));
+    }
+
+    @Test
+    public void pidDifferingOnlyByCaseIsRejectedOnCaseInsensitiveFileSystems() throws Exception {
+        Path acl = Files.write(etc.resolve("jmx.acl.org.apache.karaf.config.cfg"), JMX_ACL.getBytes(UTF_8));
+        assumeTrue("needs a case-insensitive file system", Files.exists(etc.resolve("JMX.ACL.org.apache.karaf.config.cfg")));
+        // the ACL patterns are case sensitive, so this PID is not protected by them
+        String pid = "JMX.ACL.org.apache.karaf.config";
+        TypedProperties properties = new TypedProperties();
+        properties.put("update(java.lang.String,java.util.Map)", "manager");
+        ConfigRepositoryImpl repository = repository(pid);
+        assertThrows(IOException.class, () -> repository.update(pid, properties));
+        assertEquals("the ACL file must be left untouched", JMX_ACL, new String(Files.readAllBytes(acl), UTF_8));
+    }
+
+    @Test
+    public void factoryUnsupportedSuffixIsRejected() throws Exception {
+        ConfigurationAdmin admin = createMock(ConfigurationAdmin.class);
+        replay(admin);
+        ConfigRepositoryImpl repository = new ConfigRepositoryImpl(admin);
+        assertThrows(IOException.class,
+                () -> repository.createFactoryConfiguration("my.factory", null, new TypedProperties(), "properties"));
+        assertThrows(IOException.class,
+                () -> repository.createFactoryConfiguration("my.factory", "alias", new TypedProperties(), "properties"));
+        try (Stream<Path> files = Files.list(etc)) {
+            assertEquals("no file must be written", 0, files.count());
+        }
+    }
+
+    @Test
+    public void fileinstallFilenameOfThePidIsHonored() throws Exception {
+        String pid = "my.json.pid";
+        Path json = etc.resolve(pid + ".cfg.json");
+        TypedProperties properties = new TypedProperties();
+        properties.put("felix.fileinstall.filename", json.toUri().toString());
+        properties.put("hello", "world");
+        repository(pid).update(pid, properties);
+        assertTrue("the configuration must be written to the provided file", Files.exists(json));
+        assertFalse(Files.exists(etc.resolve(pid + ".cfg")));
+    }
+
+    @Test
+    public void jsonSuffixStillWritesInsideEtc() throws Exception {
+        String pid = "my.json.pid";
+        TypedProperties properties = new TypedProperties();
+        properties.put("hello", "world");
+        repository(pid).update(pid, properties, "cfg.json");
+        assertTrue("the json file must land in etc", Files.exists(etc.resolve("my.json.pid.cfg.json")));
+    }
+
+    @Test
+    public void factoryAliasStillWritesInsideEtc() throws Exception {
+        ConfigurationAdmin admin = createMock(ConfigurationAdmin.class);
+        Configuration config = createMock(Configuration.class);
+        expect(admin.createFactoryConfiguration("my.factory", "?")).andReturn(config).anyTimes();
+        expect(config.getPid()).andReturn("my.factory.generated").anyTimes();
+        config.update(anyObject());
+        expectLastCall().anyTimes();
+        replay(admin, config);
+        TypedProperties properties = new TypedProperties();
+        properties.put("hello", "world");
+        new ConfigRepositoryImpl(admin).createFactoryConfiguration("my.factory", "alias", properties);
+        assertTrue("the cfg file must land in etc", Files.exists(etc.resolve("my.factory-alias.cfg")));
+    }
 }