Skip to content

Commit

Permalink
Use SecureString for password length validation (#42884)
Browse files Browse the repository at this point in the history 
This replaces the use of char[] in the password length validation
code, with the use of SecureString

Although the use of char[] is not in itself problematic, using a
SecureString encourages callers to think about the lifetime of the
password object and to clear it after use.
  • Loading branch information
@tvernum
tvernum authored Jun 14, 2019
1 parent e27f473 commit 3667db9
Show file tree
Hide file tree
Showing 10 changed files with 89 additions and 77 deletions.
2 changes: 1 addition & 1 deletion .../java/org/elasticsearch/xpack/core/security/action/user/ChangePasswordRequestBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public ChangePasswordRequestBuilder username(String username) {
}

public static char[] validateAndHashPassword(SecureString password, Hasher hasher) {
Validation.Error error = Validation.Users.validatePassword(password.getChars());
Validation.Error error = Validation.Users.validatePassword(password);
if (error != null) {
ValidationException validationException = new ValidationException();
validationException.addValidationError(error.toString());
Expand Down
17 changes: 12 additions & 5 deletions ...rc/main/java/org/elasticsearch/xpack/core/security/action/user/PutUserRequestBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

import java.io.IOException;
import java.io.InputStream;
import java.util.Arrays;
import java.util.Map;
import java.util.Objects;

Expand All @@ -50,7 +49,15 @@ public PutUserRequestBuilder roles(String... roles) {
return this;
}

/**
* @deprecated Use {@link #password(SecureString, Hasher)} instead.
*/
@Deprecated
public PutUserRequestBuilder password(char[] password, Hasher hasher) {
return password(new SecureString(password), hasher);
}

public PutUserRequestBuilder password(SecureString password, Hasher hasher) {
if (password != null) {
Validation.Error error = Validation.Users.validatePassword(password);
if (error != null) {
Expand All @@ -59,7 +66,7 @@ public PutUserRequestBuilder password(char[] password, Hasher hasher) {
if (request.passwordHash() != null) {
throw validationException("password_hash has already been set");
}
request.passwordHash(hasher.hash(new SecureString(password)));
request.passwordHash(hasher.hash(password));
} else {
request.passwordHash(null);
}
Expand Down Expand Up @@ -119,9 +126,9 @@ public PutUserRequestBuilder source(String username, BytesReference source, XCon
} else if (User.Fields.PASSWORD.match(currentFieldName, parser.getDeprecationHandler())) {
if (token == XContentParser.Token.VALUE_STRING) {
String password = parser.text();
char[] passwordChars = password.toCharArray();
password(passwordChars, hasher);
Arrays.fill(passwordChars, (char) 0);
try(SecureString securePassword = new SecureString(password.toCharArray())) {
password(securePassword, hasher);
}
} else {
throw new ElasticsearchParseException(
"expected field [{}] to be of type string, but found [{}] instead", currentFieldName, token);
Expand Down
9 changes: 5 additions & 4 deletions ...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/Validation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.core.security.support;

import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.xpack.core.security.authc.esnative.ClientReservedRealm;
Expand Down Expand Up @@ -81,10 +82,10 @@ public static Error validateUsername(String username, boolean allowReserved, Set
return null;
}

public static Error validatePassword(char[] password) {
return password.length >= MIN_PASSWD_LENGTH ?
null :
new Error("passwords must be at least [" + MIN_PASSWD_LENGTH + "] characters long");
public static Error validatePassword(SecureString password) {
return password.length() >= MIN_PASSWD_LENGTH ?
null :
new Error("passwords must be at least [" + MIN_PASSWD_LENGTH + "] characters long");
}

}
Expand Down
7 changes: 4 additions & 3 deletions ...gin/core/src/test/java/org/elasticsearch/xpack/core/security/support/ValidationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.core.security.support;

import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore;
Expand Down Expand Up @@ -59,12 +60,12 @@ public void testUsernameInvalidWhitespace() throws Exception {
}

public void testUsersValidatePassword() throws Exception {
String passwd = randomAlphaOfLength(randomIntBetween(0, 20));
SecureString passwd = new SecureString(randomAlphaOfLength(randomIntBetween(0, 20)).toCharArray());
logger.info("{}[{}]", passwd, passwd.length());
if (passwd.length() >= 6) {
assertThat(Users.validatePassword(passwd.toCharArray()), nullValue());
assertThat(Users.validatePassword(passwd), nullValue());
} else {
assertThat(Users.validatePassword(passwd.toCharArray()), notNullValue());
assertThat(Users.validatePassword(passwd), notNullValue());
}
}

Expand Down
2 changes: 1 addition & 1 deletion ...src/main/java/org/elasticsearch/xpack/security/authc/esnative/tool/SetupPasswordTool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ private SecureString promptForPassword(Terminal terminal, String user) throws Us
// loop for two consecutive good passwords
while (true) {
SecureString password1 = new SecureString(terminal.readSecret("Enter password for [" + user + "]: "));
Validation.Error err = Validation.Users.validatePassword(password1.getChars());
Validation.Error err = Validation.Users.validatePassword(password1);
if (err != null) {
terminal.println(err.toString());
terminal.println("Try again.");
Expand Down
30 changes: 18 additions & 12 deletions ...in/security/src/main/java/org/elasticsearch/xpack/security/authc/file/tool/UsersTool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import joptsimple.OptionSet;
import joptsimple.OptionSpec;

import org.elasticsearch.cli.EnvironmentAwareCommand;
import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.LoggingAwareMultiCommand;
Expand Down Expand Up @@ -111,7 +110,7 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
throw new UserException(ExitCodes.DATA_ERROR, "Invalid username [" + username + "]... " + validationError);
}

char[] password = parsePassword(terminal, passwordOption.value(options));
final char[] passwordHash = getPasswordHash(terminal, env, passwordOption.value(options));
String[] roles = parseRoles(terminal, env, rolesOption.value(options));

Path passwordFile = FileUserPasswdStore.resolveFile(env);
Expand All @@ -125,9 +124,8 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
if (users.containsKey(username)) {
throw new UserException(ExitCodes.CODE_ERROR, "User [" + username + "] already exists");
}
final Hasher hasher = Hasher.resolve(XPackSettings.PASSWORD_HASHING_ALGORITHM.get(env.settings()));
users = new HashMap<>(users); // make modifiable
users.put(username, hasher.hash(new SecureString(password)));
users.put(username, passwordHash);
FileUserPasswdStore.writeFile(users, passwordFile);

if (roles.length > 0) {
Expand Down Expand Up @@ -218,7 +216,7 @@ protected void printAdditionalHelp(Terminal terminal) {
protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {

String username = parseUsername(arguments.values(options), env.settings());
char[] password = parsePassword(terminal, passwordOption.value(options));
char[] passwordHash = getPasswordHash(terminal, env, passwordOption.value(options));

Path file = FileUserPasswdStore.resolveFile(env);
FileAttributesChecker attributesChecker = new FileAttributesChecker(file);
Expand All @@ -229,9 +227,8 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
if (users.containsKey(username) == false) {
throw new UserException(ExitCodes.NO_USER, "User [" + username + "] doesn't exist");
}
final Hasher hasher = Hasher.resolve(XPackSettings.PASSWORD_HASHING_ALGORITHM.get(env.settings()));
users = new HashMap<>(users); // make modifiable
users.put(username, hasher.hash(new SecureString(password)));
users.put(username, passwordHash);
FileUserPasswdStore.writeFile(users, file);

attributesChecker.check(terminal);
Expand Down Expand Up @@ -440,23 +437,32 @@ static String parseUsername(List<String> args, Settings settings) throws UserExc
return username;
}

private static char[] getPasswordHash(Terminal terminal, Environment env, String cliPasswordValue) throws UserException {
final Hasher hasher = Hasher.resolve(XPackSettings.PASSWORD_HASHING_ALGORITHM.get(env.settings()));
final char[] passwordHash;
try (SecureString password = parsePassword(terminal, cliPasswordValue)) {
passwordHash = hasher.hash(password);
}
return passwordHash;
}

// pkg private for testing
static char[] parsePassword(Terminal terminal, String passwordStr) throws UserException {
char[] password;
static SecureString parsePassword(Terminal terminal, String passwordStr) throws UserException {
SecureString password;
if (passwordStr != null) {
password = passwordStr.toCharArray();
password = new SecureString(passwordStr.toCharArray());
Validation.Error validationError = Users.validatePassword(password);
if (validationError != null) {
throw new UserException(ExitCodes.DATA_ERROR, "Invalid password..." + validationError);
}
} else {
password = terminal.readSecret("Enter new password: ");
password = new SecureString(terminal.readSecret("Enter new password: "));
Validation.Error validationError = Users.validatePassword(password);
if (validationError != null) {
throw new UserException(ExitCodes.DATA_ERROR, "Invalid password..." + validationError);
}
char[] retyped = terminal.readSecret("Retype new password: ");
if (Arrays.equals(password, retyped) == false) {
if (Arrays.equals(password.getChars(), retyped) == false) {
throw new UserException(ExitCodes.DATA_ERROR, "Password mismatch");
}
}
Expand Down
Loading

0 comments on commit 3667db9

Please sign in to comment.