Fix remaining blackmail vulnerabilities #4789
Conversation
| Transaction preparedDepositTx = processModel.getBtcWalletService().getTxFromSerializedTx(processModel.getPreparedDepositTx()); | ||
| // Remove witnesses from preparedDepositTx, so that the seller can still compute the final | ||
| // tx id, but cannot publish it before providing the buyer with a signed delayed payout tx. | ||
| return preparedDepositTx.bitcoinSerialize(false); |
There was a problem hiding this comment.
The Transaction.bitcoinSerializeToStream method does use that flag not only for adding segwit data but also for writing a flag:
// marker, flag
if (useSegwit) {
stream.write(0);
stream.write(1);
}
I don't know which function this flag has, but maybe its more safe to remove the segwit data in a different way. Maybe @oscarguindzberg can add his input here...
There was a problem hiding this comment.
the function of the flag is "tx serialzed using the new segwit format".
bitcoinSerialize(false) seems fine if you want to avoid sending the signatures.
|
I like this PR and it should be included in v1.5.0. From what I can see it looks reasonable but I'm not familiar enough with all the bitcoinj details. |
|
I just spotted that I accidentally used the non-segwit version of
This erroneously skips the check, as the ScriptSig is empty. I need to additionally pass the witness and locked up trade amount. I'll post another commit to fix this shortly. |
|
@oscarguindzberg Could you please review that PR as well? |
|
@stejbac The build is failing @oscarguindzberg I will wait for you ack before merging this |
|
@sqrrm I'm not sure why the build is failing (or the Codacy checks). The Travis build log of one job, which ran 9 hours ago, has the following error:
The other job (apparently run around the same time) has failures in |
Which jar travis cannot download at any given time is fairly random. Closing and re-opening the PR will re-run the travis build, and the dependency download will probably work. However, Codacy's |
Yes, please ignore the Codacy warning. It is just about the fact that it is not configured to run on this branch. I just manually triggered a travis re-build for this PR. |
|
I just saw in Travis that there are two failing tests right now: |
🤔 it does run locally without any problems. |
|
I got the failing tests locally after I tried merging onto release/1.5.0, so the tip map be broken right now. It seems it can be fixed by adding tearDown() methods in the failing tests to call PersistenceManager.shutDown, which removes the particular persistence manager instance created in each test from a global map. (All but one tests in each affected test class fail, presumably depending in the order they run in.) |
Yes, also #4816 is affected by this right now. Could you please push this commit to this branch to see if Travis is happy with it as well and I'll cherry-pick and push it to the other one as well? |
|
It looks like it fixed the jdk10 job but not the jdk12 job, which I guess has an unrelated problem. |
I just restarted the job. Travis is kind of unstable the last couple of days. |
|
@ripcurlx OK, I guess I should rebase onto release/1.5.0 to resolve the conflict. |
Make sure witness data is stripped from the seller's prepared deposit tx, in addition to ScriptSig data, to prevent the buyer from being able to publish it prematurely (before having signed the delayed payout tx).
Include a new 'delayedPayoutTxSellerSignature' field with the prepared delayed payout tx sent to the buyer, in DelayedPayoutTxSignatureRequest. This will allow the buyer to compute the final, signed delayedPayoutTx as early as possible and withhold their deposit tx witness from the seller until they know they have a valid delayedPayoutTx, preventing its premature publishing in the fully segwit case. (To be done in a later commit - for now just save the seller's delayedPayoutTx signature.) As part of this, run the SellerSignsDelayedPayoutTx trade task at an earlier step (just after payout tx creation) to make its signature available to the seller ASAP. Also rename 'delayedPayoutTxSignature' to 'delayedPayoutTxBuyerSignature' in DelayedPayoutTxSignatureResponse.
Improve validation of the buyer's delayed payout tx (both before & after they get the final DepositTxAndDelayedPayoutTxMessage from the peer), by finalising it independently of the seller. This is now possible since their 2-of-2 signature is included in the DelayedPayoutSignatureRequest. Check that the final delayedPayoutTx received from the seller matches it byte-for-byte (which actually makes its receipt redundant now). This also fixes an apparent security bug, where the final validation of the delayedPayoutTx appears to skip any kind of signature check (only a deposit tx hash check, which is still necessary). Finally, optimistically check the deposit tx against the input of the prepared delayedPayoutTx received from the seller, in the case that the former is non-malleable (that is, the fully segwit case) and thus has a stable ID given by the hash of the buyer's preparedDepositTx.
Strip all input witnesses from the depositTx message fields sent from the buyer, until the last (DelayedPayoutTxSignatureResponse) message is sent, where they can be bundled in as an extra field. Since the witness data doesn't affect the final deposit tx id, the seller does not need to know it until actually publishing the tx. In the (fully) segwit case, this allows the buyer to prevent the seller from publishing the deposit tx until the buyer has a valid, fully signed delayedPayoutTx. Provide the final witness data in an extra 'depositTx' field in DelayedPayoutTxSignatureResponse, which the seller can merge with his depositTx witness block (for his own input signatures).
Disallow non-P2WH depositTx inputs from the taker, while continuing to allow them from the maker, so that offers created pre-v1.5.0 can still be taken. (After some time, those inputs could be disallowed too.) This is mainly to prevent mass blackmail attacks, where more victims' money could be locked up than the DAO could possibly compensate them all for. (This is probably only an attractive attack for a buyer anyway, at least with the earlier commits.)
Make sure to use the segwit version of Script.correctlySpends in TradeWalletService.finalizeDelayedPayoutTx, which requires the input value and witness to be passed explicitly (as the latter holds the actual signature). This was causing BuyerFinalizesDelayedPayoutTx to fail to do any kind of signature check. Also refactor the method slightly and remove a redundant call to WalletService.checkScriptSig (which does the same thing as TransactionInput.verify) in the branch used by the seller.
fd79895 to
3574204
Compare
Do some extra sanity checks like tx.outputSum < tx.inputSum, to rule out any edge cases where an invalid delayed payout tx might still arise.
Make sure the taker checks the value of the 2-of-2 multisig output of the deposit tx created by the maker, before signing it. This avoids a potential security hole, where the maker attempts to steal most of the deposit by using the wrong output value and adding an extra 'change' output to himself. Note that there's no actual vulnerability at present, as the output value is indirectly checked via the validation of the delayed payout tx. In particular, the extra checks added in 345426f as part of bisq-network#4789 (Fix remaining blackmail vulnerabilities) place a lower bound on the delayed payout tx input value and with it the deposit tx output value. However, explicitly checking the amount is more robust.
This PR attempts to take advantage of the SegWit trade protocol upgrade (hard fork), to fix all the remaining cases where a buyer or seller can publish a deposit tx without their peer having a valid delayed payout tx (at least in the fully SegWit case, when taking an offer created in v1.5.0+). When taking pre-1.5.0 offers, the deposit tx inputs will be of mixed type, which makes it malleable and thus impossible to completely prevent either trader from altering its ID and thus rendering their peer's delayed payout tx invalid. In that case, the buyer should still fully validate their delayed payout tx anyway, against the agreed deposit tx, so that if the latter changes before confirming, the trade hopefully fails before the buyer makes payment. The PR additionally fixes a bug where (it appears) the buyer fails to check the signature of the final delayed payout tx.
These changes are intended to prevent blackmail attacks (or reduce the risk of them in not-fully-SegWit cases where the deposit tx is malleable).