From b1b94d870e0c8ac8d6fb9356ea4c455ddb56c20c Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 16 May 2017 10:25:34 +0200 Subject: [PATCH 1/2] Code cleanups. --- .../jetty/util/security/Credential.java | 29 ++++--------------- 1 file changed, 6 insertions(+), 23 deletions(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java b/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java index 54296ecbd4e..9b855faf092 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java @@ -26,7 +26,6 @@ import org.eclipse.jetty.util.TypeUtil; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; -/* ------------------------------------------------------------ */ /** * Credentials. The Credential class represents an abstract mechanism for * checking authentication credentials. A credential instance either represents @@ -39,15 +38,12 @@ import org.eclipse.jetty.util.log.Logger; * This class includes an implementation for unix Crypt an MD5 digest. * * @see Password - * */ public abstract class Credential implements Serializable { + private static final long serialVersionUID = -7760551052768181572L; private static final Logger LOG = Log.getLogger(Credential.class); - private static final long serialVersionUID = -7760551052768181572L; - - /* ------------------------------------------------------------ */ /** * Check a credential * @@ -59,7 +55,6 @@ public abstract class Credential implements Serializable */ public abstract boolean check(Object credentials); - /* ------------------------------------------------------------ */ /** * Get a credential from a String. If the credential String starts with a * known Credential type (eg "CRYPT:" or "MD5:" ) then a Credential of that @@ -76,15 +71,13 @@ public abstract class Credential implements Serializable return new Password(credential); } - /* ------------------------------------------------------------ */ /** * Unix Crypt Credentials */ public static class Crypt extends Credential { private static final long serialVersionUID = -2027792997664744210L; - - public static final String __TYPE = "CRYPT:"; + private static final String __TYPE = "CRYPT:"; private final String _cooked; @@ -100,58 +93,49 @@ public abstract class Credential implements Serializable credentials=new String((char[])credentials); if (!(credentials instanceof String) && !(credentials instanceof Password)) LOG.warn("Can't check " + credentials.getClass() + " against CRYPT"); - String passwd = credentials.toString(); return _cooked.equals(UnixCrypt.crypt(passwd, _cooked)); } public static String crypt(String user, String pw) { - return "CRYPT:" + UnixCrypt.crypt(pw, user); + return __TYPE + UnixCrypt.crypt(pw, user); } } - /* ------------------------------------------------------------ */ /** * MD5 Credentials */ public static class MD5 extends Credential { private static final long serialVersionUID = 5533846540822684240L; - - public static final String __TYPE = "MD5:"; - - public static final Object __md5Lock = new Object(); - + private static final String __TYPE = "MD5:"; + private static final Object __md5Lock = new Object(); private static MessageDigest __md; private final byte[] _digest; - /* ------------------------------------------------------------ */ MD5(String digest) { digest = digest.startsWith(__TYPE) ? digest.substring(__TYPE.length()) : digest; _digest = TypeUtil.parseBytes(digest, 16); } - /* ------------------------------------------------------------ */ public byte[] getDigest() { return _digest; } - /* ------------------------------------------------------------ */ @Override public boolean check(Object credentials) { try { - byte[] digest = null; - if (credentials instanceof char[]) credentials=new String((char[])credentials); if (credentials instanceof Password || credentials instanceof String) { + byte[] digest; synchronized (__md5Lock) { if (__md == null) __md = MessageDigest.getInstance("MD5"); @@ -193,7 +177,6 @@ public abstract class Credential implements Serializable } } - /* ------------------------------------------------------------ */ public static String digest(String password) { try From 042f325f1cd6e7891d72c7e668f5947b5457dc02 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 16 May 2017 10:41:08 +0200 Subject: [PATCH 2/2] Fixes #1556 - A timing channel in Password.java. --- .../jaspi/modules/DigestAuthModule.java | 3 +- .../authentication/DigestAuthenticator.java | 2 +- .../jetty/util/security/Credential.java | 57 ++++++++++++++----- .../eclipse/jetty/util/security/Password.java | 25 ++++---- 4 files changed, 57 insertions(+), 30 deletions(-) diff --git a/jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/modules/DigestAuthModule.java b/jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/modules/DigestAuthModule.java index afe3bcf1adc..54ad4f062a1 100644 --- a/jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/modules/DigestAuthModule.java +++ b/jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/modules/DigestAuthModule.java @@ -341,7 +341,7 @@ public class DigestAuthModule extends BaseAuthModule byte[] digest = md.digest(); // check digest - return (TypeUtil.toString(digest, 16).equalsIgnoreCase(response)); + return stringEquals(TypeUtil.toString(digest, 16).toLowerCase(), response == null ? null : response.toLowerCase()); } catch (Exception e) { @@ -356,6 +356,5 @@ public class DigestAuthModule extends BaseAuthModule { return username + "," + response; } - } } diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/DigestAuthenticator.java b/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/DigestAuthenticator.java index a982f56aa6b..45055670119 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/DigestAuthenticator.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/DigestAuthenticator.java @@ -401,7 +401,7 @@ public class DigestAuthenticator extends LoginAuthenticator byte[] digest = md.digest(); // check digest - return (TypeUtil.toString(digest, 16).equalsIgnoreCase(response)); + return stringEquals(TypeUtil.toString(digest, 16).toLowerCase(), response == null ? null : response.toLowerCase()); } catch (Exception e) { diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java b/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java index 9b855faf092..77692d35c4b 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java @@ -71,6 +71,44 @@ public abstract class Credential implements Serializable return new Password(credential); } + /** + *

Utility method that replaces String.equals() to avoid timing attacks.

+ * + * @param s1 the first string to compare + * @param s2 the second string to compare + * @return whether the two strings are equal + */ + protected static boolean stringEquals(String s1, String s2) + { + if (s1 == s2) + return true; + if (s1 == null || s2 == null || s1.length() != s2.length()) + return false; + boolean result = false; + for (int i = 0; i < s1.length(); i++) + result |= s1.charAt(i) == s2.charAt(i); + return result; + } + + /** + *

Utility method that replaces Arrays.equals() to avoid timing attacks.

+ * + * @param b1 the first byte array to compare + * @param b2 the second byte array to compare + * @return whether the two byte arrays are equal + */ + protected static boolean byteEquals(byte[] b1, byte[] b2) + { + if (b1 == b2) + return true; + if (b1 == null || b2 == null || b1.length != b2.length) + return false; + boolean result = false; + for (int i = 0; i < b1.length; i++) + result |= b1[i] == b2[i]; + return result; + } + /** * Unix Crypt Credentials */ @@ -93,8 +131,7 @@ public abstract class Credential implements Serializable credentials=new String((char[])credentials); if (!(credentials instanceof String) && !(credentials instanceof Password)) LOG.warn("Can't check " + credentials.getClass() + " against CRYPT"); - String passwd = credentials.toString(); - return _cooked.equals(UnixCrypt.crypt(passwd, _cooked)); + return stringEquals(_cooked, UnixCrypt.crypt(credentials.toString(), _cooked)); } public static String crypt(String user, String pw) @@ -143,26 +180,18 @@ public abstract class Credential implements Serializable __md.update(credentials.toString().getBytes(StandardCharsets.ISO_8859_1)); digest = __md.digest(); } - if (digest == null || digest.length != _digest.length) return false; - boolean digestMismatch = false; - for (int i = 0; i < digest.length; i++) - digestMismatch |= (digest[i] != _digest[i]); - return !digestMismatch; + return byteEquals(_digest, digest); } else if (credentials instanceof MD5) { - MD5 md5 = (MD5) credentials; - if (_digest.length != md5._digest.length) return false; - boolean digestMismatch = false; - for (int i = 0; i < _digest.length; i++) - digestMismatch |= (_digest[i] != md5._digest[i]); - return !digestMismatch; + MD5 md5 = (MD5)credentials; + return byteEquals(_digest, md5._digest); } else if (credentials instanceof Credential) { // Allow credential to attempt check - i.e. this'll work // for DigestAuthModule$Digest credentials - return ((Credential) credentials).check(this); + return ((Credential)credentials).check(this); } else { diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/security/Password.java b/jetty-util/src/main/java/org/eclipse/jetty/util/security/Password.java index 81ff1b92bd0..fe7839bf4e0 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/security/Password.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/security/Password.java @@ -20,7 +20,6 @@ package org.eclipse.jetty.util.security; import java.io.IOException; import java.nio.charset.StandardCharsets; -import java.util.Arrays; import java.util.Locale; import org.eclipse.jetty.util.log.Log; @@ -96,15 +95,20 @@ public class Password extends Credential @Override public boolean check(Object credentials) { - if (this == credentials) return true; + if (this == credentials) + return true; - if (credentials instanceof Password) return credentials.equals(_pw); + if (credentials instanceof Password) + return credentials.equals(_pw); - if (credentials instanceof String) return credentials.equals(_pw); + if (credentials instanceof String) + return stringEquals(_pw, (String)credentials); - if (credentials instanceof char[]) return Arrays.equals(_pw.toCharArray(), (char[]) credentials); + if (credentials instanceof char[]) + return stringEquals(_pw, new String((char[])credentials)); - if (credentials instanceof Credential) return ((Credential) credentials).check(_pw); + if (credentials instanceof Credential) + return ((Credential)credentials).check(_pw); return false; } @@ -120,14 +124,10 @@ public class Password extends Credential return false; if (o instanceof Password) - { - Password p = (Password) o; - //noinspection StringEquality - return p._pw == _pw || (null != _pw && _pw.equals(p._pw)); - } + return stringEquals(_pw, ((Password)o)._pw); if (o instanceof String) - return o.equals(_pw); + return stringEquals(_pw, (String)o); return false; } @@ -175,7 +175,6 @@ public class Password extends Credential } return buf.toString(); - } /* ------------------------------------------------------------ */