From a5d9c45dd302929f4a32aec7009be50adba6be40 Mon Sep 17 00:00:00 2001 From: jaymode Date: Tue, 8 Sep 2015 12:42:53 -0400 Subject: [PATCH] remove the override of finalize in SecuredString This change removes the override of finalize in SecuredString to resolve a issue where the char[] can be cleared by the call in the finalize method but the char array is still being used. The specific issue that occurs is in the BCrypt usage of the SecuredString. A character is concatenated and then the utf8Bytes method is called. In most cases, the proper bytes are returned but occasionally the byte array is returned with only zeroes. This occurs under load and/or memory pressure and can be provoked by running BCryptTests with a small heap (12 - 16 megabytes) and the SecuredString implementation with the overridden finalize method. Closes elastic/elasticsearch#589 Original commit: elastic/x-pack-elasticsearch@fb6430ea9d4e20f0526d36142990ae81d0066e2c --- .../shield/authc/support/BCrypt.java | 11 ++- .../shield/authc/support/SecuredString.java | 14 +--- .../shield/authc/support/BCryptTests.java | 70 +++++++++++++++++++ 3 files changed, 82 insertions(+), 13 deletions(-) create mode 100644 shield/src/test/java/org/elasticsearch/shield/authc/support/BCryptTests.java diff --git a/shield/src/main/java/org/elasticsearch/shield/authc/support/BCrypt.java b/shield/src/main/java/org/elasticsearch/shield/authc/support/BCrypt.java index af6e563897c..a8a96a57941 100644 --- a/shield/src/main/java/org/elasticsearch/shield/authc/support/BCrypt.java +++ b/shield/src/main/java/org/elasticsearch/shield/authc/support/BCrypt.java @@ -685,8 +685,15 @@ public class BCrypt { */ - // the next line is the SecuredString replacement for the above commented-out section - passwordb = ( minor >= 'a' ? password.concat("\000"): password ).utf8Bytes(); + // the next lines are the SecuredString replacement for the above commented-out section + if (minor >= 'a') { + SecuredString securedString = password.concat("\000"); + passwordb = securedString.utf8Bytes(); + // clear here since this is a new object and we don't need to reuse it + securedString.clear(); + } else { + passwordb = password.utf8Bytes(); + } /*************************** ES CHANGE END *************************/ saltb = decode_base64(real_salt, BCRYPT_SALT_LEN); diff --git a/shield/src/main/java/org/elasticsearch/shield/authc/support/SecuredString.java b/shield/src/main/java/org/elasticsearch/shield/authc/support/SecuredString.java index f8eb8e5dd39..d317cc1aab5 100644 --- a/shield/src/main/java/org/elasticsearch/shield/authc/support/SecuredString.java +++ b/shield/src/main/java/org/elasticsearch/shield/authc/support/SecuredString.java @@ -24,8 +24,7 @@ public class SecuredString implements CharSequence { private boolean cleared = false; /** - * Note: the passed in chars are not duplicated, but used directly for performance/optimization. DO NOT - * modify or clear the chars after it has been passed into this constructor. + * Note: the passed in chars are duplicated */ public SecuredString(char[] chars) { this.chars = new char[chars.length]; @@ -33,8 +32,7 @@ public class SecuredString implements CharSequence { } /** - * This constructor is used internally for the concatenate method. It DOES duplicate the passed in array, unlike - * the public constructor + * This constructor is used internally for the concatenate method. */ private SecuredString(char[] chars, int start, int end) { this.chars = new char[end - start]; @@ -89,7 +87,7 @@ public class SecuredString implements CharSequence { } /** - * @return A copy of the internal charachters. May be usd for caching. + * @return A copy of the internal characters. May be used for caching. */ public char[] copyChars() { return Arrays.copyOf(chars, chars.length); @@ -129,12 +127,6 @@ public class SecuredString implements CharSequence { Arrays.fill(chars, (char) 0); } - @Override - public void finalize() throws Throwable { - clear(); - super.finalize(); - } - /** * @param toAppend String to combine with this SecureString * @return a new SecureString with toAppend concatenated diff --git a/shield/src/test/java/org/elasticsearch/shield/authc/support/BCryptTests.java b/shield/src/test/java/org/elasticsearch/shield/authc/support/BCryptTests.java new file mode 100644 index 00000000000..0f5c3d7763c --- /dev/null +++ b/shield/src/test/java/org/elasticsearch/shield/authc/support/BCryptTests.java @@ -0,0 +1,70 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.shield.authc.support; + +import org.elasticsearch.test.ESTestCase; +import org.junit.Test; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicBoolean; + +import static org.hamcrest.Matchers.is; + +/** + * Tests for the Bcrypt implementation specifically around modifications we have made + */ +public class BCryptTests extends ESTestCase { + + + /* + * This test checks that the BCrypt implementation can verify passwords correctly when being invoked from multiple + * threads all the time. This attempts to simulate authentication of many clients at once (without a cache). + * + * This test can be used to reproduce the issue in https://github.com/elastic/x-plugins/issues/589, but it is not + * 100% reliable unless memory parameters are changed such as lowering the heap size to something really small like + * 16M and the test is really slow since the issue depends on garbage collection and object finalization. + */ + @Test + @AwaitsFix(bugUrl = "need a better way to test this") + public void testUnderLoad() throws Exception { + final String password = randomAsciiOfLengthBetween(10, 32); + final String bcrypt = BCrypt.hashpw(SecuredStringTests.build(password), BCrypt.gensalt()); + + ExecutorService threadPoolExecutor = Executors.newFixedThreadPool(100); + try { + List> callables = new ArrayList<>(100); + + final AtomicBoolean failed = new AtomicBoolean(false); + for (int i = 0; i < 100; i++) { + callables.add(new Callable() { + @Override + public Boolean call() throws Exception { + for (int i = 0; i < 10000 && !failed.get(); i++) { + if (BCrypt.checkpw(SecuredStringTests.build(password), bcrypt) == false) { + failed.set(true); + return false; + } + } + return true; + } + }); + } + + List> futures = threadPoolExecutor.invokeAll(callables); + for (Future future : futures) { + assertThat(future.get(), is(true)); + } + } finally { + threadPoolExecutor.shutdownNow(); + } + + } +}