From 22f0f81507c05c8f41b39c20026ddbe7d8e301f2 Mon Sep 17 00:00:00 2001 From: Peter Zaoral Date: Mon, 18 Nov 2024 09:28:05 +0100 Subject: [PATCH] fix: prevent inclusion of characters that could lead to FileVault path traversal (#219) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes: #211 Signed-off-by: Peter Zaoral Co-authored-by: Václav Muzikář --- .../server_admin/topics/vault.adoc | 7 +- .../topics/changes/changes-24_0_9.adoc | 6 + docs/guides/server/vault.adoc | 28 ++-- .../keycloak/vault/AbstractVaultProvider.java | 44 +++++- .../vault/AbstractVaultProviderFactory.java | 2 +- .../vault/FilesPlainTextVaultProvider.java | 18 +++ .../vault/PlainTextVaultProviderTest.java | 140 +++++++++++++++++- .../org/keycloak/vault/keyonly__escaped | 1 + .../org/keycloak/vault/keyonly_legacy | 1 + 9 files changed, 227 insertions(+), 20 deletions(-) create mode 100644 services/src/test/resources/org/keycloak/vault/keyonly__escaped create mode 100644 services/src/test/resources/org/keycloak/vault/keyonly_legacy diff --git a/docs/documentation/server_admin/topics/vault.adoc b/docs/documentation/server_admin/topics/vault.adoc index 4dabd3f6545..b6b63404e51 100644 --- a/docs/documentation/server_admin/topics/vault.adoc +++ b/docs/documentation/server_admin/topics/vault.adoc @@ -26,6 +26,7 @@ In the <<_ldap,LDAP settings>> of LDAP-based user federation. OIDC identity provider secret:: In the _Client Secret_ inside identity provider <<_identity_broker_oidc,OpenID Connect Config>> +[[_vault-key-resolvers]] === Key resolvers All built-in providers support the configuration of key resolvers. A key resolver implements the algorithm or strategy for combining the realm name with the key, obtained from the `${vault.key}` expression, into the final entry name used to retrieve the secret from the vault. {project_name} uses the `keyResolvers` property to configure the resolvers that the provider uses. The value is a comma-separated list of resolver names. An example of the configuration for the `files-plaintext` provider follows: @@ -45,13 +46,13 @@ A list of the currently available resolvers follows: |Name |Description | KEY_ONLY -| {project_name} ignores the realm name and uses the key from the vault expression. +| {project_name} ignores the realm name and uses the key from the vault expression. {project_name} escapes occurrences of underscores in the key with another underscore character. For example, if the key is called `my_secret`, {project_name} searches for an entry in the vault named `my++__++secret`. This is to prevent conflicts with the default `REALM_UNDERSCORE_KEY` resolver. | REALM_UNDERSCORE_KEY | {project_name} combines the realm and key by using an underscore character. {project_name} escapes occurrences of underscores in the realm or key with another underscore character. For example, if the realm is called `master_realm` and the key is `smtp_key`, the combined key is `master+++__+++realm_smtp+++__+++key`. | REALM_FILESEPARATOR_KEY -| {project_name} combines the realm and key by using the platform file separator character. +| {project_name} combines the realm and key by using the platform file separator character. The vault expression prohibits the use of characters that could cause path traversal, thus preventing access to secrets outside the corresponding realm. ifeval::[{project_community}==true] | FACTORY_PROVIDED @@ -60,4 +61,4 @@ endif::[] |=== -If you have not configured a resolver for the built-in providers, {project_name} selects the `REALM_UNDERSCORE_KEY`. \ No newline at end of file +If you have not configured a resolver for the built-in providers, {project_name} selects the `REALM_UNDERSCORE_KEY`. diff --git a/docs/documentation/upgrading/topics/changes/changes-24_0_9.adoc b/docs/documentation/upgrading/topics/changes/changes-24_0_9.adoc index 84a512846a5..a41638484c0 100644 --- a/docs/documentation/upgrading/topics/changes/changes-24_0_9.adoc +++ b/docs/documentation/upgrading/topics/changes/changes-24_0_9.adoc @@ -4,3 +4,9 @@ Potential vulnerable configurations have been identified in the X.509 client cer Additional configuration steps might be required depending on your current configuration. Make sure to review the updated link:{client_certificate_lookup_link}[reverse proxy guide] if you have configured the client certificate lookup via a proxy header. + += Security improvements for the key resolvers + +While using the `REALM_FILESEPARATOR_KEY` key resolver, {project_name} now restricts access to FileVault secrets outside of its realm. Characters that could cause path traversal when specifying the expression placeholder in the Administration Console are now prohibited. + +Additionally, the `KEY_ONLY` key resolver now escapes the `+_+` character to prevent reading secrets that would otherwise be linked to another realm when the `REALM_UNDERSCORE_KEY` resolver is used. The escaping simply replaces `+_+` with `+__+`, so, for example, `${vault.my_secret}` now looks for a file named `my++__++secret`. We recognize that this is a breaking change; therefore, a warning is logged to ease the transition. diff --git a/docs/guides/server/vault.adoc b/docs/guides/server/vault.adoc index f2788641c6e..91ea96b4740 100644 --- a/docs/guides/server/vault.adoc +++ b/docs/guides/server/vault.adoc @@ -43,19 +43,6 @@ Kubernetes/OpenShift Secrets are used on a per-realm basis in {project_name}, wh ${r"${vault._}"} ---- -=== Using underscores in the Name -To process the secret correctly, you double all underscores in the or the , separated by a single underscore. - -.Example -* Realm Name: `sso_realm` -* Desired Name: `ldap_credential` -* Resulting file Name: -[source, bash] ----- -sso__realm_ldap__credential ----- -Note the doubled underscores between __sso__ and __realm__ and also between __ldap__ and __credential__. - == Configuring the Java KeyStore-based vault In order to use the Java KeyStore-based vault, you need to create a KeyStore file first. You can use the following command for doing so: @@ -75,6 +62,21 @@ Note that the `--vault-type` parameter is optional and defaults to `PKCS12`. Secrets stored in the vault can then be accessed in a realm via the following placeholder (assuming using the `REALM_UNDERSCORE_KEY` key resolver): `${r"${vault.realm-name_alias}"}`. +== Using underscores in the secret names +To process the secret correctly, you double all underscores in the . When `REALM_UNDERSCORE_KEY` key resolver is used, underscores in are also doubled and and is separated by a single underscore. + +.Example +* Realm Name: `sso_realm` +* Desired Name: `ldap_credential` +* Resulting file name: +[source, bash] +---- +sso__realm_ldap__credential +---- +Note the doubled underscores between __sso__ and __realm__ and also between __ldap__ and __credential__. + +To learn more about key resolvers, see link:{adminguide_link}#_vault-key-resolvers[Key resolvers section in the Server Administration guide]. + == Example: Use an LDAP bind credential secret in the Admin Console .Example setup diff --git a/services/src/main/java/org/keycloak/vault/AbstractVaultProvider.java b/services/src/main/java/org/keycloak/vault/AbstractVaultProvider.java index b0677dee691..8f234b10599 100644 --- a/services/src/main/java/org/keycloak/vault/AbstractVaultProvider.java +++ b/services/src/main/java/org/keycloak/vault/AbstractVaultProvider.java @@ -17,6 +17,10 @@ package org.keycloak.vault; +import org.jboss.logging.Logger; + +import java.io.File; +import java.lang.invoke.MethodHandles; import java.util.List; import java.util.Optional; @@ -38,6 +42,8 @@ import java.util.Optional; */ public abstract class AbstractVaultProvider implements VaultProvider { + private static final Logger logger = Logger.getLogger(MethodHandles.lookup().lookupClass()); + protected final String realm; protected final List resolvers; @@ -56,14 +62,50 @@ public abstract class AbstractVaultProvider implements VaultProvider { @Override public VaultRawSecret obtainSecret(String vaultSecretId) { for (VaultKeyResolver resolver : this.resolvers) { - VaultRawSecret secret = this.obtainSecretInternal(resolver.apply(this.realm, vaultSecretId)); + String resolvedKey = resolver.apply(this.realm, vaultSecretId); + if (!validate(resolver, vaultSecretId, resolvedKey)) { + logger.warnf("Validation failed for secret %s with resolved key %s", vaultSecretId, resolvedKey); + return DefaultVaultRawSecret.forBuffer(Optional.empty()); + } + } + + for (VaultKeyResolver resolver : this.resolvers) { + String resolvedKey = resolver.apply(this.realm, vaultSecretId); + VaultRawSecret secret = this.obtainSecretInternal(resolvedKey); if (secret != null && secret.get().isPresent()) { return secret; } + checkForLegacyKey(resolver, vaultSecretId, resolvedKey); } return DefaultVaultRawSecret.forBuffer(Optional.empty()); } + private void checkForLegacyKey(VaultKeyResolver resolver, String vaultSecretId, String resolvedKey) { + if (resolver == AbstractVaultProviderFactory.AvailableResolvers.KEY_ONLY.getVaultKeyResolver() && vaultSecretId.contains("_")) { + String legacyKey = vaultSecretId.replaceAll("__", "_"); + VaultRawSecret legacySecret = this.obtainSecretInternal(legacyKey); + if (legacySecret != null && legacySecret.get().isPresent()) { + logger.warnf("Secret was found using legacy key '%s'. Please rename the key to '%s' and repeat the action.", legacyKey, resolvedKey); + } + } + } + + /** + * Validates the resolved key to ensure it meets the necessary criteria. + * + * @param resolver the {@link VaultKeyResolver} used to resolve the key. + * @param key the original key provided. + * @param resolvedKey the key after being resolved by the resolver. + * @return a boolean indicating whether the validation passed. + */ + protected boolean validate(VaultKeyResolver resolver, String key, String resolvedKey) { + if (key.contains(File.separator)) { + logger.warnf("Key %s contains invalid file separator character", key); + return false; + } + return true; + } + /** * Subclasses of {@code AbstractVaultProvider} must implement this method. It is meant to be implemented in the same * way as the {@link #obtainSecret(String)} method from the {@link VaultProvider} interface, but the specified vault diff --git a/services/src/main/java/org/keycloak/vault/AbstractVaultProviderFactory.java b/services/src/main/java/org/keycloak/vault/AbstractVaultProviderFactory.java index f092a7bfe47..6d407d34add 100644 --- a/services/src/main/java/org/keycloak/vault/AbstractVaultProviderFactory.java +++ b/services/src/main/java/org/keycloak/vault/AbstractVaultProviderFactory.java @@ -140,7 +140,7 @@ public abstract class AbstractVaultProviderFactory implements VaultProviderFacto * all realms to share the secrets, so instead of replicating entries for all existing realms in the vault one can * simply use key directly and all realms will obtain the same secret. */ - KEY_ONLY((realm, key) -> key), + KEY_ONLY((realm, key) -> key.replaceAll("_", "__")), /** * The realm is prepended to the vault key and they are separated by an underscore ({@code '_'}) character. If either diff --git a/services/src/main/java/org/keycloak/vault/FilesPlainTextVaultProvider.java b/services/src/main/java/org/keycloak/vault/FilesPlainTextVaultProvider.java index 4339c0f0140..e4531243187 100644 --- a/services/src/main/java/org/keycloak/vault/FilesPlainTextVaultProvider.java +++ b/services/src/main/java/org/keycloak/vault/FilesPlainTextVaultProvider.java @@ -63,6 +63,24 @@ public class FilesPlainTextVaultProvider extends AbstractVaultProvider { } } + @Override + protected boolean validate(VaultKeyResolver resolver, String key, String resolvedKey) { + if (!super.validate(resolver, key, resolvedKey)) { + return false; + } + Path secretPath = vaultPath.resolve(resolvedKey); + + Path expectedPath = vaultPath; + if (resolver == AbstractVaultProviderFactory.AvailableResolvers.REALM_FILESEPARATOR_KEY.getVaultKeyResolver()) { + expectedPath = expectedPath.resolve(realm); + } + if (!secretPath.getParent().equals(expectedPath)) { + logger.warnf("Path traversal attempt detected in secret %s.", key); + return false; + } + return true; + } + @Override public void close() { diff --git a/services/src/test/java/org/keycloak/vault/PlainTextVaultProviderTest.java b/services/src/test/java/org/keycloak/vault/PlainTextVaultProviderTest.java index 2933a6a8b42..dbbe845fef5 100644 --- a/services/src/test/java/org/keycloak/vault/PlainTextVaultProviderTest.java +++ b/services/src/test/java/org/keycloak/vault/PlainTextVaultProviderTest.java @@ -1,17 +1,29 @@ package org.keycloak.vault; import org.junit.Test; +import org.junit.Before; +import org.junit.After; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.util.Arrays; +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; +import java.io.ByteArrayOutputStream; +import java.io.PrintStream; import static org.hamcrest.CoreMatchers.not; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.assertTrue; import static org.keycloak.vault.SecretContains.secretContains; /** @@ -21,6 +33,38 @@ import static org.keycloak.vault.SecretContains.secretContains; */ public class PlainTextVaultProviderTest { + private static final Logger logger = Logger.getLogger("org.keycloak.vault"); + private BlockingQueue logMessages; + private final ByteArrayOutputStream errContent = new ByteArrayOutputStream(); + private final PrintStream originalErr = System.err; + private Handler logHandler; + + @Before + public void setUp() { + logMessages = new LinkedBlockingQueue<>(); + logger.setLevel(Level.WARNING); + logHandler = new Handler() { + @Override + public void publish(LogRecord record) { + logMessages.add(record.getMessage()); + } + + @Override + public void flush() { } + + @Override + public void close() throws SecurityException { } + }; + logger.addHandler(logHandler); + System.setErr(new PrintStream(errContent)); + } + + @After + public void tearDown() { + logger.removeHandler(logHandler); + System.setErr(originalErr); + } + @Test public void shouldObtainSecret() throws Exception { //given @@ -69,7 +113,7 @@ public class PlainTextVaultProviderTest { public void shouldOperateOnNonExistingVaultDirectory() throws Exception { //given FilesPlainTextVaultProvider provider = new FilesPlainTextVaultProvider(Scenario.NON_EXISTING.getPath(), "test", - Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.REALM_UNDERSCORE_KEY.getVaultKeyResolver())); + Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.REALM_UNDERSCORE_KEY.getVaultKeyResolver())); //when VaultRawSecret secret = provider.obtainSecret("non-existing-key"); @@ -161,4 +205,96 @@ public class PlainTextVaultProviderTest { assertThat(secretAfterFirstRead, not(secretContains("secret"))); assertThat(secretAfterSecondRead, secretContains("secret")); } -} \ No newline at end of file + + @Test + public void shouldPreventPathFileSeparatorInVaultSecretId() { + // given + FilesPlainTextVaultProvider provider = new FilesPlainTextVaultProvider( + Scenario.EXISTING.getPath(), + "test", + Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.REALM_FILESEPARATOR_KEY.getVaultKeyResolver()) + ); + + // when + VaultRawSecret secret = provider.obtainSecret(".../key1"); + + // then + assertNotNull(secret); + assertFalse(secret.get().isPresent()); + assertTrue( + logMessages.stream() + .anyMatch(msg -> msg.contains("Key .../key1 contains invalid file separator character")) + ); + } + + @Test + public void shouldNotValidateWithInvalidPath() { + // given + Path vaultPath = Paths.get("/vault"); + FilesPlainTextVaultProvider provider = new FilesPlainTextVaultProvider(vaultPath, "test_realm", + Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.REALM_FILESEPARATOR_KEY.getVaultKeyResolver())); + VaultKeyResolver resolver = AbstractVaultProviderFactory.AvailableResolvers.REALM_FILESEPARATOR_KEY.getVaultKeyResolver(); + String key = "key1"; + String resolvedKey = "../key1"; + + // when + boolean isValid = provider.validate(resolver, key, resolvedKey); + + // then + assertFalse(isValid); + } + + @Test + public void shouldValidateWithDifferentResolver() { + // given + Path vaultPath = Paths.get("/vault"); + FilesPlainTextVaultProvider provider = new FilesPlainTextVaultProvider(vaultPath, "test_realm", + Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.KEY_ONLY.getVaultKeyResolver())); + VaultKeyResolver resolver = AbstractVaultProviderFactory.AvailableResolvers.KEY_ONLY.getVaultKeyResolver(); + String key = "key1"; + String resolvedKey = "key1"; + + // when + boolean isValid = provider.validate(resolver, key, resolvedKey); + + // then + assertTrue(isValid); + } + + @Test + public void shouldSearchForEscapedKeyOnlySecret() throws Exception { + // given + FilesPlainTextVaultProvider provider = new FilesPlainTextVaultProvider(Scenario.EXISTING.getPath(), "test", + Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.KEY_ONLY.getVaultKeyResolver())); + + // when + VaultRawSecret secret = provider.obtainSecret("keyonly_escaped"); + + // then + assertNotNull(secret); + assertNotNull(secret.get().get()); + assertThat(secret, secretContains("expected_secret_value")); + } + + @Test + public void shouldSearchForKeyOnlyLegacy() throws Exception { + // given + FilesPlainTextVaultProvider provider = new FilesPlainTextVaultProvider( + Scenario.EXISTING.getPath(), + "test", + Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.KEY_ONLY.getVaultKeyResolver()) + ); + + // when + VaultRawSecret secret = provider.obtainSecret("keyonly_legacy"); + + // then + assertNotNull(secret); + assertFalse(secret.get().isPresent()); + assertTrue( + logMessages.stream() + .anyMatch(msg -> msg.contains("Secret was found using legacy key 'keyonly_legacy'. Please rename the key to 'keyonly__legacy' and repeat the action.")) + ); + } + +} diff --git a/services/src/test/resources/org/keycloak/vault/keyonly__escaped b/services/src/test/resources/org/keycloak/vault/keyonly__escaped new file mode 100644 index 00000000000..d3f5ba6657d --- /dev/null +++ b/services/src/test/resources/org/keycloak/vault/keyonly__escaped @@ -0,0 +1 @@ +expected_secret_value \ No newline at end of file diff --git a/services/src/test/resources/org/keycloak/vault/keyonly_legacy b/services/src/test/resources/org/keycloak/vault/keyonly_legacy new file mode 100644 index 00000000000..32eea160a5d --- /dev/null +++ b/services/src/test/resources/org/keycloak/vault/keyonly_legacy @@ -0,0 +1 @@ +should_not_be_retrieved \ No newline at end of file