Skip to content

Commit fb76bed

Browse files
tvernumtvernum
authored
Make SecureString comparisons constant time (elastic#135053)
Changes the SecureString methods `equals` `startsWith` and `regionMatches` to operate in constant time relative to the length of that comparison string, regardless of the length of the secure string. That is, the time to perform each of these comparisons should be the same (even though some of them _could_ be computed more efficiently) new SecureString("a").equals("abcdefghijklmn") new SecureString("abcdefghijklmn").equals("abcdefghijklmn") new SecureString("abcdefghijklmn").equals("##############") new SecureString("abcdefghijklmX").equals("abcdefghijklmn") new SecureString("X".repeat(5000)).equals("ababababababab") And similarly for `startsWith` and `regionMatches`
1 parent 62e53cd commit fb76bed

File tree

3 files changed

+105
-22
lines changed

3 files changed

+105
-22
lines changed

‎docs/changelog/135053.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 135053
2+
summary: Make `SecureString` comparisons constant time
3+
area: Infra/Core
4+
type: enhancement
5+
issues: []

‎server/src/main/java/org/elasticsearch/common/settings/SecureString.java‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -43,25 +43,27 @@ public SecureString(String s) {
4343
this(s.toCharArray());
4444
}
4545

46-
/** Constant time equality to avoid potential timing attacks. */
4746
@Override
4847
public synchronized boolean equals(Object o) {
49-
ensureNotClosed();
50-
if (this == o) return true;
51-
if (o == null || o instanceof CharSequence == false) return false;
52-
CharSequence that = (CharSequence) o;
53-
if (chars.length != that.length()) {
48+
if (this == o) {
49+
return true;
50+
}
51+
if (o instanceof CharSequence cs) {
52+
return equals(cs);
53+
} else {
5454
return false;
5555
}
56+
}
5657

57-
return compareChars(0, that, 0, chars.length);
58+
/** Constant time equality to avoid potential timing attacks. */
59+
public synchronized boolean equals(CharSequence that) {
60+
ensureNotClosed();
61+
// This is intentional to make sure the check is constant time relative to the length of the comparison string
62+
return that != null && compareChars(0, that, 0, that.length()) && this.length() == that.length();
5863
}
5964

6065
public boolean startsWith(CharSequence other) {
6166
ensureNotClosed();
62-
if (this.length() < other.length()) {
63-
return false;
64-
}
6567
return compareChars(0, other, 0, other.length());
6668
}
6769

@@ -79,8 +81,9 @@ public synchronized boolean regionMatches(int thisOffset, CharSequence other, in
7981
}
8082

8183
// Perform some calculations as long to prevent overflow
82-
if (thisOffset + (long) len > this.length() || otherOffset + (long) len > other.length()) {
83-
// cannot compare a region that runs past the end of the source
84+
if (otherOffset + (long) len > other.length()) {
85+
// cannot compare a region that runs past the end of the comparison string
86+
// we don't check the length of our string because we want to run in constant time
8487
return false;
8588
}
8689

@@ -99,13 +102,12 @@ public synchronized boolean regionMatches(int thisOffset, CharSequence other, in
99102
* Constant time comparison of a range from {@link #chars} against an identical length range from {@code other}
100103
*/
101104
private boolean compareChars(int thisOffset, CharSequence other, int otherOffset, int len) {
102-
assert len <= this.chars.length : "len is longer that secure-string: " + len + " vs " + this.chars.length;
103105
assert len <= other.length() : "len is longer that comparison string: " + len + " vs " + other.length();
104106

105107
int equals = 0;
106108
for (int i = 0; i < len; i++) {
107-
final char t = chars[thisOffset + i];
108109
final char o = other.charAt(otherOffset + i);
110+
final int t = thisOffset + i < chars.length ? chars[thisOffset + i] : (Character.MAX_VALUE + 1);
109111
equals |= t ^ o;
110112
}
111113

‎server/src/test/java/org/elasticsearch/common/settings/SecureStringTests.java‎

Lines changed: 84 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import org.elasticsearch.test.ESTestCase;
1313

1414
import java.util.Arrays;
15+
import java.util.List;
1516

1617
import static org.hamcrest.CoreMatchers.is;
1718
import static org.hamcrest.Matchers.containsString;
@@ -76,16 +77,91 @@ public void testGetCloseableCharsAfterSecureStringClosed() {
7677
}
7778

7879
public void testEquals() {
79-
final String string = randomAlphaOfLengthBetween(8, 128);
80-
final SecureString secureString = new SecureString(string.toCharArray());
81-
final StringBuilder stringBuilder = new StringBuilder(string);
80+
final List<String> strings = List.of(
81+
randomAlphaOfLengthBetween(8, 128),
82+
(char) 0 + randomAlphanumericOfLength(9),
83+
Character.MAX_VALUE + randomAlphanumericOfLength(10),
84+
randomAlphanumericOfLength(11) + (char) 0,
85+
randomAlphanumericOfLength(12) + Character.MAX_VALUE,
86+
randomAlphanumericOfLength(3) + (char) 0 + randomAlphanumericOfLength(10),
87+
randomAlphanumericOfLength(4) + Character.MAX_VALUE + randomAlphanumericOfLength(10)
88+
);
89+
for (var string : strings) {
90+
final SecureString secureString = new SecureString(string.toCharArray());
91+
92+
verifyEquals(secureString, string, true);
93+
94+
// Comparison has extra character in the middle
95+
final int split = randomIntBetween(1, string.length() - 1);
96+
final String altString = string.substring(0, split) + randomAlphanumericOfLength(1) + string.substring(split);
97+
verifyEquals(secureString, altString, false);
98+
99+
// Comparison has extra characters at beginning
100+
verifyEquals(secureString, randomAlphanumericOfLength(1) + string, false);
101+
verifyEquals(secureString, randomAlphaOfLengthBetween(2, 8) + string, false);
102+
verifyEquals(secureString, "\0" + string, false);
103+
verifyEquals(secureString, (char) -1 + string, false);
104+
105+
// Comparison has extra characters at end
106+
verifyEquals(secureString, string + randomAlphanumericOfLength(1), false);
107+
verifyEquals(secureString, string + randomAlphaOfLengthBetween(2, 8), false);
108+
verifyEquals(secureString, string + '\0', false);
109+
verifyEquals(secureString, string + (char) -1, false);
110+
111+
// Comparison is missing characters at beginning
112+
verifyEquals(secureString, string.substring(1), false);
113+
verifyEquals(secureString, string.substring(randomIntBetween(2, string.length() - 2)), false);
114+
115+
// Comparison is missing characters at end
116+
verifyEquals(secureString, string.substring(0, string.length() - 1), false);
117+
verifyEquals(secureString, string.substring(0, string.length() - randomIntBetween(2, string.length() - 2)), false);
118+
119+
// Comparison has different character at beginning
120+
verifyEquals(
121+
secureString,
122+
randomValueOtherThan(string.substring(0, 1), () -> randomAlphanumericOfLength(1)) + string.substring(1),
123+
false
124+
);
125+
if (string.charAt(0) != 0) {
126+
verifyEquals(secureString, "\0" + string.substring(1), false);
127+
}
128+
if (string.charAt(0) != (char) -1) {
129+
verifyEquals(secureString, (char) -1 + string.substring(1), false);
130+
}
131+
// Comparison has different character at end
132+
verifyEquals(
133+
secureString,
134+
string.substring(0, string.length() - 1) + randomValueOtherThan(
135+
string.substring(string.length() - 1),
136+
() -> randomAlphanumericOfLength(1)
137+
),
138+
false
139+
);
140+
if (string.endsWith("\0") == false) {
141+
verifyEquals(secureString, string.substring(0, string.length() - 1) + '\0', false);
142+
}
143+
if (string.charAt(string.length() - 1) != (char) -1) {
144+
verifyEquals(secureString, string.substring(0, string.length() - 1) + (char) -1, false);
145+
}
82146

83-
assertThat(secureString.equals(string), is(true));
84-
assertThat(secureString.equals(stringBuilder), is(true));
147+
assertThat(secureString.equals(""), is(false));
148+
final Object obj = null;
149+
// noinspection ConstantValue
150+
assertThat(secureString.equals(obj), is(false));
151+
152+
final CharSequence cs = null;
153+
assertThat(secureString.equals(cs), is(false));
154+
}
155+
156+
}
85157

86-
final int split = randomIntBetween(1, string.length() - 1);
87-
final String altString = string.substring(0, split) + randomAlphanumericOfLength(1) + string.substring(split);
88-
assertThat(secureString.equals(altString), is(false));
158+
@SuppressWarnings("EqualsBetweenInconvertibleTypes")
159+
private void verifyEquals(SecureString secure, String string, boolean expected) {
160+
// Verify that we get the same result for different types of CharSequence
161+
assertThat(secure + " == " + string, secure.equals(string), is(expected));
162+
assertThat(secure.equals(new SecureString(string.toCharArray())), is(expected));
163+
assertThat(secure.equals(new StringBuilder(string)), is(expected));
164+
assertThat(secure.equals((Object) string), is(expected));
89165
}
90166

91167
public void testStartsWith() {

0 commit comments

Comments
 (0)