Skip to content

Commit 059eb55

Browse files
tvernumtvernum
authored
Use SecureString for password length validation (#43465)
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. Backport of: #42884
1 parent c88f2f2 commit 059eb55

File tree

11 files changed

+96
-82
lines changed

11 files changed

+96
-82
lines changed

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/ChangePasswordRequestBuilder.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ public ChangePasswordRequestBuilder username(String username) {
4242
}
4343

4444
public static char[] validateAndHashPassword(SecureString password, Hasher hasher) {
45-
Validation.Error error = Validation.Users.validatePassword(password.getChars());
45+
Validation.Error error = Validation.Users.validatePassword(password);
4646
if (error != null) {
4747
ValidationException validationException = new ValidationException();
4848
validationException.addValidationError(error.toString());

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/PutUserRequestBuilder.java

+12-5
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525

2626
import java.io.IOException;
2727
import java.io.InputStream;
28-
import java.util.Arrays;
2928
import java.util.Map;
3029
import java.util.Objects;
3130

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

52+
/**
53+
* @deprecated Use {@link #password(SecureString, Hasher)} instead.
54+
*/
55+
@Deprecated
5356
public PutUserRequestBuilder password(char[] password, Hasher hasher) {
57+
return password(new SecureString(password), hasher);
58+
}
59+
60+
public PutUserRequestBuilder password(SecureString password, Hasher hasher) {
5461
if (password != null) {
5562
Validation.Error error = Validation.Users.validatePassword(password);
5663
if (error != null) {
@@ -59,7 +66,7 @@ public PutUserRequestBuilder password(char[] password, Hasher hasher) {
5966
if (request.passwordHash() != null) {
6067
throw validationException("password_hash has already been set");
6168
}
62-
request.passwordHash(hasher.hash(new SecureString(password)));
69+
request.passwordHash(hasher.hash(password));
6370
} else {
6471
request.passwordHash(null);
6572
}
@@ -119,9 +126,9 @@ public PutUserRequestBuilder source(String username, BytesReference source, XCon
119126
} else if (User.Fields.PASSWORD.match(currentFieldName, parser.getDeprecationHandler())) {
120127
if (token == XContentParser.Token.VALUE_STRING) {
121128
String password = parser.text();
122-
char[] passwordChars = password.toCharArray();
123-
password(passwordChars, hasher);
124-
Arrays.fill(passwordChars, (char) 0);
129+
try(SecureString securePassword = new SecureString(password.toCharArray())) {
130+
password(securePassword, hasher);
131+
}
125132
} else {
126133
throw new ElasticsearchParseException(
127134
"expected field [{}] to be of type string, but found [{}] instead", currentFieldName, token);

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/client/SecurityClient.java

+5
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import org.elasticsearch.action.ActionListener;
1010
import org.elasticsearch.client.ElasticsearchClient;
1111
import org.elasticsearch.common.bytes.BytesReference;
12+
import org.elasticsearch.common.settings.SecureString;
1213
import org.elasticsearch.common.xcontent.XContentType;
1314
import org.elasticsearch.xpack.core.security.action.CreateApiKeyAction;
1415
import org.elasticsearch.xpack.core.security.action.CreateApiKeyRequest;
@@ -224,6 +225,10 @@ public PutUserRequestBuilder preparePutUser(String username, char[] password, Ha
224225
return new PutUserRequestBuilder(client).username(username).password(password, hasher).roles(roles);
225226
}
226227

228+
public PutUserRequestBuilder preparePutUser(String username, SecureString password, Hasher hasher, String... roles) {
229+
return new PutUserRequestBuilder(client).username(username).password(password, hasher).roles(roles);
230+
}
231+
227232
public void putUser(PutUserRequest request, ActionListener<PutUserResponse> listener) {
228233
client.execute(PutUserAction.INSTANCE, request, listener);
229234
}

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/Validation.java

+5-4
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
*/
66
package org.elasticsearch.xpack.core.security.support;
77

8+
import org.elasticsearch.common.settings.SecureString;
89
import org.elasticsearch.common.settings.Settings;
910
import org.elasticsearch.common.util.set.Sets;
1011
import org.elasticsearch.xpack.core.security.authc.esnative.ClientReservedRealm;
@@ -81,10 +82,10 @@ public static Error validateUsername(String username, boolean allowReserved, Set
8182
return null;
8283
}
8384

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

9091
}

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/support/ValidationTests.java

+4-3
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
*/
66
package org.elasticsearch.xpack.core.security.support;
77

8+
import org.elasticsearch.common.settings.SecureString;
89
import org.elasticsearch.common.settings.Settings;
910
import org.elasticsearch.test.ESTestCase;
1011
import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore;
@@ -59,12 +60,12 @@ public void testUsernameInvalidWhitespace() throws Exception {
5960
}
6061

6162
public void testUsersValidatePassword() throws Exception {
62-
String passwd = randomAlphaOfLength(randomIntBetween(0, 20));
63+
SecureString passwd = new SecureString(randomAlphaOfLength(randomIntBetween(0, 20)).toCharArray());
6364
logger.info("{}[{}]", passwd, passwd.length());
6465
if (passwd.length() >= 6) {
65-
assertThat(Users.validatePassword(passwd.toCharArray()), nullValue());
66+
assertThat(Users.validatePassword(passwd), nullValue());
6667
} else {
67-
assertThat(Users.validatePassword(passwd.toCharArray()), notNullValue());
68+
assertThat(Users.validatePassword(passwd), notNullValue());
6869
}
6970
}
7071

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/tool/SetupPasswordTool.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ private SecureString promptForPassword(Terminal terminal, String user) throws Us
193193
// loop for two consecutive good passwords
194194
while (true) {
195195
SecureString password1 = new SecureString(terminal.readSecret("Enter password for [" + user + "]: "));
196-
Validation.Error err = Validation.Users.validatePassword(password1.getChars());
196+
Validation.Error err = Validation.Users.validatePassword(password1);
197197
if (err != null) {
198198
terminal.println(err.toString());
199199
terminal.println("Try again.");

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/tool/UsersTool.java

+18-12
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77

88
import joptsimple.OptionSet;
99
import joptsimple.OptionSpec;
10-
1110
import org.elasticsearch.cli.EnvironmentAwareCommand;
1211
import org.elasticsearch.cli.ExitCodes;
1312
import org.elasticsearch.cli.LoggingAwareMultiCommand;
@@ -111,7 +110,7 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
111110
throw new UserException(ExitCodes.DATA_ERROR, "Invalid username [" + username + "]... " + validationError);
112111
}
113112

114-
char[] password = parsePassword(terminal, passwordOption.value(options));
113+
final char[] passwordHash = getPasswordHash(terminal, env, passwordOption.value(options));
115114
String[] roles = parseRoles(terminal, env, rolesOption.value(options));
116115

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

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

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

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

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

440+
private static char[] getPasswordHash(Terminal terminal, Environment env, String cliPasswordValue) throws UserException {
441+
final Hasher hasher = Hasher.resolve(XPackSettings.PASSWORD_HASHING_ALGORITHM.get(env.settings()));
442+
final char[] passwordHash;
443+
try (SecureString password = parsePassword(terminal, cliPasswordValue)) {
444+
passwordHash = hasher.hash(password);
445+
}
446+
return passwordHash;
447+
}
448+
443449
// pkg private for testing
444-
static char[] parsePassword(Terminal terminal, String passwordStr) throws UserException {
445-
char[] password;
450+
static SecureString parsePassword(Terminal terminal, String passwordStr) throws UserException {
451+
SecureString password;
446452
if (passwordStr != null) {
447-
password = passwordStr.toCharArray();
453+
password = new SecureString(passwordStr.toCharArray());
448454
Validation.Error validationError = Users.validatePassword(password);
449455
if (validationError != null) {
450456
throw new UserException(ExitCodes.DATA_ERROR, "Invalid password..." + validationError);
451457
}
452458
} else {
453-
password = terminal.readSecret("Enter new password: ");
459+
password = new SecureString(terminal.readSecret("Enter new password: "));
454460
Validation.Error validationError = Users.validatePassword(password);
455461
if (validationError != null) {
456462
throw new UserException(ExitCodes.DATA_ERROR, "Invalid password..." + validationError);
457463
}
458464
char[] retyped = terminal.readSecret("Retype new password: ");
459-
if (Arrays.equals(password, retyped) == false) {
465+
if (Arrays.equals(password.getChars(), retyped) == false) {
460466
throw new UserException(ExitCodes.DATA_ERROR, "Password mismatch");
461467
}
462468
}

0 commit comments

Comments
 (0)