This PR adds some sanity checking to DecodeHexTx to make sure that the scripts contain valid opcodes. The main purpose of doing this is to avoid decoding witness transactions as 0-input 1-output non-witness transactions. The scripts are likely to be garbage and contain invalid opcodes when decoded as non-witness.
There is an issue where other invalid opcodes (not just OP_INVALIDOPCODE) are not handled but I am not sure how to go about doing that.
This is related to #9522.
87 | @@ -88,18 +88,38 @@ CScript ParseScript(const std::string& s) 88 | return result; 89 | } 90 | 91 | +// Check that all of the input and output scripts of a transaction contains valid opcodes 92 | +bool CheckTxScriptsSanity(CMutableTransaction& tx) 93 | +{ 94 | + // Check input scripts 95 | + for(unsigned int i = 0; i < tx.vin.size(); i++) {
You should probably skip this loop for coinbase transactions, as those don't have meaningful input scripts.
Is testing the inputs even useful, as the function of this test is to distinguish between transactions that may errorneously be decoded as 0-input transaction?
Nit: space after for (and elsewhere).
87 | @@ -88,18 +88,40 @@ CScript ParseScript(const std::string& s) 88 | return result; 89 | } 90 | 91 | +// Check that all of the input and output scripts of a transaction contains valid opcodes 92 | +bool CheckTxScriptsSanity(CMutableTransaction& tx)
Mark the tx argument const?
done
utACK 6909865869d9f2582e33e936e770538dfc541cf7
Something else you could consider is checking whether the total script is not too large (MAX_SCRIPT_SIZE) or the stack elements aren't too large (MAX_SCRIPT_ELEMENT_SIZE).
272 | +{ 273 | + CScript::const_iterator it = begin(); 274 | + while (it < end()) { 275 | + opcodetype opcode; 276 | + CScript::const_iterator prev_it = it; 277 | + if (!GetOp(it, opcode) || opcode > 0xb9 || it - prev_it > MAX_SCRIPT_ELEMENT_SIZE) {
I think this isn't actually correct. MAX_SCRIPT_ELEMENT_SIZE is the maximum length of the data, but its encoding as a CScript push may be up to 2 bytes more. Perhaps you can use GetOp2, which returns the data being pushed?
done
99 | + } 100 | + } 101 | + } 102 | + // Check output scripts 103 | + for (unsigned int i = 0; i < tx.vout.size(); i++) { 104 | + if (!tx.vout[0].scriptPubKey.HasValidOps() || tx.vout[0].scriptPubKey.size() > MAX_SCRIPT_SIZE) {
vout[i]
92 | +bool CheckTxScriptsSanity(const CMutableTransaction& tx) 93 | +{ 94 | + // Check input scripts for non-coinbase txs 95 | + if (!CTransaction(tx).IsCoinBase()) { 96 | + for (unsigned int i = 0; i < tx.vin.size(); i++) { 97 | + if (!tx.vin[i].scriptSig.HasValidOps()) {
No MAX_SCRIPT_SIZE test here?
done
reutACK e76d516a19f09f546d6fb5f16f8b4c758c5287ed
Added a function in CScript that checks if the script contains valid opcodes.
Add a test for that function
Make sure that the scripts of decoded transactions are valid scripts.
272 | +{ 273 | + CScript::const_iterator it = begin(); 274 | + while (it < end()) { 275 | + opcodetype opcode; 276 | + std::vector<unsigned char> item; 277 | + if (!GetOp(it, opcode, item) || opcode > 0xb9 || item.size() > MAX_SCRIPT_ELEMENT_SIZE) {
Let's define a constant for 0xb9, this is kind of a magic number here. OP_NOP10 would work, but maybe it's better to explicitly mark what the maximum valid opcode is.
Done
This sanity check doesn't work. invalid opcodes are not illegal, and can even appear in real transactions. For an example see transaction 95f7832b6449348bd1f3bc2e6eb110785248427bbe13853f2d61da9a6e810800 on testnet.
@roconnor-blockstream This test is only a heuristic to determine whether to prefer a non-witness interpretation of a transaction over a witness interpretation when it has 0 inputs. It's also only relevant for interpreting inputs to decoderawtransaction and fundrawtransaction (which are the only things that accept transactions with 0 inputs in the first place).
We cannot use heuristics for deciding how to interpret ambiguous parameters to API calls such as fundrawtransaction. It is a security hazard.
Imagine someone is using Bitcoin Core as part of some protocol. They have assembled data from various parties, some of them generated adversarially. The protocol safely validates the adversarial inputs and composes a valid, partial transaction, and passes it to fundrawtransaction, then sends that output to signrawtransaction and then passes it around or even broadcasts it with sendrawtransaction.
The system has validated the construction of their transaction, and the designers have tested it many times and believes it work. However, it isn't secure. After deployment an adversary tickles their data, perhaps by creating an output with invalid opcodes. These adversarially provided output scripts are legal and pass protocol's validation engine. However this causes the data passed to fundrawtransaction to be interpreted differently, either as a segwit transaction when the protocol thinks it is generating a legacy transaction, or vice versa. Now fundrawtransaciton is generating a transaction that is different than the protocol intends, and it is probably a transaction sending the funds somewhere else. Nobody expected that fine details about the contents of output scripts to change how the transaction being passed is parsed, that would be very surprising, so the software just assumes fundrawtransaction output is the transaction it thinks it has created, and blindly sends it onto signrawtransaction. With that transaction now signed, who knows where the money will end up.
@roconnor-blockstream Then what do you suggest? The only full solution is not using the same format for partial transactions (something that @achow101's PSBT format accomplishes). Anything else will inherently risk being ambiguous.
Yes, ultimately this format for partial transactions must be retired.
In the mean time I suggest either adding a, preferably mandatory, flag to fundrawtransaction to tell it whether the parameter is a partially signed segwit transaction or a partially signed legacy transaction.
If you choose to make the flag optional, then, when no flag is given, you must parse the parameter both ways and fail of both parses are successful. For consistency, and safety, decoderawtransaction should operate the same way.
(instead of a flag, you could instead create two new API calls for fundrawlegacytransaction and fundrawsegwittransaction).
Perhaps there are some other ideas.
