Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix remaining blackmail vulnerabilities #4789

Merged
merged 7 commits into from
Nov 19, 2020
Merged

Fix remaining blackmail vulnerabilities #4789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
79fa219
Fix missing segwit case when sanitising preparedDepositTx
stejbac Nov 12, 2020
f02d5ee
Send seller's delayedPayoutTx signature to peer ASAP
stejbac Nov 13, 2020
803035b
Add new BuyerFinalizesDelayedPayoutTx task
stejbac Nov 13, 2020
5a3b072
Withhold witnesses in buyer->seller depositTx data, until last step
stejbac Nov 13, 2020
69d1e16
Prevent takers from using non-segwit deposit tx inputs
stejbac Nov 13, 2020
3574204
Fix faulty signature check in finalizeDelayedPayoutTx
stejbac Nov 14, 2020
345426f
Add further validation checks for delayed payout tx
stejbac Nov 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 61 additions & 14 deletions core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@

public class TradeWalletService {
private static final Logger log = LoggerFactory.getLogger(TradeWalletService.class);
private static final Coin MIN_DELAYED_PAYOUT_TX_FEE = Coin.valueOf(1000);

private final WalletsSetup walletsSetup;
private final Preferences preferences;
Expand Down Expand Up @@ -569,8 +570,9 @@ private PreparedDepositTxAndMakerInputs makerCreatesDepositTx(boolean makerIsBuy
* @param buyerPubKey the public key of the buyer
* @param sellerPubKey the public key of the seller
* @throws SigningException if (one of) the taker input(s) was of an unrecognized type for signing
* @throws TransactionVerificationException if a maker input wasn't signed, their MultiSig script or contract hash
* doesn't match the taker's, or there was an unexpected problem with the final deposit tx or its signatures
* @throws TransactionVerificationException if a non-P2WH maker-as-buyer input wasn't signed, the maker's MultiSig
* script or contract hash doesn't match the taker's, or there was an unexpected problem with the final deposit tx
* or its signatures
* @throws WalletException if the taker's wallet is null or structurally inconsistent
*/
public Transaction takerSignsDepositTx(boolean takerIsSeller,
Expand Down Expand Up @@ -602,10 +604,11 @@ public Transaction takerSignsDepositTx(boolean takerIsSeller,
for (int i = 0; i < buyerInputs.size(); i++) {
TransactionInput makersInput = makersDepositTx.getInputs().get(i);
byte[] makersScriptSigProgram = makersInput.getScriptSig().getProgram();
if (makersScriptSigProgram.length == 0 && TransactionWitness.EMPTY.equals(makersInput.getWitness())) {
throw new TransactionVerificationException("Inputs from maker not signed.");
}
TransactionInput input = getTransactionInput(depositTx, makersScriptSigProgram, buyerInputs.get(i));
Script scriptPubKey = checkNotNull(input.getConnectedOutput()).getScriptPubKey();
if (makersScriptSigProgram.length == 0 && !ScriptPattern.isP2WH(scriptPubKey)) {
throw new TransactionVerificationException("Non-segwit inputs from maker not signed.");
}
if (!TransactionWitness.EMPTY.equals(makersInput.getWitness())) {
input.setWitness(makersInput.getWitness());
}
Expand Down Expand Up @@ -686,6 +689,21 @@ public void sellerAsMakerFinalizesDepositTx(Transaction myDepositTx,
}


public void sellerAddsBuyerWitnessesToDepositTx(Transaction myDepositTx,
Transaction buyersDepositTxWithWitnesses) {
int numberInputs = myDepositTx.getInputs().size();
for (int i = 0; i < numberInputs; i++) {
var txInput = myDepositTx.getInput(i);
var witnessFromBuyer = buyersDepositTxWithWitnesses.getInput(i).getWitness();

if (TransactionWitness.EMPTY.equals(txInput.getWitness()) &&
!TransactionWitness.EMPTY.equals(witnessFromBuyer)) {
txInput.setWitness(witnessFromBuyer);
}
}
}


///////////////////////////////////////////////////////////////////////////////////////////
// Delayed payout tx
///////////////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -729,12 +747,14 @@ public byte[] signDelayedPayoutTx(Transaction delayedPayoutTx,
return mySignature.encodeToDER();
}

public Transaction finalizeDelayedPayoutTx(Transaction delayedPayoutTx,
byte[] buyerPubKey,
byte[] sellerPubKey,
byte[] buyerSignature,
byte[] sellerSignature)
throws AddressFormatException, TransactionVerificationException, WalletException, SignatureDecodeException {
public Transaction finalizeUnconnectedDelayedPayoutTx(Transaction delayedPayoutTx,
byte[] buyerPubKey,
byte[] sellerPubKey,
byte[] buyerSignature,
byte[] sellerSignature,
Coin inputValue)
throws AddressFormatException, TransactionVerificationException, SignatureDecodeException {

Script redeemScript = get2of2MultiSigRedeemScript(buyerPubKey, sellerPubKey);
ECKey.ECDSASignature buyerECDSASignature = ECKey.ECDSASignature.decodeFromDER(buyerSignature);
ECKey.ECDSASignature sellerECDSASignature = ECKey.ECDSASignature.decodeFromDER(sellerSignature);
Expand All @@ -746,8 +766,26 @@ public Transaction finalizeDelayedPayoutTx(Transaction delayedPayoutTx,
input.setWitness(witness);
WalletService.printTx("finalizeDelayedPayoutTx", delayedPayoutTx);
WalletService.verifyTransaction(delayedPayoutTx);

if (checkNotNull(inputValue).isLessThan(delayedPayoutTx.getOutputSum().add(MIN_DELAYED_PAYOUT_TX_FEE))) {
throw new TransactionVerificationException("Delayed payout tx is paying less than the minimum allowed tx fee");
}
Script scriptPubKey = get2of2MultiSigOutputScript(buyerPubKey, sellerPubKey, false);
input.getScriptSig().correctlySpends(delayedPayoutTx, 0, witness, inputValue, scriptPubKey, Script.ALL_VERIFY_FLAGS);
return delayedPayoutTx;
}

public Transaction finalizeDelayedPayoutTx(Transaction delayedPayoutTx,
byte[] buyerPubKey,
byte[] sellerPubKey,
byte[] buyerSignature,
byte[] sellerSignature)
throws AddressFormatException, TransactionVerificationException, WalletException, SignatureDecodeException {

TransactionInput input = delayedPayoutTx.getInput(0);
finalizeUnconnectedDelayedPayoutTx(delayedPayoutTx, buyerPubKey, sellerPubKey, buyerSignature, sellerSignature, input.getValue());

WalletService.checkWalletConsistency(wallet);
WalletService.checkScriptSig(delayedPayoutTx, input, 0);
checkNotNull(input.getConnectedOutput(), "input.getConnectedOutput() must not be null");
input.verify(input.getConnectedOutput());
return delayedPayoutTx;
Expand Down Expand Up @@ -1185,11 +1223,20 @@ private RawTransactionInput getRawInputFromTransactionInput(@NotNull Transaction
private TransactionInput getTransactionInput(Transaction depositTx,
byte[] scriptProgram,
RawTransactionInput rawTransactionInput) {
return new TransactionInput(params, depositTx, scriptProgram, new TransactionOutPoint(params,
rawTransactionInput.index, new Transaction(params, rawTransactionInput.parentTransaction)),
return new TransactionInput(params, depositTx, scriptProgram, getConnectedOutPoint(rawTransactionInput),
Coin.valueOf(rawTransactionInput.value));
}

private TransactionOutPoint getConnectedOutPoint(RawTransactionInput rawTransactionInput) {
return new TransactionOutPoint(params, rawTransactionInput.index,
new Transaction(params, rawTransactionInput.parentTransaction));
}

public boolean isP2WH(RawTransactionInput rawTransactionInput) {
return ScriptPattern.isP2WH(
checkNotNull(getConnectedOutPoint(rawTransactionInput).getConnectedOutput()).getScriptPubKey());
}


// TODO: Once we have removed legacy arbitrator from dispute domain we can remove that method as well.
// Atm it is still used by traderSignAndFinalizeDisputedPayoutTx which is used by ArbitrationManager.
Expand Down
21 changes: 15 additions & 6 deletions core/src/main/java/bisq/core/trade/messages/DelayedPayoutTxSignatureRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,19 @@
public final class DelayedPayoutTxSignatureRequest extends TradeMessage implements DirectMessage {
private final NodeAddress senderNodeAddress;
private final byte[] delayedPayoutTx;
private final byte[] delayedPayoutTxSellerSignature;

public DelayedPayoutTxSignatureRequest(String uid,
String tradeId,
NodeAddress senderNodeAddress,
byte[] delayedPayoutTx) {
byte[] delayedPayoutTx,
byte[] delayedPayoutTxSellerSignature) {
this(Version.getP2PMessageVersion(),
uid,
tradeId,
senderNodeAddress,
delayedPayoutTx);
delayedPayoutTx,
delayedPayoutTxSellerSignature);
}

///////////////////////////////////////////////////////////////////////////////////////////
Expand All @@ -53,10 +56,12 @@ private DelayedPayoutTxSignatureRequest(int messageVersion,
String uid,
String tradeId,
NodeAddress senderNodeAddress,
byte[] delayedPayoutTx) {
byte[] delayedPayoutTx,
byte[] delayedPayoutTxSellerSignature) {
super(messageVersion, tradeId, uid);
this.senderNodeAddress = senderNodeAddress;
this.delayedPayoutTx = delayedPayoutTx;
this.delayedPayoutTxSellerSignature = delayedPayoutTxSellerSignature;
}


Expand All @@ -67,23 +72,27 @@ public protobuf.NetworkEnvelope toProtoNetworkEnvelope() {
.setUid(uid)
.setTradeId(tradeId)
.setSenderNodeAddress(senderNodeAddress.toProtoMessage())
.setDelayedPayoutTx(ByteString.copyFrom(delayedPayoutTx)))
.setDelayedPayoutTx(ByteString.copyFrom(delayedPayoutTx))
.setDelayedPayoutTxSellerSignature(ByteString.copyFrom(delayedPayoutTxSellerSignature)))
.build();
}

public static DelayedPayoutTxSignatureRequest fromProto(protobuf.DelayedPayoutTxSignatureRequest proto, int messageVersion) {
public static DelayedPayoutTxSignatureRequest fromProto(protobuf.DelayedPayoutTxSignatureRequest proto,
int messageVersion) {
return new DelayedPayoutTxSignatureRequest(messageVersion,
proto.getUid(),
proto.getTradeId(),
NodeAddress.fromProto(proto.getSenderNodeAddress()),
proto.getDelayedPayoutTx().toByteArray());
proto.getDelayedPayoutTx().toByteArray(),
proto.getDelayedPayoutTxSellerSignature().toByteArray());
}

@Override
public String toString() {
return "DelayedPayoutTxSignatureRequest{" +
"\n senderNodeAddress=" + senderNodeAddress +
",\n delayedPayoutTx=" + Utilities.bytesAsHexString(delayedPayoutTx) +
",\n delayedPayoutTxSellerSignature=" + Utilities.bytesAsHexString(delayedPayoutTxSellerSignature) +
"\n} " + super.toString();
}
}
27 changes: 18 additions & 9 deletions core/src/main/java/bisq/core/trade/messages/DelayedPayoutTxSignatureResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,20 @@
@Value
public final class DelayedPayoutTxSignatureResponse extends TradeMessage implements DirectMessage {
private final NodeAddress senderNodeAddress;
private final byte[] delayedPayoutTxSignature;
private final byte[] delayedPayoutTxBuyerSignature;
private final byte[] depositTx;

public DelayedPayoutTxSignatureResponse(String uid,
String tradeId,
NodeAddress senderNodeAddress,
byte[] delayedPayoutTxSignature) {
byte[] delayedPayoutTxBuyerSignature,
byte[] depositTx) {
this(Version.getP2PMessageVersion(),
uid,
tradeId,
senderNodeAddress,
delayedPayoutTxSignature);
delayedPayoutTxBuyerSignature,
depositTx);
}

///////////////////////////////////////////////////////////////////////////////////////////
Expand All @@ -53,10 +56,12 @@ private DelayedPayoutTxSignatureResponse(int messageVersion,
String uid,
String tradeId,
NodeAddress senderNodeAddress,
byte[] delayedPayoutTxSignature) {
byte[] delayedPayoutTxBuyerSignature,
byte[] depositTx) {
super(messageVersion, tradeId, uid);
this.senderNodeAddress = senderNodeAddress;
this.delayedPayoutTxSignature = delayedPayoutTxSignature;
this.delayedPayoutTxBuyerSignature = delayedPayoutTxBuyerSignature;
this.depositTx = depositTx;
}


Expand All @@ -67,24 +72,28 @@ public protobuf.NetworkEnvelope toProtoNetworkEnvelope() {
.setUid(uid)
.setTradeId(tradeId)
.setSenderNodeAddress(senderNodeAddress.toProtoMessage())
.setDelayedPayoutTxSignature(ByteString.copyFrom(delayedPayoutTxSignature))
.setDelayedPayoutTxBuyerSignature(ByteString.copyFrom(delayedPayoutTxBuyerSignature))
.setDepositTx(ByteString.copyFrom(depositTx))
)
.build();
}

public static DelayedPayoutTxSignatureResponse fromProto(protobuf.DelayedPayoutTxSignatureResponse proto, int messageVersion) {
public static DelayedPayoutTxSignatureResponse fromProto(protobuf.DelayedPayoutTxSignatureResponse proto,
int messageVersion) {
return new DelayedPayoutTxSignatureResponse(messageVersion,
proto.getUid(),
proto.getTradeId(),
NodeAddress.fromProto(proto.getSenderNodeAddress()),
proto.getDelayedPayoutTxSignature().toByteArray());
proto.getDelayedPayoutTxBuyerSignature().toByteArray(),
proto.getDepositTx().toByteArray());
}

@Override
public String toString() {
return "DelayedPayoutTxSignatureResponse{" +
"\n senderNodeAddress=" + senderNodeAddress +
",\n delayedPayoutTxSignature=" + Utilities.bytesAsHexString(delayedPayoutTxSignature) +
",\n delayedPayoutTxBuyerSignature=" + Utilities.bytesAsHexString(delayedPayoutTxBuyerSignature) +
",\n depositTx=" + Utilities.bytesAsHexString(depositTx) +
"\n} " + super.toString();
}
}
16 changes: 8 additions & 8 deletions core/src/main/java/bisq/core/trade/messages/DepositTxMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,17 @@
@Value
public final class DepositTxMessage extends TradeMessage implements DirectMessage {
private final NodeAddress senderNodeAddress;
private final byte[] depositTx;
private final byte[] depositTxWithoutWitnesses;

public DepositTxMessage(String uid,
String tradeId,
NodeAddress senderNodeAddress,
byte[] depositTx) {
byte[] depositTxWithoutWitnesses) {
this(Version.getP2PMessageVersion(),
uid,
tradeId,
senderNodeAddress,
depositTx);
depositTxWithoutWitnesses);
}

///////////////////////////////////////////////////////////////////////////////////////////
Expand All @@ -55,10 +55,10 @@ private DepositTxMessage(int messageVersion,
String uid,
String tradeId,
NodeAddress senderNodeAddress,
byte[] depositTx) {
byte[] depositTxWithoutWitnesses) {
super(messageVersion, tradeId, uid);
this.senderNodeAddress = senderNodeAddress;
this.depositTx = depositTx;
this.depositTxWithoutWitnesses = depositTxWithoutWitnesses;
}

@Override
Expand All @@ -68,7 +68,7 @@ public protobuf.NetworkEnvelope toProtoNetworkEnvelope() {
.setUid(uid)
.setTradeId(tradeId)
.setSenderNodeAddress(senderNodeAddress.toProtoMessage())
.setDepositTx(ByteString.copyFrom(depositTx)))
.setDepositTxWithoutWitnesses(ByteString.copyFrom(depositTxWithoutWitnesses)))
.build();
}

Expand All @@ -77,14 +77,14 @@ public static DepositTxMessage fromProto(protobuf.DepositTxMessage proto, int me
proto.getUid(),
proto.getTradeId(),
NodeAddress.fromProto(proto.getSenderNodeAddress()),
proto.getDepositTx().toByteArray());
proto.getDepositTxWithoutWitnesses().toByteArray());
}

