diff --git a/config/core/src/main/java/org/apache/karaf/config/core/impl/ConfigRepositoryImpl.java b/config/core/src/main/java/org/apache/karaf/config/core/impl/ConfigRepositoryImpl.java index 4f239e607ac..bc73bdecfff 100644 --- a/config/core/src/main/java/org/apache/karaf/config/core/impl/ConfigRepositoryImpl.java +++ b/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 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. * *

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).

+ * 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.

+ * + *

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.

+ * + * @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 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 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(); diff --git a/config/core/src/main/java/org/apache/karaf/config/core/impl/JsonConfigInstaller.java b/config/core/src/main/java/org/apache/karaf/config/core/impl/JsonConfigInstaller.java index 1c82595b0a3..d6865843a8d 100644 --- a/config/core/src/main/java/org/apache/karaf/config/core/impl/JsonConfigInstaller.java +++ b/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) { diff --git a/config/core/src/test/java/org/apache/karaf/config/core/impl/ConfigRepositoryImplContainmentTest.java b/config/core/src/test/java/org/apache/karaf/config/core/impl/ConfigRepositoryImplContainmentTest.java index a1f0f7f5862..02286edaf61 100644 --- a/config/core/src/test/java/org/apache/karaf/config/core/impl/ConfigRepositoryImplContainmentTest.java +++ b/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 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"))); + } }