From 49b977ba7ca482d518f3001221a117baa3ffe2ae Mon Sep 17 00:00:00 2001
From: Ioannis Kakavas <ikakavas@protonmail.com>
Date: Tue, 3 Jul 2018 11:31:48 +0300
Subject: [PATCH] resolveHasher defaults to NOOP (#31723)

* Default resolveFromHash to Hasher.NOOP

This changes the default behavior when resolving the hashing
algorithm from unrecognised hash strings, which was introduced in
 #31234

A hash string that doesn't start with an algorithm identifier can
either be a malformed/corrupted hash or a plaintext password when
Hasher.NOOP is used(against warnings).
Do not make assumptions about which of the two is true for such
strings and default to Hasher.NOOP. Hash verification will subsequently
fail for malformed hashes.
Finally, do not log the potentially malformed hash as this can very
well be a plaintext password.

Resolves #31697
Reverts 58cf95a06f1defd31b16c831708ca32a5b445f98
---
 .../xpack/core/security/authc/support/Hasher.java | 15 ++++++---------
 .../xpack/security/authc/file/FileRealmTests.java |  2 --
 .../xpack/security/authc/support/HasherTests.java |  5 +----
 3 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java
index f5275de5fc8..d12547bd906 100644
--- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java
+++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/Hasher.java
@@ -438,7 +438,8 @@ public enum Hasher {
 
     /**
      * Returns a {@link Hasher} instance that can be used to verify the {@code hash} by inspecting the
-     * hash prefix and determining the algorithm used for its generation.
+     * hash prefix and determining the algorithm used for its generation. If no specific algorithm
+     * prefix, can be determined {@code Hasher.NOOP} is returned.
      *
      * @param hash the char array from which the hashing algorithm is to be deduced
      * @return the hasher that can be used for validation
@@ -457,7 +458,8 @@ public enum Hasher {
         } else if (CharArrays.charsBeginsWith(SSHA256_PREFIX, hash)) {
             return Hasher.SSHA256;
         } else {
-            throw new IllegalArgumentException("unknown hash format for hash [" + new String(hash) + "]");
+            // This is either a non hashed password from cache or a corrupted hash string.
+            return Hasher.NOOP;
         }
     }
 
@@ -471,13 +473,8 @@ public enum Hasher {
      * @return true if the hash corresponds to the data, false otherwise
      */
     public static boolean verifyHash(SecureString data, char[] hash) {
-        try {
-            final Hasher hasher = resolveFromHash(hash);
-            return hasher.verify(data, hash);
-        } catch (IllegalArgumentException e) {
-            // The password hash format is invalid, we're unable to verify password
-            return false;
-        }
+        final Hasher hasher = resolveFromHash(hash);
+        return hasher.verify(data, hash);
     }
 
     private static char[] getPbkdf2Hash(SecureString data, int cost) {
diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileRealmTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileRealmTests.java
index b06697bc4eb..f5dad8b7c68 100644
--- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileRealmTests.java
+++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileRealmTests.java
@@ -84,13 +84,11 @@ public class FileRealmTests extends ESTestCase {
         assertThat(user.roles(), arrayContaining("role1", "role2"));
     }
 
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/31697")
     public void testAuthenticateCaching() throws Exception {
         Settings settings = Settings.builder()
             .put("cache.hash_algo", Hasher.values()[randomIntBetween(0, Hasher.values().length - 1)].name().toLowerCase(Locale.ROOT)).build();
         RealmConfig config = new RealmConfig("file-test", settings, globalSettings, TestEnvironment.newEnvironment(globalSettings),
             threadContext);
-
         when(userPasswdStore.verifyPassword(eq("user1"), eq(new SecureString("test123")), any(Supplier.class)))
                 .thenAnswer(VERIFY_PASSWORD_ANSWER);
         when(userRolesStore.roles("user1")).thenReturn(new String[]{"role1", "role2"});
diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/HasherTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/HasherTests.java
index c303c0ab468..6086dc642d2 100644
--- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/HasherTests.java
+++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/HasherTests.java
@@ -128,10 +128,7 @@ public class HasherTests extends ESTestCase {
         assertThat(Hasher.resolveFromHash(
             "{PBKDF2}1000000$UuyhtjDEzWmE2wyY80akZKPWWpy2r2X50so41YML82U=$WFasYLelqbjQwt3EqFlUcwHiC38EZC45Iu/Iz0xL1GQ=".toCharArray()),
             sameInstance(Hasher.PBKDF2_1000000));
-        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {
-            Hasher.resolveFromHash("{GBGN}cGR8S2vr3FuFuOpQitR".toCharArray());
-        });
-        assertThat(e.getMessage(), containsString("unknown hash format for hash"));
+        assertThat(Hasher.resolveFromHash("notavalidhashformat".toCharArray()), sameInstance(Hasher.NOOP));
     }
 
     private static void testHasherSelfGenerated(Hasher hasher) {