From 7fab97d26c37c1d26fbdb177cf2f60e2ab3d9128 Mon Sep 17 00:00:00 2001 From: Nico Alt Date: Tue, 2 Feb 2021 12:00:00 +0000 Subject: [PATCH 1/3] Be more specific about errors when adding pending contact Following the docs at https://code.briarproject.org/briar/briar/-/blob/beta-1.2.14/bramble-api/src/main/java/org/briarproject/bramble/api/contact/ContactManager.java#L110 Fixes #1825 --- briar-headless/README.md | 56 +++++++++++++- .../headless/contact/ContactControllerImpl.kt | 29 ++++++- .../ContactControllerIntegrationTest.kt | 39 ++++++++++ .../headless/contact/ContactControllerTest.kt | 77 ++++++++++++++++++- 4 files changed, 195 insertions(+), 6 deletions(-) diff --git a/briar-headless/README.md b/briar-headless/README.md index b608e5436..d8be1ecf6 100644 --- a/briar-headless/README.md +++ b/briar-headless/README.md @@ -105,7 +105,7 @@ The link and the alias should be posted as a JSON object: } ``` -This starts the process of adding the contact. +Adding a pending contact starts the process of adding the contact. Until it is completed, a pending contact is returned as JSON: ```json @@ -116,6 +116,60 @@ Until it is completed, a pending contact is returned as JSON: } ``` +Possible errors when adding a pending contact are: + +#### 400: Pending contact's handshake public key is invalid + +```json +{ + "error": "INVALID_PUBLIC_KEY" +} +``` + +#### 403: A contact with the same handshake public key already exists + +This error may be caused by someone attacking the user with the goal +of discovering the contacts of the user. + +In the Android client, upon encountering this issue a message dialog +is shown that asks whether the contact and the just added pending contact +are the same person. If that's the case, a message is shown that the +contact already exists and the pending contact isn't added. +If that's not the case and they are two different persons, the Android +client +[shows the following message](https://code.briarproject.org/briar/briar/-/blob/beta-1.2.14/briar-android/src/main/res/values/strings.xml#L271) +when this happens: +> [Alice] and [Bob] sent you the same link. +> +> One of them may be trying to discover who your contacts are. +> +> Don't tell them you received the same link from someone else. + +```json +{ + "error": "CONTACT_EXISTS" +} +``` + +#### 403: A pending contact with the same handshake public key already exists + +This error, too, may be caused by someone attacking the user with the goal +of discovering the contacts of the user. + +Just like above, upon encountering this issue a message dialog is shown in +the Android client that asks whether the contact and the just added pending +contact are the same person. If that's the case, the pending contact gets +updated. If that's not the case and they are two different persons, the +Android client shows the same message as above, warning the user about the +possible attack. + +```json +{ + "error": "PENDING_EXISTS" +} +``` +----------- + Before users can send messages to contacts, they become pending contacts. In this state Briar still needs to do some work in the background (e.g. spinning up a dedicated hidden service and letting the contact connect to it). diff --git a/briar-headless/src/main/java/org/briarproject/briar/headless/contact/ContactControllerImpl.kt b/briar-headless/src/main/java/org/briarproject/briar/headless/contact/ContactControllerImpl.kt index 6815f2c52..11216ecab 100644 --- a/briar-headless/src/main/java/org/briarproject/briar/headless/contact/ContactControllerImpl.kt +++ b/briar-headless/src/main/java/org/briarproject/briar/headless/contact/ContactControllerImpl.kt @@ -3,6 +3,8 @@ package org.briarproject.briar.headless.contact import com.fasterxml.jackson.databind.ObjectMapper import io.javalin.http.BadRequestResponse import io.javalin.http.Context +import io.javalin.http.ForbiddenResponse +import io.javalin.http.HttpResponseException import io.javalin.http.NotFoundResponse import org.briarproject.bramble.api.connection.ConnectionRegistry import org.briarproject.bramble.api.contact.ContactManager @@ -12,8 +14,10 @@ import org.briarproject.bramble.api.contact.event.ContactAddedEvent import org.briarproject.bramble.api.contact.event.PendingContactAddedEvent import org.briarproject.bramble.api.contact.event.PendingContactRemovedEvent import org.briarproject.bramble.api.contact.event.PendingContactStateChangedEvent +import org.briarproject.bramble.api.db.ContactExistsException import org.briarproject.bramble.api.db.NoSuchContactException import org.briarproject.bramble.api.db.NoSuchPendingContactException +import org.briarproject.bramble.api.db.PendingContactExistsException import org.briarproject.bramble.api.event.Event import org.briarproject.bramble.api.event.EventListener import org.briarproject.bramble.api.identity.AuthorConstants.MAX_AUTHOR_NAME_LENGTH @@ -25,8 +29,11 @@ import org.briarproject.briar.headless.event.WebSocketController import org.briarproject.briar.headless.getContactIdFromPathParam import org.briarproject.briar.headless.getFromJson import org.briarproject.briar.headless.json.JsonDict +import org.eclipse.jetty.http.HttpStatus.BAD_REQUEST_400 +import org.eclipse.jetty.http.HttpStatus.FORBIDDEN_403 import org.spongycastle.util.encoders.Base64 import org.spongycastle.util.encoders.DecoderException +import java.security.GeneralSecurityException import javax.annotation.concurrent.Immutable import javax.inject.Inject import javax.inject.Singleton @@ -91,9 +98,27 @@ constructor( override fun addPendingContact(ctx: Context): Context { val link = ctx.getFromJson(objectMapper, "link") val alias = ctx.getFromJson(objectMapper, "alias") - if (!LINK_REGEX.matcher(link).find()) throw BadRequestResponse("Invalid Link") + if (!LINK_REGEX.matcher(link).find()) { + ctx.status(BAD_REQUEST_400) + val details = mapOf("error" to "INVALID_LINK") + return ctx.json(details) + } checkAliasLength(alias) - val pendingContact = contactManager.addPendingContact(link, alias) + val pendingContact = try { + contactManager.addPendingContact(link, alias) + } catch (e: GeneralSecurityException) { + ctx.status(BAD_REQUEST_400) + val details = mapOf("error" to "INVALID_PUBLIC_KEY") + return ctx.json(details) + } catch (e: ContactExistsException) { + ctx.status(FORBIDDEN_403) + val details = mapOf("error" to "CONTACT_EXISTS") + return ctx.json(details) + } catch (e: PendingContactExistsException) { + ctx.status(FORBIDDEN_403) + val details = mapOf("error" to "PENDING_EXISTS") + return ctx.json(details) + } return ctx.json(pendingContact.output()) } diff --git a/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerIntegrationTest.kt b/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerIntegrationTest.kt index 2515eba10..22446ee0b 100644 --- a/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerIntegrationTest.kt +++ b/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerIntegrationTest.kt @@ -98,6 +98,45 @@ class ContactControllerIntegrationTest: IntegrationTest() { assertEquals(401, response.statusCode) } + @Test + fun `adding a pending contact with invalid link`() { + val alias = "AliasFoo" + val json = """{ + "link": "briar://invalid", + "alias": "$alias" + }""" + val response = post("$url/contacts/add/pending", json) + assertEquals(400, response.statusCode) + assertEquals("INVALID_LINK", response.jsonObject.getString("error")) + } + + @Test + fun `adding a pending contact with invalid public key`() { + val alias = "AliasFoo" + val json = """{ + "link": "briar://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "alias": "$alias" + }""" + val response = post("$url/contacts/add/pending", json) + assertEquals(400, response.statusCode) + assertEquals("INVALID_PUBLIC_KEY", response.jsonObject.getString("error")) + } + + @Test + fun `adding a pending contact that already exists as pending contact`() { + val alias = "AliasFoo" + val json = """{ + "link": "${getRealHandshakeLink(crypto)}", + "alias": "$alias" + }""" + var response = post("$url/contacts/add/pending", json) + assertEquals(200, response.statusCode) + + response = post("$url/contacts/add/pending", json) + assertEquals(403, response.statusCode) + assertEquals("PENDING_EXISTS", response.jsonObject.getString("error")) + } + @Test fun `removing a pending contact needs authentication token`() { val response = deleteWithWrongToken("$url/contacts/add/pending") diff --git a/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerTest.kt b/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerTest.kt index 7a8476b1e..52afebd76 100644 --- a/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerTest.kt +++ b/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerTest.kt @@ -1,6 +1,8 @@ package org.briarproject.briar.headless.contact import io.javalin.http.BadRequestResponse +import io.javalin.http.ForbiddenResponse +import io.javalin.http.HttpResponseException import io.javalin.http.NotFoundResponse import io.javalin.plugin.json.JavalinJson.toJson import io.mockk.Runs @@ -8,6 +10,7 @@ import io.mockk.every import io.mockk.just import io.mockk.mockkStatic import io.mockk.runs +import io.mockk.verify import org.briarproject.bramble.api.Pair import org.briarproject.bramble.api.contact.Contact import org.briarproject.bramble.api.contact.ContactId @@ -18,8 +21,10 @@ import org.briarproject.bramble.api.contact.event.ContactAddedEvent import org.briarproject.bramble.api.contact.event.PendingContactAddedEvent import org.briarproject.bramble.api.contact.event.PendingContactRemovedEvent import org.briarproject.bramble.api.contact.event.PendingContactStateChangedEvent +import org.briarproject.bramble.api.db.ContactExistsException import org.briarproject.bramble.api.db.NoSuchContactException import org.briarproject.bramble.api.db.NoSuchPendingContactException +import org.briarproject.bramble.api.db.PendingContactExistsException import org.briarproject.bramble.api.identity.AuthorConstants.MAX_AUTHOR_NAME_LENGTH import org.briarproject.bramble.api.plugin.event.ContactConnectedEvent import org.briarproject.bramble.api.plugin.event.ContactDisconnectedEvent @@ -30,9 +35,11 @@ import org.briarproject.bramble.util.StringUtils.getRandomString import org.briarproject.briar.headless.ControllerTest import org.briarproject.briar.headless.getFromJson import org.briarproject.briar.headless.json.JsonDict +import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertNotNull import org.junit.jupiter.api.Assertions.assertThrows import org.junit.jupiter.api.Test +import java.security.GeneralSecurityException import kotlin.random.Random internal class ContactControllerTest : ControllerTest() { @@ -96,9 +103,10 @@ internal class ContactControllerTest : ControllerTest() { "alias": "$alias" }""" every { ctx.body() } returns body - assertThrows(BadRequestResponse::class.java) { - controller.addPendingContact(ctx) - } + every { ctx.status(400) } returns ctx + every { ctx.json(mapOf("error" to "INVALID_LINK")) } returns ctx + controller.addPendingContact(ctx) + verify { ctx.status(400) } } @Test @@ -139,6 +147,69 @@ internal class ContactControllerTest : ControllerTest() { } } + @Test + fun testAddPendingContactPublicKeyInvalid() { + val link = "briar://adnsyffpsenoc3yzlhr24aegfq2pwan7kkselocill2choov6sbhs" + val alias = "Alias123" + val body = """{ + "link": "$link", + "alias": "$alias" + }""" + every { ctx.body() } returns body + every { ctx.status(400) } returns ctx + every { + contactManager.addPendingContact( + link, + alias + ) + } throws GeneralSecurityException() + every { ctx.json(mapOf("error" to "INVALID_PUBLIC_KEY")) } returns ctx + controller.addPendingContact(ctx) + verify { ctx.status(400) } + } + + @Test + fun testAddPendingContactSameContactKey() { + val link = "briar://adnsyffpsenoc3yzlhr24aegfq2pwan7kkselocill2choov6sbhs" + val alias = "Alias123" + val body = """{ + "link": "$link", + "alias": "$alias" + }""" + every { ctx.body() } returns body + every { ctx.status(403) } returns ctx + every { + contactManager.addPendingContact( + link, + alias + ) + } throws ContactExistsException(null, null) + every { ctx.json(mapOf("error" to "CONTACT_EXISTS")) } returns ctx + controller.addPendingContact(ctx) + verify { ctx.status(403) } + } + + @Test + fun testAddPendingContactSamePendingContactKey() { + val link = "briar://adnsyffpsenoc3yzlhr24aegfq2pwan7kkselocill2choov6sbhs" + val alias = "Alias123" + val body = """{ + "link": "$link", + "alias": "$alias" + }""" + every { ctx.body() } returns body + every { ctx.status(403) } returns ctx + every { + contactManager.addPendingContact( + link, + alias + ) + } throws PendingContactExistsException(null) + every { ctx.json(mapOf("error" to "PENDING_EXISTS")) } returns ctx + controller.addPendingContact(ctx) + verify { ctx.status(403) } + } + @Test fun testListPendingContacts() { every { contactManager.pendingContacts } returns listOf( From d095ba0b151a4a48f71f447d1bce15c27101a090 Mon Sep 17 00:00:00 2001 From: Nico Alt Date: Sat, 13 Feb 2021 12:00:00 +0000 Subject: [PATCH 2/3] Include name/alias of already existing (pending) contact in error --- briar-headless/README.md | 14 ++++++++++-- .../headless/contact/ContactControllerImpl.kt | 6 +++-- .../ContactControllerIntegrationTest.kt | 1 + .../headless/contact/ContactControllerTest.kt | 22 +++++++++++++++---- 4 files changed, 35 insertions(+), 8 deletions(-) diff --git a/briar-headless/README.md b/briar-headless/README.md index d8be1ecf6..812b7b2b5 100644 --- a/briar-headless/README.md +++ b/briar-headless/README.md @@ -118,6 +118,14 @@ Until it is completed, a pending contact is returned as JSON: Possible errors when adding a pending contact are: +#### 400: Pending contact's link is invalid + +```json +{ + "error": "INVALID_LINK" +} +``` + #### 400: Pending contact's handshake public key is invalid ```json @@ -147,7 +155,8 @@ when this happens: ```json { - "error": "CONTACT_EXISTS" + "error": "CONTACT_EXISTS", + "remoteAuthorName": "Bob" } ``` @@ -165,7 +174,8 @@ possible attack. ```json { - "error": "PENDING_EXISTS" + "error": "PENDING_EXISTS", + "pendingContactAlias": "Alice" } ``` ----------- diff --git a/briar-headless/src/main/java/org/briarproject/briar/headless/contact/ContactControllerImpl.kt b/briar-headless/src/main/java/org/briarproject/briar/headless/contact/ContactControllerImpl.kt index 11216ecab..8a7328252 100644 --- a/briar-headless/src/main/java/org/briarproject/briar/headless/contact/ContactControllerImpl.kt +++ b/briar-headless/src/main/java/org/briarproject/briar/headless/contact/ContactControllerImpl.kt @@ -112,11 +112,13 @@ constructor( return ctx.json(details) } catch (e: ContactExistsException) { ctx.status(FORBIDDEN_403) - val details = mapOf("error" to "CONTACT_EXISTS") + val details = + mapOf("error" to "CONTACT_EXISTS", "remoteAuthorName" to e.remoteAuthor.name) return ctx.json(details) } catch (e: PendingContactExistsException) { ctx.status(FORBIDDEN_403) - val details = mapOf("error" to "PENDING_EXISTS") + val details = + mapOf("error" to "PENDING_EXISTS", "pendingContactAlias" to e.pendingContact.alias) return ctx.json(details) } return ctx.json(pendingContact.output()) diff --git a/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerIntegrationTest.kt b/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerIntegrationTest.kt index 22446ee0b..65a85c1fb 100644 --- a/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerIntegrationTest.kt +++ b/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerIntegrationTest.kt @@ -135,6 +135,7 @@ class ContactControllerIntegrationTest: IntegrationTest() { response = post("$url/contacts/add/pending", json) assertEquals(403, response.statusCode) assertEquals("PENDING_EXISTS", response.jsonObject.getString("error")) + assertEquals(alias, response.jsonObject.getString("pendingContactAlias")) } @Test diff --git a/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerTest.kt b/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerTest.kt index 52afebd76..4476f5a17 100644 --- a/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerTest.kt +++ b/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerTest.kt @@ -183,8 +183,15 @@ internal class ContactControllerTest : ControllerTest() { link, alias ) - } throws ContactExistsException(null, null) - every { ctx.json(mapOf("error" to "CONTACT_EXISTS")) } returns ctx + } throws ContactExistsException(null, author) + every { + ctx.json( + mapOf( + "error" to "CONTACT_EXISTS", + "remoteAuthorName" to author.name + ) + ) + } returns ctx controller.addPendingContact(ctx) verify { ctx.status(403) } } @@ -204,8 +211,15 @@ internal class ContactControllerTest : ControllerTest() { link, alias ) - } throws PendingContactExistsException(null) - every { ctx.json(mapOf("error" to "PENDING_EXISTS")) } returns ctx + } throws PendingContactExistsException(pendingContact) + every { + ctx.json( + mapOf( + "error" to "PENDING_EXISTS", + "pendingContactAlias" to pendingContact.alias + ) + ) + } returns ctx controller.addPendingContact(ctx) verify { ctx.status(403) } } From 3e7e37f5f67dd44cae7270e800ff1ed749a0c1e3 Mon Sep 17 00:00:00 2001 From: Nico Alt Date: Fri, 19 Feb 2021 12:00:00 +0000 Subject: [PATCH 3/3] Include pending contact id in error response --- briar-headless/README.md | 1 + .../briar/headless/contact/ContactControllerImpl.kt | 7 +++++-- .../headless/contact/ContactControllerIntegrationTest.kt | 3 +++ .../briar/headless/contact/ContactControllerTest.kt | 1 + 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/briar-headless/README.md b/briar-headless/README.md index 812b7b2b5..3b37bd571 100644 --- a/briar-headless/README.md +++ b/briar-headless/README.md @@ -175,6 +175,7 @@ possible attack. ```json { "error": "PENDING_EXISTS", + "pendingContactId": "jsTgWcsEQ2g9rnomeK1g/hmO8M1Ix6ZIGWAjgBtlS9U=", "pendingContactAlias": "Alice" } ``` diff --git a/briar-headless/src/main/java/org/briarproject/briar/headless/contact/ContactControllerImpl.kt b/briar-headless/src/main/java/org/briarproject/briar/headless/contact/ContactControllerImpl.kt index 8a7328252..168520e56 100644 --- a/briar-headless/src/main/java/org/briarproject/briar/headless/contact/ContactControllerImpl.kt +++ b/briar-headless/src/main/java/org/briarproject/briar/headless/contact/ContactControllerImpl.kt @@ -117,8 +117,11 @@ constructor( return ctx.json(details) } catch (e: PendingContactExistsException) { ctx.status(FORBIDDEN_403) - val details = - mapOf("error" to "PENDING_EXISTS", "pendingContactAlias" to e.pendingContact.alias) + val details = mapOf( + "error" to "PENDING_EXISTS", + "pendingContactId" to e.pendingContact.id.bytes, + "pendingContactAlias" to e.pendingContact.alias + ) return ctx.json(details) } return ctx.json(pendingContact.output()) diff --git a/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerIntegrationTest.kt b/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerIntegrationTest.kt index 65a85c1fb..ee88dfce1 100644 --- a/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerIntegrationTest.kt +++ b/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerIntegrationTest.kt @@ -132,9 +132,12 @@ class ContactControllerIntegrationTest: IntegrationTest() { var response = post("$url/contacts/add/pending", json) assertEquals(200, response.statusCode) + val pendingContactId = response.jsonObject.getString("pendingContactId") + response = post("$url/contacts/add/pending", json) assertEquals(403, response.statusCode) assertEquals("PENDING_EXISTS", response.jsonObject.getString("error")) + assertEquals(pendingContactId, response.jsonObject.getString("pendingContactId")) assertEquals(alias, response.jsonObject.getString("pendingContactAlias")) } diff --git a/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerTest.kt b/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerTest.kt index 4476f5a17..1f4c75cca 100644 --- a/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerTest.kt +++ b/briar-headless/src/test/java/org/briarproject/briar/headless/contact/ContactControllerTest.kt @@ -216,6 +216,7 @@ internal class ContactControllerTest : ControllerTest() { ctx.json( mapOf( "error" to "PENDING_EXISTS", + "pendingContactId" to pendingContact.id.bytes, "pendingContactAlias" to pendingContact.alias ) )