From 172d9f658e42d56d192727dae54b6744de6e76e8 Mon Sep 17 00:00:00 2001 From: Oscar Guindzberg Date: Fri, 3 May 2019 15:24:38 -0300 Subject: [PATCH] SignedWitnessService improvements --- .../src/main/java/bisq/common/crypto/Sig.java | 2 +- .../account/sign/SignedWitnessService.java | 121 ++++++--- .../account/witness/AccountAgeWitness.java | 5 +- .../sign/SignedWitnessServiceTest.java | 243 ++++++++++++++++++ 4 files changed, 330 insertions(+), 41 deletions(-) create mode 100644 core/src/test/java/bisq/core/account/sign/SignedWitnessServiceTest.java diff --git a/common/src/main/java/bisq/common/crypto/Sig.java b/common/src/main/java/bisq/common/crypto/Sig.java index 85e14838967..659ed3d6b86 100644 --- a/common/src/main/java/bisq/common/crypto/Sig.java +++ b/common/src/main/java/bisq/common/crypto/Sig.java @@ -111,7 +111,7 @@ public static boolean verify(PublicKey publicKey, byte[] data, byte[] signature) sig.update(data); return sig.verify(signature); } catch (SignatureException | InvalidKeyException | NoSuchAlgorithmException e) { - throw new CryptoException("Signature verification failed. " + e.getMessage()); + throw new CryptoException("Signature verification failed", e); } } diff --git a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java index da9a421fd66..0ce0a52d40d 100644 --- a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java +++ b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java @@ -36,24 +36,35 @@ import javax.inject.Inject; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Charsets; import java.security.PublicKey; import java.security.SignatureException; +import java.time.Instant; +import java.time.Period; +import java.time.temporal.ChronoUnit; +import java.time.temporal.TemporalUnit; + import java.util.Arrays; +import java.util.Calendar; import java.util.Date; +import java.util.GregorianCalendar; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.Stack; import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; @Slf4j public class SignedWitnessService { + public static final long CHARGEBACK_SAFETY_DAYS = 30; + private final KeyRing keyRing; private final P2PService p2PService; private final AccountAgeWitnessService accountAgeWitnessService; @@ -117,7 +128,7 @@ public List getMyWitnessAgeList(PaymentAccountPayload myPaymentAccountPayl public List getVerifiedWitnessAgeList(AccountAgeWitness accountAgeWitness) { return signedWitnessMap.values().stream() .filter(e -> Arrays.equals(e.getWitnessHash(), accountAgeWitness.getHash())) - .filter(this::verify) + .filter(this::verifySignature) .map(SignedWitness::getDate) .sorted() .collect(Collectors.toList()); @@ -137,7 +148,7 @@ public SignedWitness signAccountAgeWitness(Coin tradeAmount, AccountAgeWitness a } // Any peer can sign with DSA key - public SignedWitness sign(Coin tradeAmount, AccountAgeWitness accountAgeWitness, PublicKey peersPubKey) throws CryptoException { + public SignedWitness signAccountAgeWitness(Coin tradeAmount, AccountAgeWitness accountAgeWitness, PublicKey peersPubKey) throws CryptoException { byte[] signature = Sig.sign(keyRing.getSignatureKeyPair().getPrivate(), accountAgeWitness.getHash()); return new SignedWitness(false, accountAgeWitness.getHash(), @@ -148,15 +159,15 @@ public SignedWitness sign(Coin tradeAmount, AccountAgeWitness accountAgeWitness, tradeAmount.value); } - public boolean verify(SignedWitness signedWitness) { + public boolean verifySignature(SignedWitness signedWitness) { if (signedWitness.isSignedByArbitrator()) { - return verifyWithECKey(signedWitness); + return verifySignatureWithECKey(signedWitness); } else { - return verifyWithDSAKey(signedWitness); + return verifySignatureWithDSAKey(signedWitness); } } - private boolean verifyWithECKey(SignedWitness signedWitness) { + private boolean verifySignatureWithECKey(SignedWitness signedWitness) { try { String message = Utilities.encodeToHex(signedWitness.getWitnessHash()); String signatureBase64 = new String(signedWitness.getSignature(), Charsets.UTF_8); @@ -169,18 +180,20 @@ private boolean verifyWithECKey(SignedWitness signedWitness) { return false; } } catch (SignatureException e) { - log.warn("verify signedWitness failed. signedWitness={}", signedWitness); + log.warn("verifySignature signedWitness failed. signedWitness={}", signedWitness); + log.warn("Caused by ", e); return false; } } - private boolean verifyWithDSAKey(SignedWitness signedWitness) { + private boolean verifySignatureWithDSAKey(SignedWitness signedWitness) { try { PublicKey signaturePubKey = Sig.getPublicKeyFromBytes(signedWitness.getSignerPubKey()); Sig.verify(signaturePubKey, signedWitness.getWitnessHash(), signedWitness.getSignature()); return true; } catch (CryptoException e) { - log.warn("verify signedWitness failed. signedWitness={}", signedWitness); + log.warn("verifySignature signedWitness failed. signedWitness={}", signedWitness); + log.warn("Caused by ", e); return false; } } @@ -209,43 +222,73 @@ public Set getTrustedPeerSignedWitnessSet(AccountAgeWitness accou // We go one level up by using the signer Key to lookup for SignedWitness objects which contain the signerKey as // witnessOwnerPubKey - public Set getSignedWitnessSetBySignerPubKey(byte[] signerPubKey) { + public Set getSignedWitnessSetByOwnerPubKey(byte[] ownerPubKey, Stack excluded) { return signedWitnessMap.values().stream() - .filter(e -> Arrays.equals(e.getWitnessOwnerPubKey(), signerPubKey)) + .filter(e -> Arrays.equals(e.getWitnessOwnerPubKey(), ownerPubKey)) + .filter(e -> !excluded.contains(new P2PDataStorage.ByteArray(e.getSignerPubKey()))) .collect(Collectors.toSet()); } - //TODO pass list and remove items once processed to avoid endless loop in case of multiple sigs + /** + * Checks whether the accountAgeWitness has a valid signature from a peer/arbitrator. + * @param accountAgeWitness + * @return true if accountAgeWitness is valid, false otherwise. + */ public boolean isValidAccountAgeWitness(AccountAgeWitness accountAgeWitness) { - Set arbitratorsSignedWitnessSet = getArbitratorsSignedWitnessSet(accountAgeWitness); - if (!arbitratorsSignedWitnessSet.isEmpty()) { - // Our peer was signed by arbitrator. We only check it at least one is valid and don't need to go further. - return arbitratorsSignedWitnessSet.stream().anyMatch(this::verify); + Stack excludedPubKeys = new Stack<>(); + long now = new Date().getTime(); + Set signedWitnessSet = getSignedWitnessSet(accountAgeWitness); + for (SignedWitness signedWitness : signedWitnessSet) { + if (isValidSignedWitnessInternal(signedWitness, now, excludedPubKeys)) { + return true; + } + } + // If we have not returned in the loops or they have been empty we have not found a valid signer. + return false; + } + + /** + * Helper to isValidAccountAgeWitness(accountAgeWitness) + * @param signedWitness the signedWitness to validate + * @param childSignedWitnessDateMillis the date the child SignedWitness was signed or current time if it is a leave. + * @param excludedPubKeys stack to preventsrecursive loops + * @return true if signedWitness is valid, false otherwise. + */ + private boolean isValidSignedWitnessInternal(SignedWitness signedWitness, long childSignedWitnessDateMillis, Stack excludedPubKeys) { + if (!verifySignature(signedWitness)) { + return false; + } + if (signedWitness.isSignedByArbitrator()) { + // If signed by an arbitrator we don't have to check anything else. + return true; } else { - Set trustedPeerSignedWitnessSet = getTrustedPeerSignedWitnessSet(accountAgeWitness); - // We have some SignedWitness signed by any trusted peer and need to see if it is valid and has a - // valid chain back to the arbitrators SignedWitness. - for (SignedWitness trustedPeerSignedWitness : trustedPeerSignedWitnessSet) { - if (verify(trustedPeerSignedWitness)) { - // The signature is valid. Lets see who has signed it. - // Get set of SignedWitness objects signer was witness owner. - Set signersWitnessSet = getSignedWitnessSetBySignerPubKey(trustedPeerSignedWitness.getSignerPubKey()); - for (SignedWitness signersWitness : signersWitnessSet) { - if (verify(signersWitness)) { - Optional optionalWitness = accountAgeWitnessService.getWitnessByHash(signersWitness.getWitnessHash()); - if (optionalWitness.isPresent()) { - // Enter recursion - boolean isvalid = isValidAccountAgeWitness(optionalWitness.get()); - if (isvalid) - return true; - } - } - } + if (!verifyDate(signedWitness, childSignedWitnessDateMillis)) { + return false; + } + if (excludedPubKeys.size() >= 2000) { + // Prevent DoS attack: an attacker floods the SignedWitness db with a long chain that takes lots of time to verify.ca + return false; + } + excludedPubKeys.push(new P2PDataStorage.ByteArray(signedWitness.getSignerPubKey())); + excludedPubKeys.push(new P2PDataStorage.ByteArray(signedWitness.getWitnessOwnerPubKey())); + // Iterate over signedWitness signers + Set signerSignedWitnessSet = getSignedWitnessSetByOwnerPubKey(signedWitness.getSignerPubKey(), excludedPubKeys); + for (SignedWitness signerSignedWitness : signerSignedWitnessSet) { + if (isValidSignedWitnessInternal(signerSignedWitness, signedWitness.getDate(), excludedPubKeys)) { + return true; } } - // If we have not returned in the loops or they have been empty we have not found a valid signer. - return false; + excludedPubKeys.pop(); + excludedPubKeys.pop(); } + // If we have not returned in the loops or they have been empty we have not found a valid signer. + return false; + } + + private boolean verifyDate(SignedWitness signedWitness, long childSignedWitnessDateMillis) { + long childSignedWitnessDateMinusChargebackPeriodMillis = Instant.ofEpochMilli(childSignedWitnessDateMillis).minus(CHARGEBACK_SAFETY_DAYS, ChronoUnit.DAYS).toEpochMilli(); + long signedWitnessDateMillis = signedWitness.getDate(); + return signedWitnessDateMillis <= childSignedWitnessDateMinusChargebackPeriodMillis; } @@ -253,8 +296,8 @@ public boolean isValidAccountAgeWitness(AccountAgeWitness accountAgeWitness) { // Private /////////////////////////////////////////////////////////////////////////////////////////// - - private void addToMap(SignedWitness signedWitness) { + @VisibleForTesting + void addToMap(SignedWitness signedWitness) { signedWitnessMap.putIfAbsent(signedWitness.getHashAsByteArray(), signedWitness); } } diff --git a/core/src/main/java/bisq/core/account/witness/AccountAgeWitness.java b/core/src/main/java/bisq/core/account/witness/AccountAgeWitness.java index ae361d25539..8b46ec27f59 100644 --- a/core/src/main/java/bisq/core/account/witness/AccountAgeWitness.java +++ b/core/src/main/java/bisq/core/account/witness/AccountAgeWitness.java @@ -32,6 +32,8 @@ import com.google.protobuf.ByteString; +import com.google.common.annotations.VisibleForTesting; + import java.util.Date; import java.util.concurrent.TimeUnit; @@ -50,7 +52,8 @@ public class AccountAgeWitness implements LazyProcessedPayload, PersistableNetwo private final byte[] hash; // Ripemd160(Sha256(concatenated accountHash, signature and sigPubKey)); 20 bytes private final long date; // 8 byte - AccountAgeWitness(byte[] hash, + @VisibleForTesting + public AccountAgeWitness(byte[] hash, long date) { this.hash = hash; this.date = date; diff --git a/core/src/test/java/bisq/core/account/sign/SignedWitnessServiceTest.java b/core/src/test/java/bisq/core/account/sign/SignedWitnessServiceTest.java new file mode 100644 index 00000000000..df5319ef264 --- /dev/null +++ b/core/src/test/java/bisq/core/account/sign/SignedWitnessServiceTest.java @@ -0,0 +1,243 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.core.account.sign; + + +import bisq.core.account.witness.AccountAgeWitness; +import bisq.core.arbitration.ArbitratorManager; + +import bisq.network.p2p.storage.persistence.AppendOnlyDataStoreService; + +import bisq.common.crypto.Sig; +import bisq.common.util.Utilities; + +import org.bitcoinj.core.ECKey; + +import com.google.common.base.Charsets; + +import java.security.KeyPair; + +import java.time.Instant; +import java.time.temporal.ChronoUnit; + +import java.util.Date; + +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + + +import static org.mockito.Mockito.*; + +public class SignedWitnessServiceTest { + private SignedWitnessService service; + + @Before + public void setup() { + AppendOnlyDataStoreService appendOnlyDataStoreService = mock(AppendOnlyDataStoreService.class); + ArbitratorManager arbitratorManager = mock(ArbitratorManager.class); + when(arbitratorManager.isPublicKeyInList(any())).thenReturn(true); + service = new SignedWitnessService(null, null, null, arbitratorManager, null, appendOnlyDataStoreService); + } + + @After + public void tearDown() { + } + + @Test + public void testIsValidAccountAgeWitnessOk() throws Exception { + testIsValidAccountAgeWitness(false, false, false, false); + } + + @Test + public void testIsValidAccountAgeWitnessArbitratorSignatureProblem() throws Exception { + testIsValidAccountAgeWitness(true, false, false, false); + } + + @Test + public void testIsValidAccountAgeWitnessPeerSignatureProblem() throws Exception { + testIsValidAccountAgeWitness(false, true, false, false); + } + + @Test + public void testIsValidAccountAgeWitnessDateTooSoonProblem() throws Exception { + testIsValidAccountAgeWitness(false, false, true, false); + } + + @Test + public void testIsValidAccountAgeWitnessDateTooLateProblem() throws Exception { + testIsValidAccountAgeWitness(false, false, false, true); + } + + private void testIsValidAccountAgeWitness(boolean signature1Problem, boolean signature2Problem, boolean date3TooSoonProblem, boolean date3TooLateProblem) throws Exception { + byte[] account1DataHash = org.bitcoinj.core.Utils.sha256hash160(new byte[] {1}); + byte[] account2DataHash = org.bitcoinj.core.Utils.sha256hash160(new byte[] {2}); + byte[] account3DataHash = org.bitcoinj.core.Utils.sha256hash160(new byte[] {3}); + long account1CreationTime = getTodayMinusNDays(96); + long account2CreationTime = getTodayMinusNDays(66); + long account3CreationTime = getTodayMinusNDays(36); + AccountAgeWitness aew1 = new AccountAgeWitness(account1DataHash, account1CreationTime); + AccountAgeWitness aew2 = new AccountAgeWitness(account2DataHash, account2CreationTime); + AccountAgeWitness aew3 = new AccountAgeWitness(account3DataHash, account3CreationTime); + + ECKey arbitrator1Key = new ECKey(); + KeyPair peer1KeyPair = Sig.generateKeyPair(); + KeyPair peer2KeyPair = Sig.generateKeyPair(); + KeyPair peer3KeyPair = Sig.generateKeyPair(); + + + String account1DataHashAsHexString = Utilities.encodeToHex(account1DataHash); + String account2DataHashAsHexString = Utilities.encodeToHex(account2DataHash); + String account3DataHashAsHexString = Utilities.encodeToHex(account3DataHash); + String signature1String = arbitrator1Key.signMessage(account1DataHashAsHexString); + byte[] signature1 = signature1String.getBytes(Charsets.UTF_8); + if (signature1Problem) { + signature1 = new byte[] {1, 2, 3}; + } + byte[] signature2 = Sig.sign(peer1KeyPair.getPrivate(), account2DataHashAsHexString.getBytes(Charsets.UTF_8)); + if (signature2Problem) { + signature2 = new byte[] {1, 2, 3}; + } + byte[] signature3 = Sig.sign(peer2KeyPair.getPrivate(), account3DataHashAsHexString.getBytes(Charsets.UTF_8)); + byte[] signer1PubKey = arbitrator1Key.getPubKey(); + byte[] signer2PubKey = Sig.getPublicKeyBytes(peer1KeyPair.getPublic()); + byte[] signer3PubKey = Sig.getPublicKeyBytes(peer2KeyPair.getPublic()); + byte[] witnessOwner1PubKey = Sig.getPublicKeyBytes(peer1KeyPair.getPublic()); + byte[] witnessOwner2PubKey = Sig.getPublicKeyBytes(peer2KeyPair.getPublic()); + byte[] witnessOwner3PubKey = Sig.getPublicKeyBytes(peer3KeyPair.getPublic()); + long date1 = getTodayMinusNDays(95); + long date2 = getTodayMinusNDays(64); + long date3 = getTodayMinusNDays(33); + if (date3TooSoonProblem) { + date3 = getTodayMinusNDays(63); + } else if (date3TooLateProblem) { + date3 = getTodayMinusNDays(3); + } + long tradeAmount1 = 1000; + long tradeAmount2 = 1001; + long tradeAmount3 = 1001; + + SignedWitness sw1 = new SignedWitness(true, account1DataHash, signature1, signer1PubKey, witnessOwner1PubKey, date1, tradeAmount1); + SignedWitness sw2 = new SignedWitness(false, account2DataHash, signature2, signer2PubKey, witnessOwner2PubKey, date2, tradeAmount2); + SignedWitness sw3 = new SignedWitness(false, account3DataHash, signature3, signer3PubKey, witnessOwner3PubKey, date3, tradeAmount3); + + service.addToMap(sw1); + service.addToMap(sw2); + service.addToMap(sw3); + + Assert.assertEquals(!signature1Problem, service.isValidAccountAgeWitness(aew1)); + Assert.assertEquals(!signature1Problem && !signature2Problem, service.isValidAccountAgeWitness(aew2)); + Assert.assertEquals(!signature1Problem && !signature2Problem && !date3TooSoonProblem && !date3TooLateProblem, service.isValidAccountAgeWitness(aew3)); + } + + + @Test + public void testIsValidAccountAgeWitnessEndlessLoop() throws Exception { + byte[] account1DataHash = org.bitcoinj.core.Utils.sha256hash160(new byte[] {1}); + byte[] account2DataHash = org.bitcoinj.core.Utils.sha256hash160(new byte[] {2}); + byte[] account3DataHash = org.bitcoinj.core.Utils.sha256hash160(new byte[] {3}); + long account1CreationTime = getTodayMinusNDays(96); + long account2CreationTime = getTodayMinusNDays(66); + long account3CreationTime = getTodayMinusNDays(36); + AccountAgeWitness aew1 = new AccountAgeWitness(account1DataHash, account1CreationTime); + AccountAgeWitness aew2 = new AccountAgeWitness(account2DataHash, account2CreationTime); + AccountAgeWitness aew3 = new AccountAgeWitness(account3DataHash, account3CreationTime); + + KeyPair peer1KeyPair = Sig.generateKeyPair(); + KeyPair peer2KeyPair = Sig.generateKeyPair(); + KeyPair peer3KeyPair = Sig.generateKeyPair(); + + + String account1DataHashAsHexString = Utilities.encodeToHex(account1DataHash); + String account2DataHashAsHexString = Utilities.encodeToHex(account2DataHash); + String account3DataHashAsHexString = Utilities.encodeToHex(account3DataHash); + + byte[] signature1 = Sig.sign(peer3KeyPair.getPrivate(), account1DataHashAsHexString.getBytes(Charsets.UTF_8)); + byte[] signature2 = Sig.sign(peer1KeyPair.getPrivate(), account2DataHashAsHexString.getBytes(Charsets.UTF_8)); + byte[] signature3 = Sig.sign(peer2KeyPair.getPrivate(), account3DataHashAsHexString.getBytes(Charsets.UTF_8)); + + byte[] signer1PubKey = Sig.getPublicKeyBytes(peer3KeyPair.getPublic()); + byte[] signer2PubKey = Sig.getPublicKeyBytes(peer1KeyPair.getPublic()); + byte[] signer3PubKey = Sig.getPublicKeyBytes(peer2KeyPair.getPublic()); + byte[] witnessOwner1PubKey = Sig.getPublicKeyBytes(peer1KeyPair.getPublic()); + byte[] witnessOwner2PubKey = Sig.getPublicKeyBytes(peer2KeyPair.getPublic()); + byte[] witnessOwner3PubKey = Sig.getPublicKeyBytes(peer3KeyPair.getPublic()); + long date1 = getTodayMinusNDays(95); + long date2 = getTodayMinusNDays(64); + long date3 = getTodayMinusNDays(33); + + long tradeAmount1 = 1000; + long tradeAmount2 = 1001; + long tradeAmount3 = 1001; + + SignedWitness sw1 = new SignedWitness(false, account1DataHash, signature1, signer1PubKey, witnessOwner1PubKey, date1, tradeAmount1); + SignedWitness sw2 = new SignedWitness(false, account2DataHash, signature2, signer2PubKey, witnessOwner2PubKey, date2, tradeAmount2); + SignedWitness sw3 = new SignedWitness(false, account3DataHash, signature3, signer3PubKey, witnessOwner3PubKey, date3, tradeAmount3); + + service.addToMap(sw1); + service.addToMap(sw2); + service.addToMap(sw3); + + Assert.assertFalse(service.isValidAccountAgeWitness(aew3)); + } + + + + @Test + public void testIsValidAccountAgeWitnessLongLoop() throws Exception { + AccountAgeWitness aew = null; + KeyPair signerKeyPair = Sig.generateKeyPair(); + KeyPair signedKeyPair = Sig.generateKeyPair(); + int iterations = 1002; + for (int i = 0; i < iterations; i++) { + byte[] accountDataHash = org.bitcoinj.core.Utils.sha256hash160(String.valueOf(i).getBytes(Charsets.UTF_8)); + long accountCreationTime = getTodayMinusNDays((iterations-i) * (SignedWitnessService.CHARGEBACK_SAFETY_DAYS+1)); + aew = new AccountAgeWitness(accountDataHash, accountCreationTime); + String accountDataHashAsHexString = Utilities.encodeToHex(accountDataHash); + byte[] signature; + byte[] signerPubKey; + if (i==0) { + // use arbitrator key + ECKey arbitratorKey = new ECKey(); + signedKeyPair = Sig.generateKeyPair(); + String signature1String = arbitratorKey.signMessage(accountDataHashAsHexString); + signature = signature1String.getBytes(Charsets.UTF_8); + signerPubKey = arbitratorKey.getPubKey(); + } else { + signerKeyPair = signedKeyPair; + signedKeyPair = Sig.generateKeyPair(); + signature = Sig.sign(signedKeyPair.getPrivate(), accountDataHashAsHexString.getBytes(Charsets.UTF_8)); + signerPubKey = Sig.getPublicKeyBytes(signerKeyPair.getPublic()); + } + byte[] witnessOwnerPubKey = Sig.getPublicKeyBytes(signedKeyPair.getPublic()); + long date = getTodayMinusNDays((iterations-i) * (SignedWitnessService.CHARGEBACK_SAFETY_DAYS+1)); + long tradeAmount = 1000; + SignedWitness sw = new SignedWitness(i==0, accountDataHash, signature, signerPubKey, witnessOwnerPubKey, date, tradeAmount); + service.addToMap(sw); + } + Assert.assertFalse(service.isValidAccountAgeWitness(aew)); + } + + + private long getTodayMinusNDays(long days) { + return Instant.ofEpochMilli(new Date().getTime()).minus(days, ChronoUnit.DAYS).toEpochMilli(); + } + +} +