@Override
public String toString() {
return "DepositTxMessage{" +
"\n senderNodeAddress=" + senderNodeAddress +
",\n depositTx=" + Utilities.bytesAsHexString(depositTx) +
",\n depositTxWithoutWitnesses=" + Utilities.bytesAsHexString(depositTxWithoutWitnesses) +
"\n} " + super.toString();
}
}
2 changes: 2 additions & 0 deletions core/src/main/java/bisq/core/trade/protocol/BuyerAsMakerProtocol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import bisq.core.trade.protocol.tasks.ApplyFilter;
import bisq.core.trade.protocol.tasks.TradeTask;
import bisq.core.trade.protocol.tasks.VerifyPeersAccountAgeWitness;
import bisq.core.trade.protocol.tasks.buyer.BuyerFinalizesDelayedPayoutTx;
import bisq.core.trade.protocol.tasks.buyer.BuyerProcessDelayedPayoutTxSignatureRequest;
import bisq.core.trade.protocol.tasks.buyer.BuyerSendsDelayedPayoutTxSignatureResponse;
import bisq.core.trade.protocol.tasks.buyer.BuyerSetupDepositTxListener;
Expand Down Expand Up @@ -103,6 +104,7 @@ protected void handle(DelayedPayoutTxSignatureRequest message, NodeAddress peer)
MakerRemovesOpenOffer.class,
BuyerVerifiesPreparedDelayedPayoutTx.class,
BuyerSignsDelayedPayoutTx.class,
BuyerFinalizesDelayedPayoutTx.class,
BuyerSendsDelayedPayoutTxSignatureResponse.class)
.withTimeout(30))
.executeTasks();
Expand Down
2 changes: 2 additions & 0 deletions core/src/main/java/bisq/core/trade/protocol/BuyerAsTakerProtocol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import bisq.core.trade.protocol.tasks.ApplyFilter;
import bisq.core.trade.protocol.tasks.TradeTask;
import bisq.core.trade.protocol.tasks.VerifyPeersAccountAgeWitness;
import bisq.core.trade.protocol.tasks.buyer.BuyerFinalizesDelayedPayoutTx;
import bisq.core.trade.protocol.tasks.buyer.BuyerProcessDelayedPayoutTxSignatureRequest;
import bisq.core.trade.protocol.tasks.buyer.BuyerSendsDelayedPayoutTxSignatureResponse;
import bisq.core.trade.protocol.tasks.buyer.BuyerSetupDepositTxListener;
Expand Down Expand Up @@ -119,6 +120,7 @@ protected void handle(DelayedPayoutTxSignatureRequest message, NodeAddress peer)
BuyerProcessDelayedPayoutTxSignatureRequest.class,
BuyerVerifiesPreparedDelayedPayoutTx.class,
BuyerSignsDelayedPayoutTx.class,
BuyerFinalizesDelayedPayoutTx.class,
BuyerSendsDelayedPayoutTxSignatureResponse.class)
.withTimeout(30))
.executeTasks();
Expand Down
Loading