honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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@fb6430ea9d
This commit is contained in:
jaymode 2015-09-08 12:42:53 -04:00
parent 9e3bf47a87
commit a5d9c45dd3
3 changed files with 82 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
shield/src/main/java/org/elasticsearch/shield/authc/support/BCrypt.java
View File

@ -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);

14
shield/src/main/java/org/elasticsearch/shield/authc/support/SecuredString.java
View File

@ -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

70
shield/src/test/java/org/elasticsearch/shield/authc/support/BCryptTests.java Normal file
View File

@ -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<Callable<Boolean>> callables = new ArrayList<>(100);
final AtomicBoolean failed = new AtomicBoolean(false);
for (int i = 0; i < 100; i++) {
callables.add(new Callable<Boolean>() {
@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<Future<Boolean>> futures = threadPoolExecutor.invokeAll(callables);
for (Future<Boolean> future : futures) {
assertThat(future.get(), is(true));
}
} finally {
threadPoolExecutor.shutdownNow();
}
}
}