synchronize access to json key storage in account model

3 files changed, 79 insertions, 77 deletions

diff --git a/src/main/java/de/pixart/messenger/crypto/OtrService.java b/src/main/java/de/pixart/messenger/crypto/OtrService.java
index 5a43e711d..18f9670d8 100644
--- a/src/main/java/de/pixart/messenger/crypto/OtrService.java
+++ b/src/main/java/de/pixart/messenger/crypto/OtrService.java
@@ -53,29 +53,31 @@ 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;
+            }
+        }
 	}
 
 	private void saveKey() {
diff --git a/src/main/java/de/pixart/messenger/entities/Account.java b/src/main/java/de/pixart/messenger/entities/Account.java
index abce5d151..eca0733cf 100644
--- a/src/main/java/de/pixart/messenger/entities/Account.java
+++ b/src/main/java/de/pixart/messenger/entities/Account.java
@@ -202,7 +202,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;
@@ -237,11 +237,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,19 +393,32 @@ 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) {
-		try {
-			this.keys.put(keyName, keyValue);
-			return true;
-		} catch (final JSONException e) {
-			return false;
+    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 setPrivateKeyAlias(String alias) {
+    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;
+            }
+        }
+    }
+
+    public boolean setPrivateKeyAlias(String alias) {
 		return setKey("private_key_alias", alias);
 	}
 
@@ -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;
-		}
-		return true;
+        synchronized (this.keys) {
+            return keys.remove(KEY_PGP_SIGNATURE) != null;
+        }
 	}
 
 	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/de/pixart/messenger/xmpp/XmppConnection.java b/src/main/java/de/pixart/messenger/xmpp/XmppConnection.java
index 05b691390..5f42339f7 100644
--- a/src/main/java/de/pixart/messenger/xmpp/XmppConnection.java
+++ b/src/main/java/de/pixart/messenger/xmpp/XmppConnection.java
@@ -874,18 +874,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());