From 8139e3a1c7aa70e0f0c8363dcc22bbdbd3ef93c4 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 5 Dec 2017 14:30:36 -0800 Subject: [PATCH] Add validation of keystore setting names (#27626) This commit restricts settings added to the keystore to have a lowercase ascii name. The java Keystore javadocs state that case sensitivity of key alias names are implementation dependent. This ensures regardless of case sensitivity in a jvm implementation, the keys will be stored as we expect. --- .../common/settings/KeyStoreWrapper.java | 21 ++++++++++++++++++- .../common/settings/SecureSetting.java | 1 + .../common/settings/KeyStoreWrapperTests.java | 10 +++++++++ .../common/settings/SettingsTests.java | 9 ++++++++ 4 files changed, 40 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index 441bb131f03..6ebc47c8252 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -49,6 +49,7 @@ import java.util.HashSet; import java.util.Locale; import java.util.Map; import java.util.Set; +import java.util.regex.Pattern; import java.util.stream.Collectors; import org.apache.lucene.codecs.CodecUtil; @@ -59,7 +60,6 @@ import org.apache.lucene.store.IndexInput; import org.apache.lucene.store.IndexOutput; import org.apache.lucene.store.SimpleFSDirectory; import org.apache.lucene.util.SetOnce; -import org.elasticsearch.bootstrap.BootstrapSettings; import org.elasticsearch.cli.ExitCodes; import org.elasticsearch.cli.UserException; import org.elasticsearch.common.Randomness; @@ -75,6 +75,11 @@ import org.elasticsearch.common.Randomness; */ public class KeyStoreWrapper implements SecureSettings { + /** + * A regex for the valid characters that a setting name in the keystore may use. + */ + private static final Pattern ALLOWED_SETTING_NAME = Pattern.compile("[a-z0-9_\\-.]+"); + public static final Setting SEED_SETTING = SecureSetting.secureString("keystore.seed", null); /** Characters that may be used in the bootstrap seed setting added to all keystores. */ @@ -383,6 +388,18 @@ public class KeyStoreWrapper implements SecureSettings { return Base64.getDecoder().wrap(bytesStream); } + /** + * Ensure the given setting name is allowed. + * + * @throws IllegalArgumentException if the setting name is not valid + */ + public static void validateSettingName(String setting) { + if (ALLOWED_SETTING_NAME.matcher(setting).matches() == false) { + throw new IllegalArgumentException("Setting name [" + setting + "] does not match the allowed setting name pattern [" + + ALLOWED_SETTING_NAME.pattern() + "]"); + } + } + /** * Set a string setting. * @@ -390,6 +407,7 @@ public class KeyStoreWrapper implements SecureSettings { */ void setString(String setting, char[] value) throws GeneralSecurityException { assert isLoaded(); + validateSettingName(setting); if (ASCII_ENCODER.canEncode(CharBuffer.wrap(value)) == false) { throw new IllegalArgumentException("Value must be ascii"); } @@ -401,6 +419,7 @@ public class KeyStoreWrapper implements SecureSettings { /** Set a file setting. */ void setFile(String setting, byte[] bytes) throws GeneralSecurityException { assert isLoaded(); + validateSettingName(setting); bytes = Base64.getEncoder().encode(bytes); char[] chars = new char[bytes.length]; for (int i = 0; i < chars.length; ++i) { diff --git a/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java b/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java index 4a1e598bba8..c23a0bd42e3 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java +++ b/core/src/main/java/org/elasticsearch/common/settings/SecureSetting.java @@ -46,6 +46,7 @@ public abstract class SecureSetting extends Setting { private SecureSetting(String key, Property... properties) { super(key, (String)null, null, ArrayUtils.concat(properties, FIXED_PROPERTIES, Property.class)); assert assertAllowedProperties(properties); + KeyStoreWrapper.validateSettingName(key); } private boolean assertAllowedProperties(Setting.Property... properties) { diff --git a/core/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java b/core/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java index 11d1e1f5735..c3b34b7c3ef 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java @@ -97,4 +97,14 @@ public class KeyStoreWrapperTests extends ESTestCase { keystore.decrypt(new char[0]); assertEquals(seed.toString(), keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey()).toString()); } + + public void testIllegalSettingName() throws Exception { + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> KeyStoreWrapper.validateSettingName("UpperCase")); + assertTrue(e.getMessage().contains("does not match the allowed setting name pattern")); + KeyStoreWrapper keystore = KeyStoreWrapper.create(new char[0]); + e = expectThrows(IllegalArgumentException.class, () -> keystore.setString("UpperCase", new char[0])); + assertTrue(e.getMessage().contains("does not match the allowed setting name pattern")); + e = expectThrows(IllegalArgumentException.class, () -> keystore.setFile("UpperCase", new byte[0])); + assertTrue(e.getMessage().contains("does not match the allowed setting name pattern")); + } } diff --git a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java index 04cd1717e7f..039de112fac 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java @@ -486,6 +486,15 @@ public class SettingsTests extends ESTestCase { assertTrue(e.getMessage().contains("must be stored inside the Elasticsearch keystore")); } + public void testSecureSettingIllegalName() { + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + SecureSetting.secureString("UpperCaseSetting", null)); + assertTrue(e.getMessage().contains("does not match the allowed setting name pattern")); + e = expectThrows(IllegalArgumentException.class, () -> + SecureSetting.secureFile("UpperCaseSetting", null)); + assertTrue(e.getMessage().contains("does not match the allowed setting name pattern")); + } + public void testGetAsArrayFailsOnDuplicates() { final IllegalStateException e = expectThrows(IllegalStateException.class, () -> Settings.builder() .put("foobar.0", "bar")