From 685e1422a5ce6636cbfcbb6d3efcb0ca4ad00924 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Wed, 4 May 2016 20:39:22 -0300 Subject: [PATCH] Do not allow session ID reuse and clean up sessions for introducee It was possible that a malicious introducer sends new request with the same session ID that was used previously and thus causing introducees to have multiple states for the same session ID. This commits prevents that from happening and adds an integration test for that scenario. Also if an introducee removes an introducer, all past session states will be deleted from the database. For this, a test was added as well. Closes #371 Closes #372 --- .../IntroductionIntegrationTest.java | 204 +++++++++++++++++- .../IntroductionIntegrationTestComponent.java | 11 + .../IntroductionGroupFactory.java | 2 +- .../introduction/IntroductionManagerImpl.java | 28 ++- .../introduction/IntroductionValidator.java | 2 + .../introduction/MessageSender.java | 2 +- 6 files changed, 241 insertions(+), 8 deletions(-) diff --git a/briar-android-tests/src/test/java/org/briarproject/IntroductionIntegrationTest.java b/briar-android-tests/src/test/java/org/briarproject/IntroductionIntegrationTest.java index 56502a64b..cb219c322 100644 --- a/briar-android-tests/src/test/java/org/briarproject/IntroductionIntegrationTest.java +++ b/briar-android-tests/src/test/java/org/briarproject/IntroductionIntegrationTest.java @@ -2,11 +2,17 @@ package org.briarproject; import net.jodah.concurrentunit.Waiter; +import org.briarproject.api.clients.SessionId; import org.briarproject.api.contact.Contact; import org.briarproject.api.contact.ContactId; import org.briarproject.api.contact.ContactManager; import org.briarproject.api.crypto.SecretKey; +import org.briarproject.api.data.BdfDictionary; +import org.briarproject.api.data.BdfEntry; +import org.briarproject.api.db.DatabaseComponent; import org.briarproject.api.db.DbException; +import org.briarproject.api.db.Metadata; +import org.briarproject.api.db.Transaction; import org.briarproject.api.event.Event; import org.briarproject.api.event.EventListener; import org.briarproject.api.event.IntroductionAbortedEvent; @@ -18,17 +24,21 @@ import org.briarproject.api.identity.AuthorFactory; import org.briarproject.api.identity.IdentityManager; import org.briarproject.api.identity.LocalAuthor; import org.briarproject.api.introduction.IntroductionManager; +import org.briarproject.api.introduction.IntroductionMessage; import org.briarproject.api.introduction.IntroductionRequest; -import org.briarproject.api.clients.SessionId; import org.briarproject.api.lifecycle.LifecycleManager; import org.briarproject.api.properties.TransportProperties; import org.briarproject.api.properties.TransportPropertyManager; +import org.briarproject.api.sync.Group; +import org.briarproject.api.sync.MessageId; import org.briarproject.api.sync.SyncSession; import org.briarproject.api.sync.SyncSessionFactory; import org.briarproject.api.system.Clock; import org.briarproject.contact.ContactModule; import org.briarproject.crypto.CryptoModule; +import org.briarproject.introduction.IntroductionGroupFactory; import org.briarproject.introduction.IntroductionModule; +import org.briarproject.introduction.MessageSender; import org.briarproject.lifecycle.LifecycleModule; import org.briarproject.properties.PropertiesModule; import org.briarproject.sync.SyncModule; @@ -41,7 +51,10 @@ import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.File; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; +import java.util.List; +import java.util.Map; import java.util.concurrent.TimeoutException; import java.util.logging.Logger; @@ -50,6 +63,12 @@ import javax.inject.Inject; import static org.briarproject.TestPluginsModule.MAX_LATENCY; import static org.briarproject.TestPluginsModule.TRANSPORT_ID; import static org.briarproject.api.identity.AuthorConstants.MAX_PUBLIC_KEY_LENGTH; +import static org.briarproject.api.introduction.IntroductionConstants.GROUP_ID; +import static org.briarproject.api.introduction.IntroductionConstants.NAME; +import static org.briarproject.api.introduction.IntroductionConstants.PUBLIC_KEY; +import static org.briarproject.api.introduction.IntroductionConstants.SESSION_ID; +import static org.briarproject.api.introduction.IntroductionConstants.TYPE; +import static org.briarproject.api.introduction.IntroductionConstants.TYPE_REQUEST; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -629,6 +648,189 @@ public class IntroductionIntegrationTest extends BriarTestCase { } } + @Test + public void testSessionIdReuse() throws Exception { + startLifecycles(); + try { + // Add Identities + addDefaultIdentities(); + + // Add Transport Properties + addTransportProperties(); + + // Add introducees as contacts + contactId1 = contactManager0.addContact(author1, + author0.getId(), master, clock.currentTimeMillis(), true, + true + ); + contactId2 = contactManager0.addContact(author2, + author0.getId(), master, clock.currentTimeMillis(), true, + true + ); + // Add introducer back + contactId0 = contactManager1.addContact(author0, + author1.getId(), master, clock.currentTimeMillis(), true, + true + ); + ContactId contactId02 = contactManager2.addContact(author0, + author2.getId(), master, clock.currentTimeMillis(), true, + true + ); + assertTrue(contactId0.equals(contactId02)); + + // listen to events + IntroducerListener listener0 = new IntroducerListener(); + t0.getEventBus().addListener(listener0); + IntroduceeListener listener1 = new IntroduceeListener(1, true); + t1.getEventBus().addListener(listener1); + IntroduceeListener listener2 = new IntroduceeListener(2, true); + t2.getEventBus().addListener(listener2); + + // make introduction + long time = clock.currentTimeMillis(); + Contact introducee1 = contactManager0.getContact(contactId1); + Contact introducee2 = contactManager0.getContact(contactId2); + introductionManager0 + .makeIntroduction(introducee1, introducee2, "Hi!", time); + + // sync first request message + deliverMessage(sync0, contactId0, sync1, contactId1, "0 to 1"); + eventWaiter.await(TIMEOUT, 1); + assertTrue(listener1.requestReceived); + + // get SessionId + List list = new ArrayList<>( + introductionManager1.getIntroductionMessages(contactId0)); + assertEquals(2, list.size()); + assertTrue(list.get(0) instanceof IntroductionRequest); + IntroductionRequest msg = (IntroductionRequest) list.get(0); + SessionId sessionId = msg.getSessionId(); + + // get contact group + IntroductionGroupFactory groupFactory = + t0.getIntroductionGroupFactory(); + Group group = groupFactory.createIntroductionGroup(introducee1); + + // create new message with same SessionId + BdfDictionary d = BdfDictionary.of( + new BdfEntry(TYPE, TYPE_REQUEST), + new BdfEntry(SESSION_ID, sessionId), + new BdfEntry(GROUP_ID, group.getId()), + new BdfEntry(NAME, TestUtils.getRandomString(42)), + new BdfEntry(PUBLIC_KEY, + TestUtils.getRandomBytes(MAX_PUBLIC_KEY_LENGTH)) + ); + + // reset request received state + listener1.requestReceived = false; + + // add the message to the queue + DatabaseComponent db0 = t0.getDatabaseComponent(); + MessageSender sender0 = t0.getMessageSender(); + Transaction txn = db0.startTransaction(false); + try { + sender0.sendMessage(txn, d); + txn.setComplete(); + } finally { + db0.endTransaction(txn); + } + + // actually send message + deliverMessage(sync0, contactId0, sync1, contactId1, "0 to 1"); + + // make sure it does not arrive + assertFalse(listener1.requestReceived); + } finally { + stopLifecycles(); + } + } + + @Test + public void testIntroducerRemovedCleanup() throws Exception { + startLifecycles(); + try { + // Add Identities + addDefaultIdentities(); + + // Add Transport Properties + addTransportProperties(); + + // Add introducees as contacts + contactId1 = contactManager0.addContact(author1, + author0.getId(), master, clock.currentTimeMillis(), true, + true + ); + contactId2 = contactManager0.addContact(author2, + author0.getId(), master, clock.currentTimeMillis(), true, + true + ); + // Add introducer back + contactId0 = contactManager1.addContact(author0, + author1.getId(), master, clock.currentTimeMillis(), true, + true + ); + ContactId contactId02 = contactManager2.addContact(author0, + author2.getId(), master, clock.currentTimeMillis(), true, + true + ); + assertTrue(contactId0.equals(contactId02)); + + // listen to events + IntroducerListener listener0 = new IntroducerListener(); + t0.getEventBus().addListener(listener0); + IntroduceeListener listener1 = new IntroduceeListener(1, true); + t1.getEventBus().addListener(listener1); + IntroduceeListener listener2 = new IntroduceeListener(2, true); + t2.getEventBus().addListener(listener2); + + // make introduction + long time = clock.currentTimeMillis(); + Contact introducee1 = contactManager0.getContact(contactId1); + Contact introducee2 = contactManager0.getContact(contactId2); + introductionManager0 + .makeIntroduction(introducee1, introducee2, "Hi!", time); + + // sync first request message + deliverMessage(sync0, contactId0, sync1, contactId1, "0 to 1"); + eventWaiter.await(TIMEOUT, 1); + assertTrue(listener1.requestReceived); + + // get database and local group for introducee + DatabaseComponent db1 = t1.getDatabaseComponent(); + IntroductionGroupFactory groupFactory1 = + t1.getIntroductionGroupFactory(); + Group group1 = groupFactory1.createLocalGroup(); + + // get local session state messages + Map map; + Transaction txn = db1.startTransaction(false); + try { + map = db1.getMessageMetadata(txn, group1.getId()); + txn.setComplete(); + } finally { + db1.endTransaction(txn); + } + // check that we have one session state + assertEquals(1, map.size()); + + // introducee1 removes introducer + contactManager1.removeContact(contactId0); + + // get local session state messages again + txn = db1.startTransaction(false); + try { + map = db1.getMessageMetadata(txn, group1.getId()); + txn.setComplete(); + } finally { + db1.endTransaction(txn); + } + // make sure local state got deleted + assertEquals(0, map.size()); + } finally { + stopLifecycles(); + } + } + // TODO add a test for faking responses when #256 is implemented @After diff --git a/briar-android-tests/src/test/java/org/briarproject/IntroductionIntegrationTestComponent.java b/briar-android-tests/src/test/java/org/briarproject/IntroductionIntegrationTestComponent.java index b02732756..1a0853291 100644 --- a/briar-android-tests/src/test/java/org/briarproject/IntroductionIntegrationTestComponent.java +++ b/briar-android-tests/src/test/java/org/briarproject/IntroductionIntegrationTestComponent.java @@ -1,6 +1,7 @@ package org.briarproject; import org.briarproject.api.contact.ContactManager; +import org.briarproject.api.db.DatabaseComponent; import org.briarproject.api.event.EventBus; import org.briarproject.api.identity.IdentityManager; import org.briarproject.api.introduction.IntroductionManager; @@ -14,7 +15,9 @@ import org.briarproject.data.DataModule; import org.briarproject.db.DatabaseModule; import org.briarproject.event.EventModule; import org.briarproject.identity.IdentityModule; +import org.briarproject.introduction.IntroductionGroupFactory; import org.briarproject.introduction.IntroductionModule; +import org.briarproject.introduction.MessageSender; import org.briarproject.lifecycle.LifecycleModule; import org.briarproject.properties.PropertiesModule; import org.briarproject.sync.SyncModule; @@ -74,4 +77,12 @@ public interface IntroductionIntegrationTestComponent { SyncSessionFactory getSyncSessionFactory(); + /* the following methods are only needed to manually construct messages */ + + DatabaseComponent getDatabaseComponent(); + + MessageSender getMessageSender(); + + IntroductionGroupFactory getIntroductionGroupFactory(); + } diff --git a/briar-core/src/org/briarproject/introduction/IntroductionGroupFactory.java b/briar-core/src/org/briarproject/introduction/IntroductionGroupFactory.java index a255c8114..f04437d18 100644 --- a/briar-core/src/org/briarproject/introduction/IntroductionGroupFactory.java +++ b/briar-core/src/org/briarproject/introduction/IntroductionGroupFactory.java @@ -6,7 +6,7 @@ import org.briarproject.api.sync.Group; import javax.inject.Inject; -class IntroductionGroupFactory { +public class IntroductionGroupFactory { final private PrivateGroupFactory privateGroupFactory; final private Group localGroup; diff --git a/briar-core/src/org/briarproject/introduction/IntroductionManagerImpl.java b/briar-core/src/org/briarproject/introduction/IntroductionManagerImpl.java index 97740b10a..29954399a 100644 --- a/briar-core/src/org/briarproject/introduction/IntroductionManagerImpl.java +++ b/briar-core/src/org/briarproject/introduction/IntroductionManagerImpl.java @@ -147,8 +147,13 @@ class IntroductionManagerImpl extends BdfIncomingMessageHook for (Map.Entry entry : map.entrySet()) { BdfDictionary d = entry.getValue(); long role = d.getLong(ROLE, -1L); - if (role != ROLE_INTRODUCER) continue; - if (d.getLong(CONTACT_ID_1).equals(id) || + if (role != ROLE_INTRODUCER) { + if (d.getLong(CONTACT_ID_1).equals(id)) { + // delete states if introducee removes introducer + deleteMessage(txn, entry.getKey()); + } + } + else if (d.getLong(CONTACT_ID_1).equals(id) || d.getLong(CONTACT_ID_2).equals(id)) { IntroducerProtocolState state = IntroducerProtocolState @@ -178,13 +183,19 @@ class IntroductionManagerImpl extends BdfIncomingMessageHook // Get message data and type GroupId groupId = m.getGroupId(); - message.put(GROUP_ID, groupId); long type = message.getLong(TYPE, -1L); // we are an introducee, need to initialize new state if (type == TYPE_REQUEST) { + boolean stateExists = true; + try { + getSessionState(txn, groupId, message.getRaw(SESSION_ID), false); + } catch (FormatException e) { + stateExists = false; + } BdfDictionary state; try { + if (stateExists) throw new FormatException(); state = introduceeManager.initialize(txn, groupId, message); } catch (FormatException e) { if (LOG.isLoggable(WARNING)) { @@ -450,7 +461,8 @@ class IntroductionManagerImpl extends BdfIncomingMessageHook } private BdfDictionary getSessionState(Transaction txn, GroupId groupId, - byte[] sessionId) throws DbException, FormatException { + byte[] sessionId, boolean warn) + throws DbException, FormatException { try { // See if we can find the state directly for the introducer @@ -476,7 +488,7 @@ class IntroductionManagerImpl extends BdfIncomingMessageHook if (g.equals(groupId)) return state; } } - if (LOG.isLoggable(WARNING)) { + if (warn && LOG.isLoggable(WARNING)) { LOG.warning( "No session state found for message with session ID " + Arrays.hashCode(sessionId)); @@ -485,6 +497,12 @@ class IntroductionManagerImpl extends BdfIncomingMessageHook } } + private BdfDictionary getSessionState(Transaction txn, GroupId groupId, + byte[] sessionId) throws DbException, FormatException { + + return getSessionState(txn, groupId, sessionId, true); + } + private void deleteMessage(Transaction txn, MessageId messageId) throws DbException { diff --git a/briar-core/src/org/briarproject/introduction/IntroductionValidator.java b/briar-core/src/org/briarproject/introduction/IntroductionValidator.java index 3babe437b..374a15b2e 100644 --- a/briar-core/src/org/briarproject/introduction/IntroductionValidator.java +++ b/briar-core/src/org/briarproject/introduction/IntroductionValidator.java @@ -16,6 +16,7 @@ import static org.briarproject.api.identity.AuthorConstants.MAX_AUTHOR_NAME_LENG import static org.briarproject.api.identity.AuthorConstants.MAX_PUBLIC_KEY_LENGTH; import static org.briarproject.api.introduction.IntroductionConstants.ACCEPT; import static org.briarproject.api.introduction.IntroductionConstants.E_PUBLIC_KEY; +import static org.briarproject.api.introduction.IntroductionConstants.GROUP_ID; import static org.briarproject.api.introduction.IntroductionConstants.MESSAGE_ID; import static org.briarproject.api.introduction.IntroductionConstants.MESSAGE_TIME; import static org.briarproject.api.introduction.IntroductionConstants.MSG; @@ -63,6 +64,7 @@ class IntroductionValidator extends BdfMessageValidator { d.put(TYPE, type); d.put(SESSION_ID, id); + d.put(GROUP_ID, m.getGroupId()); d.put(MESSAGE_ID, m.getId()); d.put(MESSAGE_TIME, m.getTimestamp()); return d; diff --git a/briar-core/src/org/briarproject/introduction/MessageSender.java b/briar-core/src/org/briarproject/introduction/MessageSender.java index 7152a1344..9327dc9e2 100644 --- a/briar-core/src/org/briarproject/introduction/MessageSender.java +++ b/briar-core/src/org/briarproject/introduction/MessageSender.java @@ -32,7 +32,7 @@ import static org.briarproject.api.introduction.IntroductionConstants.TYPE_ACK; import static org.briarproject.api.introduction.IntroductionConstants.TYPE_REQUEST; import static org.briarproject.api.introduction.IntroductionConstants.TYPE_RESPONSE; -class MessageSender { +public class MessageSender { final private DatabaseComponent db; final private ClientHelper clientHelper;