Fixes #1556 - A timing channel in Password.java.

This commit is contained in:
Simone Bordet 2017-05-16 10:41:08 +02:00
parent b1b94d870e
commit 042f325f1c
4 changed files with 57 additions and 30 deletions

View File

@ -341,7 +341,7 @@ public class DigestAuthModule extends BaseAuthModule
byte[] digest = md.digest(); byte[] digest = md.digest();
// check 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) catch (Exception e)
{ {
@ -356,6 +356,5 @@ public class DigestAuthModule extends BaseAuthModule
{ {
return username + "," + response; return username + "," + response;
} }
} }
} }

View File

@ -401,7 +401,7 @@ public class DigestAuthenticator extends LoginAuthenticator
byte[] digest = md.digest(); byte[] digest = md.digest();
// check 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) catch (Exception e)
{ {

View File

@ -71,6 +71,44 @@ public abstract class Credential implements Serializable
return new Password(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 * Unix Crypt Credentials
*/ */
@ -93,8 +131,7 @@ public abstract class Credential implements Serializable
credentials=new String((char[])credentials); credentials=new String((char[])credentials);
if (!(credentials instanceof String) && !(credentials instanceof Password)) if (!(credentials instanceof String) && !(credentials instanceof Password))
LOG.warn("Can't check " + credentials.getClass() + " against CRYPT"); LOG.warn("Can't check " + credentials.getClass() + " against CRYPT");
String passwd = credentials.toString(); return stringEquals(_cooked, UnixCrypt.crypt(credentials.toString(), _cooked));
return _cooked.equals(UnixCrypt.crypt(passwd, _cooked));
} }
public static String crypt(String user, String pw) public static String crypt(String user, String pw)
@ -143,20 +180,12 @@ public abstract class Credential implements Serializable
__md.update(credentials.toString().getBytes(StandardCharsets.ISO_8859_1)); __md.update(credentials.toString().getBytes(StandardCharsets.ISO_8859_1));
digest = __md.digest(); digest = __md.digest();
} }
if (digest == null || digest.length != _digest.length) return false; return byteEquals(_digest, digest);
boolean digestMismatch = false;
for (int i = 0; i < digest.length; i++)
digestMismatch |= (digest[i] != _digest[i]);
return !digestMismatch;
} }
else if (credentials instanceof MD5) else if (credentials instanceof MD5)
{ {
MD5 md5 = (MD5)credentials; MD5 md5 = (MD5)credentials;
if (_digest.length != md5._digest.length) return false; return byteEquals(_digest, md5._digest);
boolean digestMismatch = false;
for (int i = 0; i < _digest.length; i++)
digestMismatch |= (_digest[i] != md5._digest[i]);
return !digestMismatch;
} }
else if (credentials instanceof Credential) else if (credentials instanceof Credential)
{ {

View File

@ -20,7 +20,6 @@ package org.eclipse.jetty.util.security;
import java.io.IOException; import java.io.IOException;
import java.nio.charset.StandardCharsets; import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Locale; import java.util.Locale;
import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Log;
@ -96,15 +95,20 @@ public class Password extends Credential
@Override @Override
public boolean check(Object credentials) 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; return false;
} }
@ -120,14 +124,10 @@ public class Password extends Credential
return false; return false;
if (o instanceof Password) if (o instanceof Password)
{ return stringEquals(_pw, ((Password)o)._pw);
Password p = (Password) o;
//noinspection StringEquality
return p._pw == _pw || (null != _pw && _pw.equals(p._pw));
}
if (o instanceof String) if (o instanceof String)
return o.equals(_pw); return stringEquals(_pw, (String)o);
return false; return false;
} }
@ -175,7 +175,6 @@ public class Password extends Credential
} }
return buf.toString(); return buf.toString();
} }
/* ------------------------------------------------------------ */ /* ------------------------------------------------------------ */