synchronize access to json key storage in account model

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 af11756f..3663cd3b 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 9218e524..7d60dcf7 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 boolean setKey(final String keyName, final String keyValue) {
+	public int getKeyAsInt(final String name, int defaultValue) {
+		String key = getKey(name);
 		try {
-			this.keys.put(keyName, keyValue);
-			return true;
-		} catch (final JSONException e) {
-			return false;
+			return key == null ? defaultValue : Integer.parseInt(key);
+		} catch (NumberFormatException e) {
+			return defaultValue;
+		}
+	}
+
+	public boolean setKey(final String keyName, final String keyValue) {
+		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 ab92535a..383e990d 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());