synchronize access to json key storage in account model

src/main/java/eu/siacs/conversations/crypto/OtrService.java

@@ -53,10 +53,11 @@ 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;
 		}
+		synchronized (keys) {
 			try {
 				BigInteger x = new BigInteger(keys.getString("otr_x"), 16);
 				BigInteger y = new BigInteger(keys.getString("otr_y"), 16);
@@ -77,6 +78,7 @@ public class OtrService extends OtrCryptoEngineImpl implements OtrEngineHost {
 				return null;
 			}
 		}
+	}
 
 	private void saveKey() {
 		PublicKey publicKey = keyPair.getPublic();

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);
+			tmp = new JSONObject(keys);
-		} catch (final JSONException ignored) {
+		} catch(JSONException e) {
-			this.keys = new JSONObject();
+			tmp = new JSONObject();
 		}
+		this.keys = tmp;
 		this.avatar = avatar;
 		this.displayName = displayName;
 		this.hostname = hostname;
@@ -391,10 +393,22 @@ public class Account extends AbstractEntity {
 	}
 
 	public String getKey(final String name) {
+		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) {
+		synchronized (this.keys) {
 			try {
 				this.keys.put(keyName, keyValue);
 				return true;
@@ -402,6 +416,7 @@ public class Account extends AbstractEntity {
 				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);
+		synchronized (this.keys) {
 			values.put(KEYS, this.keys.toString());
+		}
 		values.put(ROSTERVERSION, rosterVersion);
 		values.put(AVATAR, avatar);
 		values.put(DISPLAY_NAME, displayName);
@@ -496,36 +513,21 @@ public class Account extends AbstractEntity {
 	}
 
 	public String getPgpSignature() {
-		try {
+		return getKey(KEY_PGP_SIGNATURE);
-			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;
-		}
 	}
 
 	public boolean setPgpSignature(String signature) {
-		try {
+		return setKey(KEY_PGP_SIGNATURE, signature);
-			keys.put(KEY_PGP_SIGNATURE, signature);
-		} catch (JSONException e) {
-			return false;
-		}
-		return true;
 	}
 
 	public boolean unsetPgpSignature() {
-		try {
+		synchronized (this.keys) {
-			keys.put(KEY_PGP_SIGNATURE, JSONObject.NULL);
+			return keys.remove(KEY_PGP_SIGNATURE) != null;
-		} catch (JSONException e) {
-			return false;
 		}
-		return true;
 	}
 
 	public long getPgpId() {
+		synchronized (this.keys) {
 			if (keys.has(KEY_PGP_ID)) {
 				try {
 					return keys.getLong(KEY_PGP_ID);
@@ -536,8 +538,10 @@ public class Account extends AbstractEntity {
 				return 0;
 			}
 		}
+	}
 
 	public boolean setPgpSignId(long pgpID) {
+		synchronized (this.keys) {
 			try {
 				keys.put(KEY_PGP_ID, pgpID);
 			} catch (JSONException e) {
@@ -545,6 +549,7 @@ public class Account extends AbstractEntity {
 			}
 			return true;
 		}
+	}
 
 	public Roster getRoster() {
 		return this.roster;

src/main/java/eu/siacs/conversations/xmpp/XmppConnection.java

@@ -852,19 +852,14 @@ public class XmppConnection implements Runnable {
 			saslMechanism = new Anonymous(tagWriter, account, mXmppConnectionService.getRNG());
 		}
 		if (saslMechanism != null) {
-			final JSONObject keys = account.getKeys();
+			final int pinnedMechanism = account.getKeyAsInt(Account.PINNED_MECHANISM_KEY, -1);
-			try {
+			if (pinnedMechanism > saslMechanism.getPriority()) {
-				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) +
+						") than pinned priority (" + pinnedMechanism +
 						"). Possible downgrade attack?");
 				throw new SecurityException();
 			}
-			} catch (final JSONException e) {
-				Log.d(Config.LOGTAG, "Parse error while checking pinned auth mechanism");
-			}
 			Log.d(Config.LOGTAG, account.getJid().toString() + ": Authenticating with " + saslMechanism.getMechanism());
 			auth.setAttribute("mechanism", saslMechanism.getMechanism());
 			if (!saslMechanism.getClientFirstMessage().isEmpty()) {