Skip to content

Commit

Permalink
Fixes #1556 - A timing channel in Password.java.
Browse files Browse the repository at this point in the history
  • Loading branch information
sbordet committed May 16, 2017
1 parent b1b94d8 commit 042f325
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 30 deletions.
Expand Up @@ -341,7 +341,7 @@ public boolean check(Object credentials)
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)
{
Expand All @@ -356,6 +356,5 @@ public String toString()
{
return username + "," + response;
}

}
}
Expand Up @@ -401,7 +401,7 @@ public boolean check(Object credentials)
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)
{
Expand Down
Expand Up @@ -71,6 +71,44 @@ public static Credential getCredential(String credential)
return new Password(credential);
}

/**
* <p>Utility method that replaces String.equals() to avoid timing attacks.</p>
*
* @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;
}

/**
* <p>Utility method that replaces Arrays.equals() to avoid timing attacks.</p>
*
* @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
*/
Expand All @@ -93,8 +131,7 @@ public boolean check(Object credentials)
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)
Expand Down Expand Up @@ -143,26 +180,18 @@ public boolean check(Object credentials)
__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
{
Expand Down
Expand Up @@ -20,7 +20,6 @@

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Locale;

import org.eclipse.jetty.util.log.Log;
Expand Down Expand Up @@ -96,15 +95,20 @@ public String toStarString()
@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;
}
Expand All @@ -120,14 +124,10 @@ public boolean equals(Object o)
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;
}
Expand Down Expand Up @@ -175,7 +175,6 @@ public static String obfuscate(String s)

}
return buf.toString();

}

/* ------------------------------------------------------------ */
Expand Down

0 comments on commit 042f325

Please sign in to comment.