From 847877f9d2954130a73860118cb7b6fe073aafe8 Mon Sep 17 00:00:00 2001 From: Sam Whited Date: Sat, 15 Nov 2014 09:42:40 -0500 Subject: [PATCH] Add auth method pinning --- .../conversations/crypto/sasl/DigestMd5.java | 8 +++- .../conversations/crypto/sasl/Plain.java | 8 +++- .../crypto/sasl/SaslMechanism.java | 9 +++++ .../conversations/crypto/sasl/ScramSha1.java | 8 +++- .../siacs/conversations/entities/Account.java | 2 + .../conversations/xmpp/XmppConnection.java | 39 ++++++++++++------- 6 files changed, 58 insertions(+), 16 deletions(-) diff --git a/src/main/java/eu/siacs/conversations/crypto/sasl/DigestMd5.java b/src/main/java/eu/siacs/conversations/crypto/sasl/DigestMd5.java index b56d2a466..850cacc2a 100644 --- a/src/main/java/eu/siacs/conversations/crypto/sasl/DigestMd5.java +++ b/src/main/java/eu/siacs/conversations/crypto/sasl/DigestMd5.java @@ -17,7 +17,13 @@ public class DigestMd5 extends SaslMechanism { super(tagWriter, account, rng); } - public static String getMechanism() { + @Override + public int getPriority() { + return 10; + } + + @Override + public String getMechanism() { return "DIGEST-MD5"; } diff --git a/src/main/java/eu/siacs/conversations/crypto/sasl/Plain.java b/src/main/java/eu/siacs/conversations/crypto/sasl/Plain.java index f7e7ee8ae..c7dedc5e4 100644 --- a/src/main/java/eu/siacs/conversations/crypto/sasl/Plain.java +++ b/src/main/java/eu/siacs/conversations/crypto/sasl/Plain.java @@ -12,7 +12,13 @@ public class Plain extends SaslMechanism { super(tagWriter, account, null); } - public static String getMechanism() { + @Override + public int getPriority() { + return 0; + } + + @Override + public String getMechanism() { return "PLAIN"; } diff --git a/src/main/java/eu/siacs/conversations/crypto/sasl/SaslMechanism.java b/src/main/java/eu/siacs/conversations/crypto/sasl/SaslMechanism.java index 7dd5e99c3..14d8b944b 100644 --- a/src/main/java/eu/siacs/conversations/crypto/sasl/SaslMechanism.java +++ b/src/main/java/eu/siacs/conversations/crypto/sasl/SaslMechanism.java @@ -44,6 +44,15 @@ public abstract class SaslMechanism { this.rng = rng; } + /** + * The priority is used to pin the authentication mechanism. If authentication fails, it MAY be retried with another + * mechanism of the same priority, but MUST NOT be tried with a mechanism of lower priority (to prevent downgrade + * attacks). + * @return An arbitrary int representing the priority + */ + public abstract int getPriority(); + + public abstract String getMechanism(); public String getClientFirstMessage() { return ""; } diff --git a/src/main/java/eu/siacs/conversations/crypto/sasl/ScramSha1.java b/src/main/java/eu/siacs/conversations/crypto/sasl/ScramSha1.java index 2073de2d8..f3589fa28 100644 --- a/src/main/java/eu/siacs/conversations/crypto/sasl/ScramSha1.java +++ b/src/main/java/eu/siacs/conversations/crypto/sasl/ScramSha1.java @@ -43,7 +43,13 @@ public class ScramSha1 extends SaslMechanism { clientFirstMessageBare = ""; } - public static String getMechanism() { + @Override + public int getPriority() { + return 20; + } + + @Override + public String getMechanism() { return "SCRAM-SHA-1"; } diff --git a/src/main/java/eu/siacs/conversations/entities/Account.java b/src/main/java/eu/siacs/conversations/entities/Account.java index 8b6a7bd72..fd580649c 100644 --- a/src/main/java/eu/siacs/conversations/entities/Account.java +++ b/src/main/java/eu/siacs/conversations/entities/Account.java @@ -34,6 +34,8 @@ public class Account extends AbstractEntity { public static final String KEYS = "keys"; public static final String AVATAR = "avatar"; + public static final String PINNED_MECHANISM_KEY = "pinned_mechanism"; + public static final int OPTION_USETLS = 0; public static final int OPTION_DISABLED = 1; public static final int OPTION_REGISTER = 2; diff --git a/src/main/java/eu/siacs/conversations/xmpp/XmppConnection.java b/src/main/java/eu/siacs/conversations/xmpp/XmppConnection.java index c34a08a84..24e42c7ba 100644 --- a/src/main/java/eu/siacs/conversations/xmpp/XmppConnection.java +++ b/src/main/java/eu/siacs/conversations/xmpp/XmppConnection.java @@ -12,6 +12,8 @@ import android.util.Log; import android.util.SparseArray; import org.apache.http.conn.ssl.StrictHostnameVerifier; +import org.json.JSONException; +import org.json.JSONObject; import org.xmlpull.v1.XmlPullParserException; import java.io.IOException; @@ -291,6 +293,8 @@ public class XmppConnection implements Runnable { Log.e(Config.LOGTAG, String.valueOf(e)); } Log.d(Config.LOGTAG, account.getJid().toBareJid().toString() + ": logged in"); + account.setKey(Account.PINNED_MECHANISM_KEY, + String.valueOf(saslMechanism.getPriority())); tagReader.reset(); sendStartStream(); processStream(tagReader.readTag()); @@ -629,23 +633,32 @@ public class XmppConnection implements Runnable { .findChild("mechanisms")); final Element auth = new Element("auth"); auth.setAttribute("xmlns", "urn:ietf:params:xml:ns:xmpp-sasl"); - if (mechanisms.contains(ScramSha1.getMechanism())) { + if (mechanisms.contains("SCRAM-SHA-1")) { saslMechanism = new ScramSha1(tagWriter, account, mXmppConnectionService.getRNG()); - Log.d(Config.LOGTAG, "Authenticating with " + ScramSha1.getMechanism()); - auth.setAttribute("mechanism", ScramSha1.getMechanism()); - } else if (mechanisms.contains(DigestMd5.getMechanism())) { - Log.d(Config.LOGTAG, "Authenticating with " + DigestMd5.getMechanism()); + } else if (mechanisms.contains("DIGEST-MD5")) { saslMechanism = new DigestMd5(tagWriter, account, mXmppConnectionService.getRNG()); - auth.setAttribute("mechanism", DigestMd5.getMechanism()); - } else if (mechanisms.contains(Plain.getMechanism())) { - Log.d(Config.LOGTAG, "Authenticating with " + Plain.getMechanism()); + } else if (mechanisms.contains("PLAIN")) { saslMechanism = new Plain(tagWriter, account); - auth.setAttribute("mechanism", Plain.getMechanism()); } - if (!saslMechanism.getClientFirstMessage().isEmpty()) { - auth.setContent(saslMechanism.getClientFirstMessage()); - } - tagWriter.writeElement(auth); + final JSONObject keys = account.getKeys(); + try { + if (keys.has(Account.PINNED_MECHANISM_KEY) && + keys.getInt(Account.PINNED_MECHANISM_KEY) > saslMechanism.getPriority() ) { + Log.e(Config.LOGTAG, "Auth failed. Authentication mechanism " + saslMechanism.getMechanism() + + " has lower priority (" + String.valueOf(saslMechanism.getPriority()) + + ") than pinned priority (" + keys.getInt(Account.PINNED_MECHANISM_KEY) + + "). Possible downgrade attack?"); + disconnect(true); + } + } catch (final JSONException e) { + Log.d(Config.LOGTAG, "Parse error while checking pinned auth mechanism"); + } + Log.d(Config.LOGTAG, "Authenticating with " + saslMechanism.getMechanism()); + auth.setAttribute("mechanism", saslMechanism.getMechanism()); + if (!saslMechanism.getClientFirstMessage().isEmpty()) { + auth.setContent(saslMechanism.getClientFirstMessage()); + } + tagWriter.writeElement(auth); } else if (this.streamFeatures.hasChild("sm", "urn:xmpp:sm:" + smVersion) && streamId != null) {