gnodet-bot commented on code in PR #26782:
URL: https://github.com/apache/camel/pull/26782#discussion_r4091003339
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/lifecycle/FileBasedKeyLifecycleManager.java:
##########
@@ -442,19 +442,44 @@ private static void restrictToOwner(Path path) {
}
private Path getPrivateKeyFile(String keyId) {
- return keyDirectory.resolve(keyId + ".private.json");
+ return resolveKeyFile(keyId, ".private.json");
}
private Path getPublicKeyFile(String keyId) {
- return keyDirectory.resolve(keyId + ".public.json");
+ return resolveKeyFile(keyId, ".public.json");
}
private Path getMetadataFile(String keyId) {
- return keyDirectory.resolve(keyId + ".metadata");
+ return resolveKeyFile(keyId, ".metadata");
}
private Path getLegacyKeyFile(String keyId) {
- return keyDirectory.resolve(keyId + ".key");
+ return resolveKeyFile(keyId, ".key");
+ }
+
+ /**
+ * Resolves a key file inside the configured key directory, confining it
to that directory. keyId values arrive from
+ * message headers (CamelPQCKeyId / CamelPQCNewKeyId), so a value
containing path separators, parent references or
+ * an absolute path could otherwise resolve to a location outside the key
directory. keyId is therefore constrained
+ * to a single flat file-name segment, and the resolved path is checked to
lie textually under the key directory
+ * (which is stored absolute and normalized in the constructor).
+ * <p/>
+ * The {@code startsWith} check is textual and does not follow symbolic
links, so a symlink placed inside the key
+ * directory could still point elsewhere. Creating such a link requires
write access to the operator-controlled key
+ * directory, which is outside the header-supplied keyId threat this
guards against.
+ */
+ private Path resolveKeyFile(String keyId, String suffix) {
+ if (keyId == null || keyId.isBlank()) {
+ throw new IllegalArgumentException("keyId must not be null or
empty");
+ }
+ if (keyId.indexOf('/') >= 0 || keyId.indexOf('\\') >= 0 ||
keyId.indexOf('\0') >= 0) {
+ throw new IllegalArgumentException("keyId must not contain path
separators: '" + keyId + "'");
Review Comment:
⚠️ **Medium — raw `keyId` echoed in exception message may reach API
responses**
The exception message embeds the untrusted `keyId` verbatim: `"keyId must
not contain path separators: '" + keyId + "'"`. If this
`IllegalArgumentException` is propagated to a Camel Exchange error body or an
HTTP response (e.g. via a Camel REST endpoint fronting the PQC component), an
attacker can probe the server by observing whether their crafted `keyId`
appears in the response. Truncate or sanitise the value in the message:
```suggestion
throw new IllegalArgumentException("keyId must not contain path
separators (length: " + keyId.length() + ")");
```
Same applies to the `startsWith` guard on line 480.
##########
components/camel-pqc/src/test/java/org/apache/camel/component/pqc/lifecycle/FileBasedKeyLifecycleManagerPathTest.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.camel.component.pqc.lifecycle;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * keyId values reach the file-based key store from message headers
(CamelPQCKeyId / CamelPQCNewKeyId). The manager must
+ * keep every key file inside its configured directory regardless of the
supplied keyId.
+ */
+class FileBasedKeyLifecycleManagerPathTest {
+
+ @Test
+ void rejectsKeyIdWithParentTraversal(@TempDir Path tempDir) throws
Exception {
+ Path keyDir = tempDir.resolve("keys");
+ FileBasedKeyLifecycleManager manager = new
FileBasedKeyLifecycleManager(keyDir.toString());
+
+ // A sentinel file one level above the key directory that an
unconfined "../victim" keyId would target
+ Path sentinel = tempDir.resolve("victim.private.json");
+ Files.writeString(sentinel, "{}");
+
+ assertThrows(IllegalArgumentException.class, () ->
manager.getKey("../victim"));
+ assertThrows(IllegalArgumentException.class, () ->
manager.deleteKey("../victim"));
+
+ assertTrue(Files.exists(sentinel), "a file outside the key directory
must not be reachable via keyId");
+ }
+
+ @Test
+ void rejectsAbsoluteSeparatorAndBlankKeyIds(@TempDir Path tempDir) throws
Exception {
+ Path keyDir = tempDir.resolve("keys");
+ FileBasedKeyLifecycleManager manager = new
FileBasedKeyLifecycleManager(keyDir.toString());
+
+ assertThrows(IllegalArgumentException.class, () ->
manager.getKeyMetadata("sub/evil"));
+ assertThrows(IllegalArgumentException.class, () ->
manager.getKeyMetadata("/etc/evil"));
+ assertThrows(IllegalArgumentException.class,
+ () ->
manager.getKeyMetadata(tempDir.resolve("abs").toString()));
+ assertThrows(IllegalArgumentException.class, () ->
manager.getKeyMetadata(""));
Review Comment:
⚠️ **Medium — `startsWith` fallback is untested; all rejection cases hit the
character check first**
Every `assertThrows` in this test targets a `keyId` that contains `/` or is
blank, so it is rejected by the character-check guard before the `startsWith`
line is ever evaluated. A NUL character (`\0`) is also guarded but not tested.
The `startsWith` second line of defence — the only thing standing between a
bypass of the character check (e.g. a future refactor removing the slash guard)
and an actual path escape — has zero test coverage.
Add two cases:
```suggestion
assertThrows(IllegalArgumentException.class, () ->
manager.getKeyMetadata("sub/evil"));
assertThrows(IllegalArgumentException.class, () ->
manager.getKeyMetadata("/etc/evil"));
assertThrows(IllegalArgumentException.class,
() ->
manager.getKeyMetadata(tempDir.resolve("abs").toString()));
assertThrows(IllegalArgumentException.class, () ->
manager.getKeyMetadata(""));
// NUL character — caught by character guard
assertThrows(IllegalArgumentException.class, () ->
manager.getKeyMetadata("evil\0inject"));
```
And add a separate test that exercises the `startsWith` guard directly
(construct a `keyId+suffix` that passes the character check but whose
normalized result escapes the directory — this requires a creative but valid
path on the target OS, or a mock of `keyDirectory`).
##########
components/camel-pqc/src/test/java/org/apache/camel/component/pqc/lifecycle/FileBasedKeyLifecycleManagerPathTest.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.camel.component.pqc.lifecycle;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * keyId values reach the file-based key store from message headers
(CamelPQCKeyId / CamelPQCNewKeyId). The manager must
+ * keep every key file inside its configured directory regardless of the
supplied keyId.
+ */
+class FileBasedKeyLifecycleManagerPathTest {
+
+ @Test
+ void rejectsKeyIdWithParentTraversal(@TempDir Path tempDir) throws
Exception {
+ Path keyDir = tempDir.resolve("keys");
+ FileBasedKeyLifecycleManager manager = new
FileBasedKeyLifecycleManager(keyDir.toString());
+
+ // A sentinel file one level above the key directory that an
unconfined "../victim" keyId would target
+ Path sentinel = tempDir.resolve("victim.private.json");
+ Files.writeString(sentinel, "{}");
+
+ assertThrows(IllegalArgumentException.class, () ->
manager.getKey("../victim"));
+ assertThrows(IllegalArgumentException.class, () ->
manager.deleteKey("../victim"));
+
+ assertTrue(Files.exists(sentinel), "a file outside the key directory
must not be reachable via keyId");
+ }
+
+ @Test
+ void rejectsAbsoluteSeparatorAndBlankKeyIds(@TempDir Path tempDir) throws
Exception {
+ Path keyDir = tempDir.resolve("keys");
+ FileBasedKeyLifecycleManager manager = new
FileBasedKeyLifecycleManager(keyDir.toString());
+
+ assertThrows(IllegalArgumentException.class, () ->
manager.getKeyMetadata("sub/evil"));
+ assertThrows(IllegalArgumentException.class, () ->
manager.getKeyMetadata("/etc/evil"));
+ assertThrows(IllegalArgumentException.class,
+ () ->
manager.getKeyMetadata(tempDir.resolve("abs").toString()));
+ assertThrows(IllegalArgumentException.class, () ->
manager.getKeyMetadata(""));
+ }
+
+ @Test
+ void allowsPlainKeyId(@TempDir Path tempDir) throws Exception {
+ Path keyDir = tempDir.resolve("keys");
+ FileBasedKeyLifecycleManager manager = new
FileBasedKeyLifecycleManager(keyDir.toString());
+
+ // A normal flat keyId is accepted: metadata for an absent key returns
null rather than being rejected
+ assertNull(manager.getKeyMetadata("tenant-a-signing-key"));
Review Comment:
🔍 **Low — `allowsPlainKeyId` only verifies no exception; it doesn't assert
the resolved path stays inside the directory**
`assertNull(manager.getKeyMetadata("tenant-a-signing-key"))` confirms the
call doesn't throw, but it says nothing about where the path resolved. A future
refactor that broke path construction (e.g. accidentally used
`keyDirectory.getParent()` as the base) would still pass this assertion. Since
`resolveKeyFile` returns the `Path` (visible to the caller via the helper
methods), the acceptance test should verify the resolved path:
```suggestion
// A normal flat keyId is accepted: metadata for an absent key
returns null rather than being rejected
assertNull(manager.getKeyMetadata("tenant-a-signing-key"));
// Verify the resolved private-key path (via getKey which calls
resolveKeyFile) stays inside keyDir
Path keyDirPath = keyDir.toAbsolutePath().normalize();
// getKey returns null for absent key, but the path check happens
before the file-existence check
// — use getKeyMetadata (same resolver) and assert no exception is
the proxy for path acceptance
// (ideally expose a package-private getKeyFilePath for testing;
as-is, null return is sufficient)
assertTrue(keyDirPath.toString().startsWith(keyDirPath.getParent().toString()),
"sanity: keyDir is a proper subdirectory");
```
Alternatively, make `resolveKeyFile` package-private and add a direct
assertion:
`assertTrue(manager.resolveKeyFile("tenant-a-signing-key",
".private.json").startsWith(keyDir.toAbsolutePath().normalize()))`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
