This is an automated email from the ASF dual-hosted git repository.
jbonofre pushed a commit to branch karaf-4.4.x
in repository https://gitbox.apache.org/repos/asf/karaf.git
The following commit(s) were added to refs/heads/karaf-4.4.x by this push:
new 024e4cbf3d Ensure configuration files are always written within
${karaf.etc} (#2868)
024e4cbf3d is described below
commit 024e4cbf3d7b946e79d18792788ed68347982b02
Author: JB Onofré <[email protected]>
AuthorDate: Sat Sep 12 07:55:16 2026 +0200
Ensure configuration files are always written within ${karaf.etc} (#2868)
ConfigRepositoryImpl derived the target file from caller-supplied input
(the felix.fileinstall.filename property and the PID / factory alias used
to build the file name) without checking that the result stayed inside
${karaf.etc}. Add a canonical-path containment check, shared through a new
org.apache.karaf.util.PathUtils helper, and reuse it in
ConfigMBeanImpl.install() and the config:install command in place of the
weaker finalname.contains("..") string check.
---
.../karaf/config/command/InstallCommand.java | 5 +-
.../karaf/config/core/impl/ConfigMBeanImpl.java | 5 +-
.../config/core/impl/ConfigRepositoryImpl.java | 23 +++-
.../config/core/impl/ConfigMBeanImplTest.java | 4 +-
.../impl/ConfigRepositoryImplContainmentTest.java | 137 +++++++++++++++++++++
.../main/java/org/apache/karaf/util/PathUtils.java | 50 ++++++++
.../java/org/apache/karaf/util/PathUtilsTest.java | 67 ++++++++++
7 files changed, 281 insertions(+), 10 deletions(-)
diff --git
a/config/command/src/main/java/org/apache/karaf/config/command/InstallCommand.java
b/config/command/src/main/java/org/apache/karaf/config/command/InstallCommand.java
index ea55226765..fad01ed2fb 100644
---
a/config/command/src/main/java/org/apache/karaf/config/command/InstallCommand.java
+++
b/config/command/src/main/java/org/apache/karaf/config/command/InstallCommand.java
@@ -21,6 +21,7 @@ import org.apache.karaf.shell.api.action.Argument;
import org.apache.karaf.shell.api.action.Command;
import org.apache.karaf.shell.api.action.Option;
import org.apache.karaf.shell.api.action.lifecycle.Service;
+import org.apache.karaf.util.PathUtils;
import org.apache.karaf.util.StreamUtils;
import java.io.BufferedInputStream;
@@ -45,11 +46,9 @@ public class InstallCommand implements Action {
@Override
public Object execute() throws Exception {
- if (finalname.contains("..")) {
- throw new IllegalArgumentException("For security reason, relative
path is not allowed in config file final name");
- }
File etcFolder = new File(System.getProperty("karaf.etc"));
File file = new File(etcFolder, finalname);
+ PathUtils.checkWithin(etcFolder, file);
if (file.exists()) {
if (!override) {
throw new IllegalArgumentException("Configuration file {}
already exists " + finalname);
diff --git
a/config/core/src/main/java/org/apache/karaf/config/core/impl/ConfigMBeanImpl.java
b/config/core/src/main/java/org/apache/karaf/config/core/impl/ConfigMBeanImpl.java
index 81e1fed4c9..889cf7e0e6 100644
---
a/config/core/src/main/java/org/apache/karaf/config/core/impl/ConfigMBeanImpl.java
+++
b/config/core/src/main/java/org/apache/karaf/config/core/impl/ConfigMBeanImpl.java
@@ -31,6 +31,7 @@ import javax.management.StandardMBean;
import org.apache.felix.utils.properties.TypedProperties;
import org.apache.karaf.config.core.ConfigMBean;
import org.apache.karaf.config.core.ConfigRepository;
+import org.apache.karaf.util.PathUtils;
import org.apache.karaf.util.StreamUtils;
import org.osgi.framework.InvalidSyntaxException;
import org.osgi.service.cm.Configuration;
@@ -84,12 +85,10 @@ public class ConfigMBeanImpl extends StandardMBean
implements ConfigMBean {
@Override
public void install(String url, String finalname, boolean override) throws
MBeanException {
- if (finalname.contains("..")) {
- throw new IllegalArgumentException("For security reason, relative
path is not allowed in config file final name");
- }
try {
File etcFolder = new File(System.getProperty("karaf.etc"));
File file = new File(etcFolder, finalname);
+ PathUtils.checkWithin(etcFolder, file);
if (file.exists()) {
if (!override) {
throw new IllegalArgumentException("Configuration file {}
already exists " + finalname);
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 3f057863e3..470575377e 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
@@ -33,6 +33,7 @@ import java.util.Map;
import org.apache.felix.cm.json.Configurations;
import org.apache.felix.utils.properties.TypedProperties;
import org.apache.karaf.config.core.ConfigRepository;
+import org.apache.karaf.util.PathUtils;
import org.osgi.framework.Constants;
import org.osgi.framework.InvalidSyntaxException;
import org.osgi.service.cm.Configuration;
@@ -83,6 +84,7 @@ public class ConfigRepositoryImpl implements ConfigRepository
{
if (file == null) {
file = generateConfigFilename(cfg, suffix);
}
+ checkConfigFileLocation(file);
props.putAll(properties);
props.keySet().retainAll(properties.keySet());
store(props, file);
@@ -109,6 +111,19 @@ public class ConfigRepositoryImpl implements
ConfigRepository {
return new File(System.getProperty("karaf.etc"), fName);
}
+ /**
+ * Ensure that the configuration file we are about to write stays within
the Karaf etc folder.
+ *
+ * <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>
+ */
+ private static void checkConfigFileLocation(File file) throws IOException {
+ PathUtils.checkWithin(new File(System.getProperty("karaf.etc")), file);
+ }
+
/* (non-Javadoc)
* @see
org.apache.karaf.shell.config.impl.ConfigRepository#delete(java.lang.String)
*/
@@ -204,12 +219,14 @@ public class ConfigRepositoryImpl implements
ConfigRepository {
@Override
public String createFactoryConfiguration(String factoryPid, String alias,
Map<String, Object> properties, String suffix) throws IOException {
- Configuration config =
configAdmin.createFactoryConfiguration(factoryPid, "?");
- TypedProperties props = new TypedProperties();
File file = null;
if (alias != null && !"".equals(alias.trim())) {
file = new File(new File(System.getProperty("karaf.etc")),
factoryPid + "-" + alias + "." + suffix);
- } else {
+ checkConfigFileLocation(file);
+ }
+ Configuration config =
configAdmin.createFactoryConfiguration(factoryPid, "?");
+ TypedProperties props = new TypedProperties();
+ if (file == null) {
file = Files.createTempFile(new
File(System.getProperty("karaf.etc")).toPath(), factoryPid + "-", "." +
suffix).toFile();
}
props.putAll(properties);
diff --git
a/config/core/src/test/java/org/apache/karaf/config/core/impl/ConfigMBeanImplTest.java
b/config/core/src/test/java/org/apache/karaf/config/core/impl/ConfigMBeanImplTest.java
index 4bdf08abf9..908f30594d 100644
---
a/config/core/src/test/java/org/apache/karaf/config/core/impl/ConfigMBeanImplTest.java
+++
b/config/core/src/test/java/org/apache/karaf/config/core/impl/ConfigMBeanImplTest.java
@@ -16,13 +16,15 @@ package org.apache.karaf.config.core.impl;
import org.junit.Assert;
import org.junit.Test;
+import javax.management.MBeanException;
+
import java.io.BufferedReader;
import java.io.File;
import java.io.FileReader;
public class ConfigMBeanImplTest {
- @Test(expected = IllegalArgumentException.class)
+ @Test(expected = MBeanException.class)
public void testInstallWithNonAuthorizePath() throws Exception {
System.setProperty("karaf.etc", ".");
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
new file mode 100644
index 0000000000..a1f0f7f586
--- /dev/null
+++
b/config/core/src/test/java/org/apache/karaf/config/core/impl/ConfigRepositoryImplContainmentTest.java
@@ -0,0 +1,137 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.karaf.config.core.impl;
+
+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.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Hashtable;
+
+import org.apache.felix.utils.properties.TypedProperties;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.osgi.service.cm.Configuration;
+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.
+ */
+public class ConfigRepositoryImplContainmentTest {
+
+ private Path etc;
+ private Path outside;
+ private String previousEtc;
+
+ @Before
+ public void setUp() throws Exception {
+ Path base = Files.createTempDirectory("karaf-config-containment");
+ etc = Files.createDirectories(base.resolve("etc"));
+ outside = Files.createDirectories(base.resolve("outside"));
+ previousEtc = System.getProperty("karaf.etc");
+ System.setProperty("karaf.etc", etc.toFile().getCanonicalPath());
+ }
+
+ @After
+ public void tearDown() {
+ if (previousEtc == null) {
+ System.clearProperty("karaf.etc");
+ } else {
+ System.setProperty("karaf.etc", previousEtc);
+ }
+ }
+
+ private ConfigRepositoryImpl repository(String pid) throws Exception {
+ ConfigurationAdmin admin = createMock(ConfigurationAdmin.class);
+ Configuration config = createMock(Configuration.class);
+ expect(admin.getConfiguration(pid, "?")).andReturn(config).anyTimes();
+ expect(config.getProcessedProperties(anyObject())).andReturn(new
Hashtable<>()).anyTimes();
+ expect(config.getPid()).andReturn(pid).anyTimes();
+ expect(config.getFactoryPid()).andReturn(null).anyTimes();
+ config.update(anyObject());
+ expectLastCall().anyTimes();
+ replay(admin, config);
+ return new ConfigRepositoryImpl(admin);
+ }
+
+ @Test
+ public void traversalPidIsRejected() throws Exception {
+ String pid = "../outside/evil";
+ try {
+ repository(pid).update(pid, new TypedProperties());
+ fail("expected the traversal PID to be rejected");
+ } catch (IOException e) {
+ // expected
+ }
+ assertFalse("no file must be written outside etc",
Files.exists(outside.resolve("evil.cfg")));
+ }
+
+ @Test
+ public void fileinstallFilenameOutsideEtcIsRejected() throws Exception {
+ String pid = "my.legit.pid";
+ File escape = outside.resolve("evil.cfg").toFile();
+ TypedProperties properties = new TypedProperties();
+ properties.put("felix.fileinstall.filename",
escape.toURI().toString());
+ try {
+ repository(pid).update(pid, properties);
+ fail("expected the out-of-etc felix.fileinstall.filename to be
rejected");
+ } catch (IOException e) {
+ // expected
+ }
+ assertFalse("no file must be written outside etc", escape.exists());
+ }
+
+ @Test
+ public void factoryAliasTraversalIsRejected() 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);
+
+ // the alias is concatenated as "<factoryPid>-<alias>.<suffix>", so an
escaping payload
+ // needs a leading path segment before the ".." sequence
+ try {
+ new ConfigRepositoryImpl(admin)
+ .createFactoryConfiguration("my.factory",
"x/../../outside/evil", new TypedProperties());
+ fail("expected the traversal alias to be rejected");
+ } catch (IOException e) {
+ // expected
+ }
+ assertFalse(Files.exists(outside.resolve("evil.cfg")));
+ }
+
+ @Test
+ public void regularUpdateStillWritesInsideEtc() throws Exception {
+ String pid = "my.regular.pid";
+ TypedProperties properties = new TypedProperties();
+ properties.put("hello", "world");
+ repository(pid).update(pid, properties);
+ assertTrue("the cfg file must land in etc",
Files.exists(etc.resolve("my.regular.pid.cfg")));
+ }
+}
diff --git a/util/src/main/java/org/apache/karaf/util/PathUtils.java
b/util/src/main/java/org/apache/karaf/util/PathUtils.java
new file mode 100644
index 0000000000..76e2b85eeb
--- /dev/null
+++ b/util/src/main/java/org/apache/karaf/util/PathUtils.java
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.karaf.util;
+
+import java.io.File;
+import java.io.IOException;
+
+public final class PathUtils {
+
+ private PathUtils() {
+ }
+
+ /**
+ * Check whether {@code child} resolves to a location inside {@code
parent}, following symbolic
+ * links and normalizing {@code ..} segments through canonical paths.
+ */
+ public static boolean isWithin(File parent, File child) throws IOException
{
+ return
child.getCanonicalFile().toPath().startsWith(parent.getCanonicalFile().toPath());
+ }
+
+ /**
+ * Ensure that {@code child} resolves to a location inside {@code parent},
throwing an
+ * {@link IOException} otherwise.
+ *
+ * <p>Use this to stop a caller-controlled file name or path from escaping
the directory it is
+ * meant to stay within (path traversal, CWE-22) -- a plain {@code
contains("..")} check does
+ * not catch absolute paths or symbolic links.</p>
+ */
+ public static void checkWithin(File parent, File child) throws IOException
{
+ if (!isWithin(parent, child)) {
+ throw new IOException("For security reasons, " + child + " has to
be located inside "
+ + parent.getCanonicalFile());
+ }
+ }
+
+}
diff --git a/util/src/test/java/org/apache/karaf/util/PathUtilsTest.java
b/util/src/test/java/org/apache/karaf/util/PathUtilsTest.java
new file mode 100644
index 0000000000..6c3e8f1718
--- /dev/null
+++ b/util/src/test/java/org/apache/karaf/util/PathUtilsTest.java
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.karaf.util;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import org.junit.Test;
+
+public class PathUtilsTest {
+
+ @Test
+ public void childInsideParentIsAccepted() throws Exception {
+ Path parent = Files.createTempDirectory("pathutils");
+ File child = new File(parent.toFile(), "sub/child.cfg");
+ assertTrue(PathUtils.isWithin(parent.toFile(), child));
+ PathUtils.checkWithin(parent.toFile(), child);
+ }
+
+ @Test
+ public void relativeTraversalIsRejected() throws Exception {
+ Path parent = Files.createTempDirectory("pathutils");
+ File child = new File(parent.toFile(), "../escape.cfg");
+ assertFalse(PathUtils.isWithin(parent.toFile(), child));
+ try {
+ PathUtils.checkWithin(parent.toFile(), child);
+ fail("expected traversal to be rejected");
+ } catch (IOException e) {
+ // expected
+ }
+ }
+
+ @Test
+ public void absolutePathOutsideParentIsRejected() throws Exception {
+ Path parent = Files.createTempDirectory("pathutils");
+ File child = new File(System.getProperty("java.io.tmpdir"),
"definitely-not-under-parent.cfg");
+ assertFalse(PathUtils.isWithin(parent.toFile(), child));
+ }
+
+ @Test
+ public void siblingWithSharedPrefixIsRejected() throws Exception {
+ Path base = Files.createTempDirectory("pathutils");
+ File parent = new File(base.toFile(), "etc");
+ File child = new File(base.toFile(), "etc-evil/child.cfg");
+ assertFalse(PathUtils.isWithin(parent, child));
+ }
+}
