From 44f5a9db1ea581d56d758fb3e60a8f0924e338ae Mon Sep 17 00:00:00 2001
From: Torsten Grote <t@grobox.de>
Date: Fri, 27 Apr 2018 11:04:08 -0300
Subject: [PATCH] Address last review comments
---
.../IntroduceeProtocolEngine.java | 1 -
.../IntroducerProtocolEngine.java | 7 +++-
.../introduction/IntroductionCrypto.java | 3 ++
.../introduction/IntroductionCryptoImpl.java | 5 +--
.../introduction/IntroductionManagerImpl.java | 7 +++-
.../introduction/IntroductionValidator.java | 2 +-
.../IntroductionIntegrationTest.java | 39 +++++++++++++++++++
.../IntroductionValidatorTest.java | 23 +++++++++++
8 files changed, 79 insertions(+), 8 deletions(-)
diff --git a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroduceeProtocolEngine.java b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroduceeProtocolEngine.java
index 43f243a015..322e46a6b6 100644
--- a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroduceeProtocolEngine.java
+++ b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroduceeProtocolEngine.java
@@ -391,7 +391,6 @@ class IntroduceeProtocolEngine
mac = crypto.authMac(ourMacKey, s, localAuthor.getId());
signature = crypto.sign(ourMacKey, localAuthor.getPrivateKey());
} catch (GeneralSecurityException e) {
- // TODO
if (LOG.isLoggable(WARNING))
LOG.log(WARNING, e.toString(), e);
return abort(txn, s);
diff --git a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroducerProtocolEngine.java b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroducerProtocolEngine.java
index b923baac83..ed19fe651a 100644
--- a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroducerProtocolEngine.java
+++ b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroducerProtocolEngine.java
@@ -343,6 +343,12 @@ class IntroducerProtocolEngine
// The dependency, if any, must be the last remote message
if (isInvalidDependency(s, m.getGroupId(), m.getPreviousMessageId()))
return abort(txn, s);
+ // The message must be expected in the current state
+ boolean senderIsAlice = senderIsAlice(s, m);
+ if (senderIsAlice && s.getState() != B_DECLINED)
+ return abort(txn, s);
+ else if (!senderIsAlice && s.getState() != A_DECLINED)
+ return abort(txn, s);
// Mark the response visible in the UI
markMessageVisibleInUi(txn, m.getMessageId());
@@ -350,7 +356,6 @@ class IntroducerProtocolEngine
messageTracker
.trackMessage(txn, m.getGroupId(), m.getTimestamp(), false);
- boolean senderIsAlice = senderIsAlice(s, m);
Introducee introduceeA, introduceeB;
if (senderIsAlice) {
introduceeA = new Introducee(s.getIntroduceeA(), m.getMessageId());
diff --git a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionCrypto.java b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionCrypto.java
index cc5c9ab1a2..37f7aa10f6 100644
--- a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionCrypto.java
+++ b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionCrypto.java
@@ -19,6 +19,9 @@ interface IntroductionCrypto {
/**
* Returns true if the local author is alice
+ *
+ * Alice is the Author whose unique ID has the lower ID,
+ * comparing the IDs as byte strings.
*/
boolean isAlice(AuthorId local, AuthorId remote);
diff --git a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionCryptoImpl.java b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionCryptoImpl.java
index db24fda6b2..f9d53323b0 100644
--- a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionCryptoImpl.java
+++ b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionCryptoImpl.java
@@ -1,6 +1,5 @@
package org.briarproject.briar.introduction;
-import org.briarproject.bramble.api.Bytes;
import org.briarproject.bramble.api.FormatException;
import org.briarproject.bramble.api.client.ClientHelper;
import org.briarproject.bramble.api.crypto.CryptoComponent;
@@ -68,9 +67,7 @@ class IntroductionCryptoImpl implements IntroductionCrypto {
@Override
public boolean isAlice(AuthorId local, AuthorId remote) {
- byte[] a = local.getBytes();
- byte[] b = remote.getBytes();
- return Bytes.COMPARATOR.compare(new Bytes(a), new Bytes(b)) < 0;
+ return local.compareTo(remote) < 0;
}
@Override
diff --git a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java
index 89d26c73fa..efb857a0f5 100644
--- a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java
+++ b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java
@@ -353,7 +353,12 @@ class IntroductionManagerImpl extends ConversationClientImpl
try {
// Look up the session
StoredSession ss = getSession(txn, sessionId);
- if (ss == null) throw new IllegalArgumentException();
+ if (ss == null) {
+ // Actions from the UI may be based on stale information.
+ // The contact might just have been deleted, for example.
+ // Throwing a DbException here aborts gracefully.
+ throw new DbException();
+ }
// Parse the session
Contact contact = db.getContact(txn, contactId);
GroupId contactGroupId = getContactGroup(contact).getId();
diff --git a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionValidator.java b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionValidator.java
index 362d7e247a..929c8bddf9 100644
--- a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionValidator.java
+++ b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionValidator.java
@@ -155,7 +155,7 @@ class IntroductionValidator extends BdfMessageValidator {
byte[] sessionIdBytes = body.getRaw(1);
checkLength(sessionIdBytes, UniqueId.LENGTH);
- byte[] previousMessageId = body.getOptionalRaw(2);
+ byte[] previousMessageId = body.getRaw(2);
checkLength(previousMessageId, UniqueId.LENGTH);
byte[] mac = body.getOptionalRaw(3);
diff --git a/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionIntegrationTest.java b/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionIntegrationTest.java
index b692492f42..df0d46b881 100644
--- a/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionIntegrationTest.java
+++ b/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionIntegrationTest.java
@@ -63,6 +63,7 @@ import static org.briarproject.briar.introduction.IntroductionConstants.SESSION_
import static org.briarproject.briar.introduction.IntroductionConstants.SESSION_KEY_SESSION_ID;
import static org.briarproject.briar.introduction.MessageType.ACCEPT;
import static org.briarproject.briar.introduction.MessageType.AUTH;
+import static org.briarproject.briar.introduction.MessageType.DECLINE;
import static org.briarproject.briar.test.BriarTestUtils.assertGroupCount;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
@@ -681,6 +682,41 @@ public class IntroductionIntegrationTest
assertTrue(listener0.aborted);
}
+ /**
+ * One introducee sends an DECLINE and then another DECLINE message.
+ * The introducer should notice this and ABORT the session.
+ */
+ @Test
+ public void testDoubleDecline() throws Exception {
+ addListeners(false, true);
+
+ // make the introduction
+ long time = clock.currentTimeMillis();
+ introductionManager0
+ .makeIntroduction(contact1From0, contact2From0, null, time);
+
+ // sync REQUEST to introducee1
+ sync0To1(1, true);
+
+ // save DECLINE from introducee1
+ DeclineMessage m = (DeclineMessage) getMessageFor(c1.getClientHelper(),
+ contact0From1, DECLINE);
+
+ // sync DECLINE back to introducer
+ sync1To0(1, true);
+
+ // fake a second DECLINE message also from introducee1
+ Message msg = c1.getMessageEncoder()
+ .encodeDeclineMessage(m.getGroupId(), m.getTimestamp() + 1,
+ m.getMessageId(), m.getSessionId());
+ c1.getClientHelper().addLocalMessage(msg, new BdfDictionary(), true);
+
+ // sync fake DECLINE back to introducer
+ sync1To0(1, true);
+
+ assertTrue(listener0.aborted);
+ }
+
/**
* One introducee sends two AUTH messages.
* The introducer should notice this and ABORT the session.
@@ -1093,6 +1129,9 @@ public class IntroductionIntegrationTest
if (type == ACCEPT) {
//noinspection ConstantConditions
return c0.getMessageParser().parseAcceptMessage(m, body);
+ } else if (type == DECLINE) {
+ //noinspection ConstantConditions
+ return c0.getMessageParser().parseDeclineMessage(m, body);
} else if (type == AUTH) {
//noinspection ConstantConditions
return c0.getMessageParser().parseAuthMessage(m, body);
diff --git a/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionValidatorTest.java b/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionValidatorTest.java
index b1ad9caa9b..4f52aa4de9 100644
--- a/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionValidatorTest.java
+++ b/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionValidatorTest.java
@@ -268,6 +268,21 @@ public class IntroductionValidatorTest extends ValidatorTestCase {
validator.validateMessage(message, group, body);
}
+ @Test(expected = FormatException.class)
+ public void testRejectsInvalidPreviousMsgIdForAuth() throws Exception {
+ BdfList body = BdfList.of(AUTH.getValue(), sessionId.getBytes(),
+ 1, getRandomBytes(MAC_BYTES),
+ signature);
+ validator.validateMessage(message, group, body);
+ }
+
+ @Test(expected = FormatException.class)
+ public void testRejectsPreviousMsgIdNullForAuth() throws Exception {
+ BdfList body = BdfList.of(AUTH.getValue(), sessionId.getBytes(), null,
+ getRandomBytes(MAC_BYTES), signature);
+ validator.validateMessage(message, group, body);
+ }
+
@Test(expected = FormatException.class)
public void testRejectsTooShortMacForAuth() throws Exception {
BdfList body = BdfList.of(AUTH.getValue(), sessionId.getBytes(),
@@ -358,6 +373,14 @@ public class IntroductionValidatorTest extends ValidatorTestCase {
validator.validateMessage(message, group, body);
}
+ @Test(expected = FormatException.class)
+ public void testRejectsPreviousMsgIdNullForActivate() throws Exception {
+ BdfList body =
+ BdfList.of(ACTIVATE.getValue(), sessionId.getBytes(), null,
+ mac);
+ validator.validateMessage(message, group, body);
+ }
+
@Test(expected = FormatException.class)
public void testRejectsInvalidMacForActivate() throws Exception {
BdfList body = BdfList.of(ACTIVATE.getValue(), sessionId.getBytes(),
--
GitLab