This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/8.5.x by this push:
new 46827ad Harden CredentialHandler implementations - constant-time
comparisons
46827ad is described below
commit 46827ad4d2ffdb11605e6afd815f325146882851
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Mar 17 13:36:20 2022 +0000
Harden CredentialHandler implementations - constant-time comparisons
Based on a patch proposed by schultz
---
.../realm/DigestCredentialHandlerBase.java | 75 +++++++++++++++++++++-
.../realm/MessageDigestCredentialHandler.java | 5 +-
webapps/docs/changelog.xml | 4 ++
3 files changed, 81 insertions(+), 3 deletions(-)
diff --git a/java/org/apache/catalina/realm/DigestCredentialHandlerBase.java
b/java/org/apache/catalina/realm/DigestCredentialHandlerBase.java
index b6a33d5..122adef 100644
--- a/java/org/apache/catalina/realm/DigestCredentialHandlerBase.java
+++ b/java/org/apache/catalina/realm/DigestCredentialHandlerBase.java
@@ -16,6 +16,7 @@
*/
package org.apache.catalina.realm;
+import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.util.Random;
@@ -200,7 +201,7 @@ public abstract class DigestCredentialHandlerBase
implements CredentialHandler {
return false;
}
- return storedHexEncoded.equalsIgnoreCase(inputHexEncoded);
+ return DigestCredentialHandlerBase.equals(storedHexEncoded,
inputHexEncoded, true);
}
@@ -292,4 +293,76 @@ public abstract class DigestCredentialHandlerBase
implements CredentialHandler {
* @return the logger for the CredentialHandler instance.
*/
protected abstract Log getLog();
+
+ /**
+ * Implements String equality which always compares all characters in the
+ * string, without stopping early if any characters do not match.
+ *
+ * @implNote
+ * This implementation was adapted from {@link MessageDigest#isEqual}
+ * which we assume is as optimizer-defeating as possible.
+ *
+ * @param s1 The first string to compare.
+ * @param s2 The second string to compare.
+ * @param ignoreCase <code>true</code> if the strings should be compared
+ * without regard to case. Note that "true" here is only guaranteed
+ * to work with plain ASCII characters.
+ *
+ * @return <code>true</code> if the strings are equal to each other,
+ * <code>false</code> otherwise.
+ */
+ public static boolean equals(final String s1, final String s2, final
boolean ignoreCase) {
+ if (s1 == s2) {
+ return true;
+ }
+ if (s1 == null || s2 == null) {
+ return false;
+ }
+
+ final int len1 = s1.length();
+ final int len2 = s2.length();
+
+ if (len2 == 0) {
+ return len1 == 0;
+ }
+
+ int result = 0;
+ result |= len1 - len2;
+
+ // time-constant comparison
+ for (int i = 0; i < len1; i++) {
+ // If i >= len2, index2 is 0; otherwise, i.
+ final int index2 = ((i - len2) >>> 31) * i;
+ char c1 = s1.charAt(i);
+ char c2 = s2.charAt(index2);
+ if(ignoreCase) {
+ c1 = Character.toLowerCase(c1);
+ c2 = Character.toLowerCase(c2);
+ }
+ result |= c1 ^ c2;
+ }
+ return result == 0;
+ }
+
+ /**
+ * Implements byte-array equality which always compares all bytes in the
+ * array, without stopping early if any bytes do not match.
+ *
+ * @implNote
+ * Implementation note: this method delegates to {@link
MessageDigest#isEqual}
+ * under the assumption that it provides a constant-time comparison of the
+ * bytes in the arrays. Java 7+ has such an implementation, but neither the
+ * Javadoc nor any specification requires it. Therefore, Tomcat should
+ * continue to use <i>this</i> method internally in case the JDK
+ * implementation changes so this method can be re-implemented properly.
+ *
+ * @param b1 The first array to compare.
+ * @param b2 The second array to compare.
+ *
+ * @return <code>true</code> if the arrays are equal to each other,
+ * <code>false</code> otherwise.
+ */
+ public static boolean equals(final byte[] b1, final byte[] b2) {
+ return MessageDigest.isEqual(b1, b2);
+ }
}
diff --git a/java/org/apache/catalina/realm/MessageDigestCredentialHandler.java
b/java/org/apache/catalina/realm/MessageDigestCredentialHandler.java
index 616e0e5..d43465c 100644
--- a/java/org/apache/catalina/realm/MessageDigestCredentialHandler.java
+++ b/java/org/apache/catalina/realm/MessageDigestCredentialHandler.java
@@ -100,7 +100,7 @@ public class MessageDigestCredentialHandler extends
DigestCredentialHandlerBase
if (getAlgorithm() == null) {
// No digests, compare directly
- return storedCredentials.equals(inputCredentials);
+ return DigestCredentialHandlerBase.equals(inputCredentials,
storedCredentials, false);
} else {
// Some directories and databases prefix the password with the hash
// type. The string is in a format compatible with Base64.encode
not
@@ -112,7 +112,8 @@ public class MessageDigestCredentialHandler extends
DigestCredentialHandlerBase
byte[] userDigest = ConcurrentMessageDigest.digest(
getAlgorithm(),
inputCredentials.getBytes(StandardCharsets.ISO_8859_1));
String base64UserDigest =
Base64.encodeBase64String(userDigest);
- return base64UserDigest.equals(base64ServerDigest);
+
+ return DigestCredentialHandlerBase.equals(base64UserDigest,
base64ServerDigest, false);
} else if (storedCredentials.startsWith("{SSHA}")) {
// "{SSHA}<sha-1 digest:20><salt:n>"
// Need to convert the salt to bytes to apply it to the user's
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 8af1ae0..d10be4b 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -112,6 +112,10 @@
specific error codes and/or exception types with the
<code>ErrorReportValve</code>. (markt)
</add>
+ <scode>
+ Harden the CredentialHandler implementations by switching to a
+ constant-time implementation for credential comparisons.
(schultz/markt)
+ </scode>
</changelog>
</subsection>
<subsection name="Web applications">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
