From f6cfa277414caf999a7180f921f7d040f6b0c3ff Mon Sep 17 00:00:00 2001 From: Daniel Gultsch Date: Thu, 13 Oct 2016 11:27:26 +0200 Subject: [PATCH] synchronize access to json key storage in account model --- .../conversations/crypto/OtrService.java | 40 +++++---- .../siacs/conversations/entities/Account.java | 89 ++++++++++--------- .../conversations/xmpp/XmppConnection.java | 19 ++-- 3 files changed, 75 insertions(+), 73 deletions(-) diff --git a/src/main/java/eu/siacs/conversations/crypto/OtrService.java b/src/main/java/eu/siacs/conversations/crypto/OtrService.java index af11756f6..3663cd3be 100644 --- a/src/main/java/eu/siacs/conversations/crypto/OtrService.java +++ b/src/main/java/eu/siacs/conversations/crypto/OtrService.java @@ -53,28 +53,30 @@ public class OtrService extends OtrCryptoEngineImpl implements OtrEngineHost { this.mXmppConnectionService = service; } - private KeyPair loadKey(JSONObject keys) { + private KeyPair loadKey(final JSONObject keys) { if (keys == null) { return null; } - try { - BigInteger x = new BigInteger(keys.getString("otr_x"), 16); - BigInteger y = new BigInteger(keys.getString("otr_y"), 16); - BigInteger p = new BigInteger(keys.getString("otr_p"), 16); - BigInteger q = new BigInteger(keys.getString("otr_q"), 16); - BigInteger g = new BigInteger(keys.getString("otr_g"), 16); - KeyFactory keyFactory = KeyFactory.getInstance("DSA"); - DSAPublicKeySpec pubKeySpec = new DSAPublicKeySpec(y, p, q, g); - DSAPrivateKeySpec privateKeySpec = new DSAPrivateKeySpec(x, p, q, g); - PublicKey publicKey = keyFactory.generatePublic(pubKeySpec); - PrivateKey privateKey = keyFactory.generatePrivate(privateKeySpec); - return new KeyPair(publicKey, privateKey); - } catch (JSONException e) { - return null; - } catch (NoSuchAlgorithmException e) { - return null; - } catch (InvalidKeySpecException e) { - return null; + synchronized (keys) { + try { + BigInteger x = new BigInteger(keys.getString("otr_x"), 16); + BigInteger y = new BigInteger(keys.getString("otr_y"), 16); + BigInteger p = new BigInteger(keys.getString("otr_p"), 16); + BigInteger q = new BigInteger(keys.getString("otr_q"), 16); + BigInteger g = new BigInteger(keys.getString("otr_g"), 16); + KeyFactory keyFactory = KeyFactory.getInstance("DSA"); + DSAPublicKeySpec pubKeySpec = new DSAPublicKeySpec(y, p, q, g); + DSAPrivateKeySpec privateKeySpec = new DSAPrivateKeySpec(x, p, q, g); + PublicKey publicKey = keyFactory.generatePublic(pubKeySpec); + PrivateKey privateKey = keyFactory.generatePrivate(privateKeySpec); + return new KeyPair(publicKey, privateKey); + } catch (JSONException e) { + return null; + } catch (NoSuchAlgorithmException e) { + return null; + } catch (InvalidKeySpecException e) { + return null; + } } } diff --git a/src/main/java/eu/siacs/conversations/entities/Account.java b/src/main/java/eu/siacs/conversations/entities/Account.java index 9218e5248..7d60dcf73 100644 --- a/src/main/java/eu/siacs/conversations/entities/Account.java +++ b/src/main/java/eu/siacs/conversations/entities/Account.java @@ -203,7 +203,7 @@ public class Account extends AbstractEntity { protected int options = 0; protected String rosterVersion; protected State status = State.OFFLINE; - protected JSONObject keys = new JSONObject(); + protected final JSONObject keys; protected String avatar; protected String displayName = null; protected String hostname = null; @@ -238,11 +238,13 @@ public class Account extends AbstractEntity { this.password = password; this.options = options; this.rosterVersion = rosterVersion; + JSONObject tmp; try { - this.keys = new JSONObject(keys); - } catch (final JSONException ignored) { - this.keys = new JSONObject(); + tmp = new JSONObject(keys); + } catch(JSONException e) { + tmp = new JSONObject(); } + this.keys = tmp; this.avatar = avatar; this.displayName = displayName; this.hostname = hostname; @@ -391,15 +393,28 @@ public class Account extends AbstractEntity { } public String getKey(final String name) { - return this.keys.optString(name, null); + synchronized (this.keys) { + return this.keys.optString(name, null); + } + } + + public int getKeyAsInt(final String name, int defaultValue) { + String key = getKey(name); + try { + return key == null ? defaultValue : Integer.parseInt(key); + } catch (NumberFormatException e) { + return defaultValue; + } } public boolean setKey(final String keyName, final String keyValue) { - try { - this.keys.put(keyName, keyValue); - return true; - } catch (final JSONException e) { - return false; + synchronized (this.keys) { + try { + this.keys.put(keyName, keyValue); + return true; + } catch (final JSONException e) { + return false; + } } } @@ -419,7 +434,9 @@ public class Account extends AbstractEntity { values.put(SERVER, jid.getDomainpart()); values.put(PASSWORD, password); values.put(OPTIONS, options); - values.put(KEYS, this.keys.toString()); + synchronized (this.keys) { + values.put(KEYS, this.keys.toString()); + } values.put(ROSTERVERSION, rosterVersion); values.put(AVATAR, avatar); values.put(DISPLAY_NAME, displayName); @@ -496,54 +513,42 @@ public class Account extends AbstractEntity { } public String getPgpSignature() { - try { - if (keys.has(KEY_PGP_SIGNATURE) && !"null".equals(keys.getString(KEY_PGP_SIGNATURE))) { - return keys.getString(KEY_PGP_SIGNATURE); - } else { - return null; - } - } catch (final JSONException e) { - return null; - } + return getKey(KEY_PGP_SIGNATURE); } public boolean setPgpSignature(String signature) { - try { - keys.put(KEY_PGP_SIGNATURE, signature); - } catch (JSONException e) { - return false; - } - return true; + return setKey(KEY_PGP_SIGNATURE, signature); } public boolean unsetPgpSignature() { - try { - keys.put(KEY_PGP_SIGNATURE, JSONObject.NULL); - } catch (JSONException e) { - return false; + synchronized (this.keys) { + return keys.remove(KEY_PGP_SIGNATURE) != null; } - return true; } public long getPgpId() { - if (keys.has(KEY_PGP_ID)) { - try { - return keys.getLong(KEY_PGP_ID); - } catch (JSONException e) { + synchronized (this.keys) { + if (keys.has(KEY_PGP_ID)) { + try { + return keys.getLong(KEY_PGP_ID); + } catch (JSONException e) { + return 0; + } + } else { return 0; } - } else { - return 0; } } public boolean setPgpSignId(long pgpID) { - try { - keys.put(KEY_PGP_ID, pgpID); - } catch (JSONException e) { - return false; + synchronized (this.keys) { + try { + keys.put(KEY_PGP_ID, pgpID); + } catch (JSONException e) { + return false; + } + return true; } - return true; } public Roster getRoster() { diff --git a/src/main/java/eu/siacs/conversations/xmpp/XmppConnection.java b/src/main/java/eu/siacs/conversations/xmpp/XmppConnection.java index ab92535af..383e990d2 100644 --- a/src/main/java/eu/siacs/conversations/xmpp/XmppConnection.java +++ b/src/main/java/eu/siacs/conversations/xmpp/XmppConnection.java @@ -852,18 +852,13 @@ public class XmppConnection implements Runnable { saslMechanism = new Anonymous(tagWriter, account, mXmppConnectionService.getRNG()); } if (saslMechanism != null) { - 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?"); - throw new SecurityException(); - } - } catch (final JSONException e) { - Log.d(Config.LOGTAG, "Parse error while checking pinned auth mechanism"); + final int pinnedMechanism = account.getKeyAsInt(Account.PINNED_MECHANISM_KEY, -1); + if (pinnedMechanism > saslMechanism.getPriority()) { + Log.e(Config.LOGTAG, "Auth failed. Authentication mechanism " + saslMechanism.getMechanism() + + " has lower priority (" + String.valueOf(saslMechanism.getPriority()) + + ") than pinned priority (" + pinnedMechanism + + "). Possible downgrade attack?"); + throw new SecurityException(); } Log.d(Config.LOGTAG, account.getJid().toString() + ": Authenticating with " + saslMechanism.getMechanism()); auth.setAttribute("mechanism", saslMechanism.getMechanism());