requestRedemption accepts invalid _redeemerOutputScripts
sr-gi opened this issue · 5 comments
Description
Invalid _redeemerOutputScripts looking like properly formatted redeemscripts can be passed to requestRedemption without being caught. This affects to the 4 output types supported (P2PKH, P2SH, P2WPKH and P2WSH).
This is supposed to be guarded by BCUtils.extractHash, but the actual size of the script is never checked.
The root of the issue is the same for segwit and legacy outputs, the encoding of the script size is assumed to be properly set. For legacy, it is done by checking that 20 bytes are pushed to the stack (0x14) but not checking the actual size of the script. For Segwit scripts, the size of the script is parsed from the output, but the actual size is not computed either (the expected size is compared with the parsed size, but not with the actual size).
Exploit Scenario
Call requestRedemption with a script using the following format:
- P2PKH:
1976a914 <script_with_more_than_20_bytes> 88ac - P2SH:
17a914 <script_with_more_than_20_bytes> 87 - P2PKH:
0014 <script_with_more_than_20_bytes> - P2PKH:
0020 <script_with_more_than_32_bytes>
Invalid P2PKH example
_outputValueBytes = 4062b00700000000
_redeemerOutputScript = 1976a914f86f0bc0a2232970ccdf4569815db500f12683618888ac
This script contains an additional byte (88) before the script suffix (88ac).
Mitigation
Force a check of the script size on top of checking that the script template is properly formatted (this should be done in BCUtils.extractHash. Will open a linked issue there too).
Extra information
Notice extractHash is also used in several other parts of the code. However, it is normally guarded by validateVout, which will catch a transaction including this type of script, since they are incorrectly formatted. Also, this transaction won't be accepted in a block, so this should not trigger if the provided data comes from a block or can be checked agains a block / known valid transaction. However, in this case the data is provided directly from the user, so no additional checks are run.
After further checking this, it would also fail for Segwit outputs. Updating the issue.
Link to the underlying issue in bitcoin-spv - @prestwich we can hack around this here, but I think a new release of bitcoin-spv is the way to go.
Generally BTCUtils was written to parse pre-validated structs. So this comes from using it outside that setting. Good catch
The quick fix would be be to add a validateVout call immediately before.
The bitcoin-spv@next already makes these checks. So it's pretty simple to backport them
another immediate fix:
require(uint8(_output[8]) + 1 == output.length + 9);