From 7fab97d26c37c1d26fbdb177cf2f60e2ab3d9128 Mon Sep 17 00:00:00 2001 From: Nico Alt Date: Tue, 2 Feb 2021 12:00:00 +0000 Subject: [PATCH] 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(