From 8180f4785753181d7eb3d4fc31cb51878d7838b2 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 30 Jul 2018 14:48:30 +0300 Subject: [PATCH 1/3] Ensure decryption related exceptions are handled This commit ensures that all possible Exceptions in KeyStoreWrapper#decrypt() are handled. More specifically, in the case that a wrong password is used for secure settings, calling readX on the DataInputStream that wraps the CipherInputStream can throw an IOException. It also adds a test for loading a KeyStoreWrapper with a wrong password. --- .../elasticsearch/common/settings/KeyStoreWrapper.java | 2 +- .../action/admin/ReloadSecureSettingsIT.java | 7 ------- .../common/settings/KeyStoreWrapperTests.java | 10 ++++++++++ 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index eee45743ee32e..e017e9e7ca93f 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -359,7 +359,7 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio if (input.read() != -1) { throw new SecurityException("Keystore has been corrupted or tampered with"); } - } catch (EOFException e) { + } catch (IOException e) { throw new SecurityException("Keystore has been corrupted or tampered with", e); } } diff --git a/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java b/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java index c8503603f665c..f1a51d1ddf956 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java @@ -205,14 +205,7 @@ public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { assertThat(nodesMap.size(), equalTo(cluster().size())); for (final NodesReloadSecureSettingsResponse.NodeResponse nodeResponse : nodesReloadResponse.getNodes()) { assertThat(nodeResponse.reloadException(), notNullValue()); - // Running in a JVM with a BouncyCastle FIPS Security Provider, decrypting the Keystore with the wrong - // password returns a SecurityException if the DataInputStream can't be fully consumed - if (inFipsJvm()) { assertThat(nodeResponse.reloadException(), instanceOf(SecurityException.class)); - } else { - assertThat(nodeResponse.reloadException(), instanceOf(IOException.class)); - } - } } catch (final AssertionError e) { reloadSettingsError.set(e); diff --git a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java index fe7b02d63ecce..9b8d6e53d6459 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java @@ -99,6 +99,16 @@ public void testCreate() throws Exception { assertTrue(keystore.getSettingNames().contains(KeyStoreWrapper.SEED_SETTING.getKey())); } + public void testDecryptKeyStoreWithWrongPassword() throws Exception { + KeyStoreWrapper keystore = KeyStoreWrapper.create(); + keystore.save(env.configFile(), new char[0]); + final KeyStoreWrapper loadedkeystore = KeyStoreWrapper.load(env.configFile()); + final Exception exception = expectThrows(Exception.class, + () -> loadedkeystore.decrypt(new char[]{'i', 'n', 'v', 'a', 'l', 'i', 'd'})); + exception.printStackTrace(); + assertThat(exception.getMessage(), containsString("Keystore has been corrupted or tampered with")); + } + public void testCannotReadStringFromClosedKeystore() throws Exception { KeyStoreWrapper keystore = KeyStoreWrapper.create(); assertThat(keystore.getSettingNames(), Matchers.hasItem(KeyStoreWrapper.SEED_SETTING.getKey())); From c8b4968f604aeefb3026bf0d135d1098733dca5b Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 30 Jul 2018 18:55:18 +0300 Subject: [PATCH 2/3] Remove unused import --- .../org/elasticsearch/action/admin/ReloadSecureSettingsIT.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java b/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java index f1a51d1ddf956..7952758240544 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java @@ -32,7 +32,6 @@ import org.elasticsearch.plugins.ReloadablePlugin; import org.elasticsearch.test.ESIntegTestCase; -import java.io.IOException; import java.io.InputStream; import java.nio.file.Files; import java.nio.file.StandardCopyOption; From bacfb773cb59f8151fb52c19b5aa17827eacfbbf Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 30 Jul 2018 19:46:37 +0300 Subject: [PATCH 3/3] Correct the test --- .../elasticsearch/common/settings/KeyStoreWrapperTests.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java index 9b8d6e53d6459..bb2b1df7f8c03 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java @@ -103,9 +103,8 @@ public void testDecryptKeyStoreWithWrongPassword() throws Exception { KeyStoreWrapper keystore = KeyStoreWrapper.create(); keystore.save(env.configFile(), new char[0]); final KeyStoreWrapper loadedkeystore = KeyStoreWrapper.load(env.configFile()); - final Exception exception = expectThrows(Exception.class, + final SecurityException exception = expectThrows(SecurityException.class, () -> loadedkeystore.decrypt(new char[]{'i', 'n', 'v', 'a', 'l', 'i', 'd'})); - exception.printStackTrace(); assertThat(exception.getMessage(), containsString("Keystore has been corrupted or tampered with")); }