CORDA-759: Enforce key checks on identity de-anonymisation (corda#1993)

Previously when de-anonymising a Party instance, the name of the Party was used rather than
the key, meaning a Party could be constructed with a random nonsense key and any name, and be treated as corresponding to the well known identity. This is not a security hole in itself as
in any real scenario a party shouldn't be trusted without having been registered, it creates
a significant risk of a security hole depending on how trusted the anonymous identity is, and
the returned identity is considered.

docs/source/changelog.rst

@@ -11,6 +11,9 @@ UNRELEASED
 
 * ``CordaRPCOps`` implementation now checks permissions for any function invocation, rather than just when starting flows.
 
+* ``wellKnownPartyFromAnonymous()`` now always resolve the key to a ``Party``, then the party to the well known party.
+  Previously if it was passed a ``Party`` it would use its name as-is without verifying the key matched that name.
+
 * ``OpaqueBytes.bytes`` now returns a clone of its underlying ``ByteArray``, and has been redeclared as ``final``.
   This is a minor change to the public API, but is required to ensure that classes like ``SecureHash`` are immutable.
 

finance/src/test/kotlin/net/corda/finance/contracts/asset/CashTests.kt

@@ -54,8 +54,7 @@ class CashTests {
     lateinit var database: CordaPersistence
     private lateinit var vaultStatesUnconsumed: List<StateAndRef<Cash.State>>
 
-    private lateinit var OUR_IDENTITY_1: AbstractParty
-    private lateinit var OUR_IDENTITY_AND_CERT: PartyAndCertificate
+    private lateinit var ourIdentity: AbstractParty
     private lateinit var miniCorpAnonymised: AnonymousParty
     private val CHARLIE_ANONYMISED = CHARLIE_IDENTITY.party.anonymise()
 
@@ -65,28 +64,33 @@ class CashTests {
     fun setUp() {
         LogHelper.setLevel(NodeVaultService::class)
         megaCorpServices = MockServices(listOf("net.corda.finance.contracts.asset"), MEGA_CORP.name, MEGA_CORP_KEY)
+        miniCorpServices = MockServices(listOf("net.corda.finance.contracts.asset"), MINI_CORP.name, MINI_CORP_KEY)
+        val notaryServices = MockServices(listOf("net.corda.finance.contracts.asset"), DUMMY_NOTARY.name, DUMMY_NOTARY_KEY)
         val databaseAndServices = makeTestDatabaseAndMockServices(
                 cordappPackages = listOf("net.corda.finance.contracts.asset"),
                 initialIdentityName = CordaX500Name(organisation = "Me", locality = "London", country = "GB"),
                 keys = listOf(generateKeyPair()))
         database = databaseAndServices.first
-        miniCorpServices = MockServices(listOf("net.corda.finance.contracts.asset"), MINI_CORP.name, MINI_CORP_KEY)
         ourServices = databaseAndServices.second
-        OUR_IDENTITY_AND_CERT = ourServices.myInfo.singleIdentityAndCert()
-        OUR_IDENTITY_1 = ourServices.myInfo.singleIdentity()
+
+        // Set up and register identities
+        ourIdentity = ourServices.myInfo.singleIdentity()
+        miniCorpAnonymised = miniCorpServices.myInfo.singleIdentityAndCert().party.anonymise()
+        (miniCorpServices.myInfo.legalIdentitiesAndCerts + megaCorpServices.myInfo.legalIdentitiesAndCerts + notaryServices.myInfo.legalIdentitiesAndCerts).forEach { identity ->
+            ourServices.identityService.verifyAndRegisterIdentity(identity)
+        }
 
         // Create some cash. Any attempt to spend >$500 will require multiple issuers to be involved.
-        database.transaction {
+                database.transaction {
             ourServices.fillWithSomeTestCash(howMuch = 100.DOLLARS, atLeastThisManyStates = 1, atMostThisManyStates = 1,
-                    ownedBy = OUR_IDENTITY_1, issuedBy = MEGA_CORP.ref(1), issuerServices = megaCorpServices)
+                    owner = ourIdentity, issuedBy = MEGA_CORP.ref(1), issuerServices = megaCorpServices)
             ourServices.fillWithSomeTestCash(howMuch = 400.DOLLARS, atLeastThisManyStates = 1, atMostThisManyStates = 1,
-                    ownedBy = OUR_IDENTITY_1, issuedBy = MEGA_CORP.ref(1), issuerServices = megaCorpServices)
+                    owner = ourIdentity, issuedBy = MEGA_CORP.ref(1), issuerServices = megaCorpServices)
             ourServices.fillWithSomeTestCash(howMuch = 80.DOLLARS, atLeastThisManyStates = 1, atMostThisManyStates = 1,
-                    ownedBy = OUR_IDENTITY_1, issuedBy = MINI_CORP.ref(1), issuerServices = miniCorpServices)
+                    owner = ourIdentity, issuedBy = MINI_CORP.ref(1), issuerServices = miniCorpServices)
             ourServices.fillWithSomeTestCash(howMuch = 80.SWISS_FRANCS, atLeastThisManyStates = 1, atMostThisManyStates = 1,
-                    ownedBy = OUR_IDENTITY_1, issuedBy = MINI_CORP.ref(1), issuerServices = miniCorpServices)
+                    owner = ourIdentity, issuedBy = MINI_CORP.ref(1), issuerServices = miniCorpServices)
         }
-        miniCorpAnonymised = miniCorpServices.myInfo.singleIdentityAndCert().party.anonymise()
         database.transaction {
             vaultStatesUnconsumed = ourServices.vaultService.queryBy<Cash.State>().states
         }
@@ -495,7 +499,7 @@ class CashTests {
 
     private fun makeCash(amount: Amount<Currency>, issuer: AbstractParty, depositRef: Byte = 1) =
             StateAndRef(
-                    TransactionState(Cash.State(amount `issued by` issuer.ref(depositRef), OUR_IDENTITY_1), Cash.PROGRAM_ID, DUMMY_NOTARY),
+                    TransactionState(Cash.State(amount `issued by` issuer.ref(depositRef), ourIdentity), Cash.PROGRAM_ID, DUMMY_NOTARY),
                     StateRef(SecureHash.randomSHA256(), Random().nextInt(32))
             )
 
@@ -509,12 +513,14 @@ class CashTests {
         return tx.toWireTransaction(serviceHub)
     }
 
-    private fun makeSpend(amount: Amount<Currency>, dest: AbstractParty): WireTransaction {
+    private fun makeSpend(services: ServiceHub, amount: Amount<Currency>, dest: AbstractParty): WireTransaction {
+        val ourIdentity = services.myInfo.singleIdentityAndCert()
+        val changeIdentity = services.keyManagementService.freshKeyAndCert(ourIdentity, false)
         val tx = TransactionBuilder(DUMMY_NOTARY)
         database.transaction {
-            Cash.generateSpend(ourServices, tx, amount, OUR_IDENTITY_AND_CERT, dest)
+            Cash.generateSpend(services, tx, amount, changeIdentity, dest)
         }
-        return tx.toWireTransaction(miniCorpServices)
+        return tx.toWireTransaction(services)
     }
 
     /**
@@ -588,30 +594,31 @@ class CashTests {
     fun generateExitWithEmptyVault() {
         assertFailsWith<IllegalArgumentException> {
             val tx = TransactionBuilder(DUMMY_NOTARY)
-            Cash().generateExit(tx, Amount(100, Issued(CHARLIE.ref(1), GBP)), emptyList(), OUR_IDENTITY_1)
+            Cash().generateExit(tx, Amount(100, Issued(CHARLIE.ref(1), GBP)), emptyList(), ourIdentity)
         }
     }
 
     @Test
     fun generateSimpleDirectSpend() {
         val wtx =
                 database.transaction {
-                    makeSpend(100.DOLLARS, miniCorpAnonymised)
+                    makeSpend(ourServices, 100.DOLLARS, miniCorpAnonymised)
                 }
         database.transaction {
             val vaultState = vaultStatesUnconsumed.elementAt(0)
             assertEquals(vaultState.ref, wtx.inputs[0])
             assertEquals(vaultState.state.data.copy(owner = miniCorpAnonymised), wtx.getOutput(0))
-            assertEquals(OUR_IDENTITY_1.owningKey, wtx.commands.single { it.value is Cash.Commands.Move }.signers[0])
+            assertEquals(ourIdentity.owningKey, wtx.commands.single { it.value is Cash.Commands.Move }.signers[0])
         }
     }
 
     @Test
     fun generateSimpleSpendWithParties() {
+        val changeIdentity = ourServices.keyManagementService.freshKeyAndCert(ourServices.myInfo.singleIdentityAndCert(), false)
         database.transaction {
 
             val tx = TransactionBuilder(DUMMY_NOTARY)
-            Cash.generateSpend(ourServices, tx, 80.DOLLARS, OUR_IDENTITY_AND_CERT, ALICE, setOf(MINI_CORP))
+            Cash.generateSpend(ourServices, tx, 80.DOLLARS, changeIdentity, ALICE, setOf(MINI_CORP))
 
             assertEquals(vaultStatesUnconsumed.elementAt(2).ref, tx.inputStates()[0])
         }
@@ -621,7 +628,7 @@ class CashTests {
     fun generateSimpleSpendWithChange() {
         val wtx =
                 database.transaction {
-                    makeSpend(10.DOLLARS, miniCorpAnonymised)
+                    makeSpend(ourServices, 10.DOLLARS, miniCorpAnonymised)
                 }
         database.transaction {
             val vaultState = vaultStatesUnconsumed.elementAt(0)
@@ -638,31 +645,31 @@ class CashTests {
             assertEquals(vaultState.ref, wtx.inputs[0])
             assertEquals(vaultState.state.data.copy(owner = miniCorpAnonymised, amount = 10.DOLLARS `issued by` defaultIssuer), wtx.outputs[0].data)
             assertEquals(vaultState.state.data.copy(amount = changeAmount, owner = changeOwner), wtx.outputs[1].data)
-            assertEquals(OUR_IDENTITY_1.owningKey, wtx.commands.single { it.value is Cash.Commands.Move }.signers[0])
+            assertEquals(ourIdentity.owningKey, wtx.commands.single { it.value is Cash.Commands.Move }.signers[0])
         }
     }
 
     @Test
     fun generateSpendWithTwoInputs() {
         val wtx =
                 database.transaction {
-                    makeSpend(500.DOLLARS, miniCorpAnonymised)
+                    makeSpend(ourServices, 500.DOLLARS, miniCorpAnonymised)
                 }
         database.transaction {
             val vaultState0 = vaultStatesUnconsumed.elementAt(0)
             val vaultState1 = vaultStatesUnconsumed.elementAt(1)
             assertEquals(vaultState0.ref, wtx.inputs[0])
             assertEquals(vaultState1.ref, wtx.inputs[1])
             assertEquals(vaultState0.state.data.copy(owner = miniCorpAnonymised, amount = 500.DOLLARS `issued by` defaultIssuer), wtx.getOutput(0))
-            assertEquals(OUR_IDENTITY_1.owningKey, wtx.commands.single { it.value is Cash.Commands.Move }.signers[0])
+            assertEquals(ourIdentity.owningKey, wtx.commands.single { it.value is Cash.Commands.Move }.signers[0])
         }
     }
 
     @Test
     fun generateSpendMixedDeposits() {
         val wtx =
                 database.transaction {
-                    val wtx = makeSpend(580.DOLLARS, miniCorpAnonymised)
+                    val wtx = makeSpend(ourServices, 580.DOLLARS, miniCorpAnonymised)
                     assertEquals(3, wtx.inputs.size)
                     wtx
                 }
@@ -675,7 +682,7 @@ class CashTests {
             assertEquals(vaultState2.ref, wtx.inputs[2])
             assertEquals(vaultState0.state.data.copy(owner = miniCorpAnonymised, amount = 500.DOLLARS `issued by` defaultIssuer), wtx.outputs[1].data)
             assertEquals(vaultState2.state.data.copy(owner = miniCorpAnonymised), wtx.outputs[0].data)
-            assertEquals(OUR_IDENTITY_1.owningKey, wtx.commands.single { it.value is Cash.Commands.Move }.signers[0])
+            assertEquals(ourIdentity.owningKey, wtx.commands.single { it.value is Cash.Commands.Move }.signers[0])
         }
     }
 
@@ -684,12 +691,12 @@ class CashTests {
         database.transaction {
 
             val e: InsufficientBalanceException = assertFailsWith("balance") {
-                makeSpend(1000.DOLLARS, miniCorpAnonymised)
+                makeSpend(ourServices, 1000.DOLLARS, miniCorpAnonymised)
             }
             assertEquals((1000 - 580).DOLLARS, e.amountMissing)
 
             assertFailsWith(InsufficientBalanceException::class) {
-                makeSpend(81.SWISS_FRANCS, miniCorpAnonymised)
+                makeSpend(ourServices, 81.SWISS_FRANCS, miniCorpAnonymised)
             }
         }
     }
@@ -821,7 +828,7 @@ class CashTests {
     fun multiSpend() {
         val tx = TransactionBuilder(DUMMY_NOTARY)
         database.transaction {
-            val changeIdentity = ourServices.keyManagementService.freshKeyAndCert(OUR_IDENTITY_AND_CERT, false)
+            val changeIdentity = ourServices.keyManagementService.freshKeyAndCert(ourServices.myInfo.singleIdentityAndCert(), false)
             val payments = listOf(
                     PartyAndAmount(miniCorpAnonymised, 400.DOLLARS),
                     PartyAndAmount(CHARLIE_ANONYMISED, 150.DOLLARS)

node/src/main/kotlin/net/corda/node/services/identity/InMemoryIdentityService.kt

@@ -99,14 +99,14 @@ class InMemoryIdentityService(identities: Iterable<PartyAndCertificate> = emptyS
     override fun partyFromKey(key: PublicKey): Party? = keyToParties[key]?.party
     override fun wellKnownPartyFromX500Name(name: CordaX500Name): Party? = principalToParties[name]?.party
     override fun wellKnownPartyFromAnonymous(party: AbstractParty): Party? {
-        // Expand the anonymous party to a full party (i.e. has a name) if possible
-        val candidate = party as? Party ?: keyToParties[party.owningKey]?.party
+        // The original version of this would return the party as-is if it was a Party (rather than AnonymousParty),
+        // however that means that we don't verify that we know who owns the key. As such as now enforce turning the key
+        // into a party, and from there figure out the well known party.
+        val candidate = partyFromKey(party.owningKey)
         // TODO: This should be done via the network map cache, which is the authoritative source of well known identities
-        // Look up the well known identity for that name
         return if (candidate != null) {
-            // If we have a well known identity by that name, use it in preference to the candidate. Otherwise default
-            // back to the candidate.
-            principalToParties[candidate.name]?.party ?: candidate
+            require(party.nameOrNull() == null || party.nameOrNull() == candidate.name) { "Candidate party ${candidate} does not match expected ${party}" }
+            wellKnownPartyFromX500Name(candidate.name)
         } else {
             null
         }

node/src/main/kotlin/net/corda/node/services/identity/PersistentIdentityService.kt

@@ -164,15 +164,13 @@ class PersistentIdentityService(identities: Iterable<PartyAndCertificate> = empt
     override fun partyFromKey(key: PublicKey): Party? = certificateFromKey(key)?.party
     override fun wellKnownPartyFromX500Name(name: CordaX500Name): Party? = certificateFromCordaX500Name(name)?.party
     override fun wellKnownPartyFromAnonymous(party: AbstractParty): Party? {
-        // Expand the anonymous party to a full party (i.e. has a name) if possible
-        val candidate = party as? Party ?: partyFromKey(party.owningKey)
+        // The original version of this would return the party as-is if it was a Party (rather than AnonymousParty),
+        // however that means that we don't verify that we know who owns the key. As such as now enforce turning the key
+        // into a party, and from there figure out the well known party.
+        val candidate = partyFromKey(party.owningKey)
         // TODO: This should be done via the network map cache, which is the authoritative source of well known identities
-        // Look up the well known identity for that name
         return if (candidate != null) {
-            // If we have a well known identity by that name, use it in preference to the candidate. Otherwise default
-            // back to the candidate.
-            val res = wellKnownPartyFromX500Name(candidate.name) ?: candidate
-            res
+            wellKnownPartyFromX500Name(candidate.name)
         } else {
             null
         }

node/src/main/kotlin/net/corda/node/services/persistence/AbstractPartyToX500NameAsStringConverter.kt

@@ -23,7 +23,7 @@ class AbstractPartyToX500NameAsStringConverter(identitySvc: () -> IdentityServic
         if (party != null) {
             val partyName = identityService.wellKnownPartyFromAnonymous(party)?.toString()
             if (partyName != null) return partyName
-            log.warn("Identity service unable to resolve AbstractParty: $party")
+             log.warn("Identity service unable to resolve AbstractParty: $party")
         }
         return null // non resolvable anonymous parties
     }

node/src/test/java/net/corda/node/services/vault/VaultQueryJavaTests.java

@@ -1,37 +1,57 @@
 package net.corda.node.services.vault;
 
-import com.google.common.collect.*;
-import kotlin.*;
+import com.google.common.collect.ImmutableSet;
+import kotlin.Pair;
+import kotlin.Triple;
 import net.corda.core.contracts.*;
-import net.corda.core.identity.*;
-import net.corda.core.messaging.*;
-import net.corda.core.node.services.*;
+import net.corda.core.identity.AbstractParty;
+import net.corda.core.messaging.DataFeed;
+import net.corda.core.node.services.IdentityService;
+import net.corda.core.node.services.Vault;
+import net.corda.core.node.services.VaultQueryException;
+import net.corda.core.node.services.VaultService;
 import net.corda.core.node.services.vault.*;
-import net.corda.core.node.services.vault.QueryCriteria.*;
-import net.corda.core.utilities.*;
-import net.corda.finance.contracts.*;
-import net.corda.finance.contracts.asset.*;
-import net.corda.finance.schemas.*;
-import net.corda.node.utilities.*;
-import net.corda.testing.*;
-import net.corda.testing.contracts.*;
-import net.corda.testing.node.*;
-import org.junit.*;
+import net.corda.core.node.services.vault.QueryCriteria.LinearStateQueryCriteria;
+import net.corda.core.node.services.vault.QueryCriteria.VaultCustomQueryCriteria;
+import net.corda.core.node.services.vault.QueryCriteria.VaultQueryCriteria;
+import net.corda.core.utilities.EncodingUtils;
+import net.corda.core.utilities.OpaqueBytes;
+import net.corda.finance.contracts.DealState;
+import net.corda.finance.contracts.asset.Cash;
+import net.corda.finance.contracts.asset.CashUtilities;
+import net.corda.finance.schemas.CashSchemaV1;
+import net.corda.node.utilities.CordaPersistence;
+import net.corda.node.utilities.DatabaseTransaction;
+import net.corda.testing.SerializationEnvironmentRule;
+import net.corda.testing.TestConstants;
+import net.corda.testing.contracts.DummyLinearContract;
+import net.corda.testing.contracts.VaultFiller;
+import net.corda.testing.node.MockServices;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
 import rx.Observable;
 
-import java.io.*;
-import java.lang.reflect.*;
-import java.security.*;
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.KeyPair;
+import java.security.cert.CertificateException;
 import java.util.*;
-import java.util.stream.*;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import java.util.stream.StreamSupport;
 
-import static net.corda.core.node.services.vault.QueryCriteriaUtils.*;
-import static net.corda.core.utilities.ByteArrays.*;
+import static net.corda.core.node.services.vault.QueryCriteriaUtils.DEFAULT_PAGE_NUM;
+import static net.corda.core.node.services.vault.QueryCriteriaUtils.MAX_PAGE_SIZE;
+import static net.corda.core.utilities.ByteArrays.toHexString;
 import static net.corda.finance.contracts.asset.CashUtilities.*;
 import static net.corda.testing.CoreTestUtils.*;
 import static net.corda.testing.TestConstants.*;
-import static net.corda.testing.node.MockServices.*;
-import static org.assertj.core.api.Assertions.*;
+import static net.corda.testing.node.MockServices.makeTestDatabaseAndMockServices;
+import static net.corda.testing.node.MockServices.makeTestIdentityService;
+import static org.assertj.core.api.Assertions.assertThat;
 
 public class VaultQueryJavaTests {
     @Rule
@@ -42,7 +62,7 @@ public class VaultQueryJavaTests {
     private CordaPersistence database;
 
     @Before
-    public void setUp() {
+    public void setUp() throws CertificateException, InvalidAlgorithmParameterException {
         List<String> cordappPackages = Arrays.asList("net.corda.testing.contracts", "net.corda.finance.contracts.asset", CashSchemaV1.class.getPackage().getName());
         ArrayList<KeyPair> keys = new ArrayList<>();
         keys.add(getMEGA_CORP_KEY());
@@ -54,6 +74,8 @@ public void setUp() {
         database = databaseAndServices.getFirst();
         services = databaseAndServices.getSecond();
         vaultService = services.getVaultService();
+        services.getIdentityService().verifyAndRegisterIdentity(getDUMMY_CASH_ISSUER_IDENTITY());
+        services.getIdentityService().verifyAndRegisterIdentity(getDUMMY_NOTARY_IDENTITY());
     }
 
     @After