extractHash does not check the size of the script

[sr-gi]

Description

extractHash checks that the template for P2PKH, P2SH, P2PKH and P2WSH is valid (proper prefix and suffix), but the actual size of the provided script is never checked.

bitcoin-spv/solidity/contracts/BTCUtils.sol

Lines 406 to 434 in 3a35db8

	function extractHash(bytes memory _output) internal pure returns (bytes memory) {
	if (uint8(_output.slice(9, 1)[0]) == 0) {
	uint256 _len = uint8(_output[8]);
	if (_len < 2) {
	return hex"";
	}
	_len -= 2;
	// Check for maliciously formatted witness outputs
	if (uint8(_output.slice(10, 1)[0]) != uint8(_len)) {
	return hex"";
	}
	return _output.slice(11, _len);
	} else {
	bytes32 _tag = _output.keccak256Slice(8, 3);
	// p2pkh
	if (_tag == keccak256(hex"1976a9")) {
	// Check for maliciously formatted p2pkh
	if (uint8(_output.slice(11, 1)[0]) != 0x14 ||
	_output.keccak256Slice(_output.length - 2, 2) != keccak256(hex"88ac")) {
	return hex"";
	}
	return _output.slice(12, 20);
	//p2sh
	} else if (_tag == keccak256(hex"17a914")) {
	// Check for maliciously formatted p2sh
	if (uint8(_output.slice(_output.length - 1, 1)[0]) != 0x87) {
	return hex"";
	}
	return _output.slice(11, 20);

Therefore a script encoded under the following format will be accepted:

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>

This can cause issues if the provided data does not come from a validated transaction, for instance if the data is being provided directly from a user.

This is related to keep-network/tbtc#658

Invalid P2PKH example

The following call won't be caught, even though the script is invalid (notice the extra 88 by the end of the script):

extractHash(0x4062b007000000001976a914f86f0bc0a2232970ccdf4569815db500f12683618888ac)

Fix

Add an additional check for maliciously formatted scripts to check the actual length of the provided script.

[sr-gi]

Update: this actually affects both segwit and legacy outputs.

[prestwich]

Related: #155 #156

As a general rule, the parser does not handle invalid structures. Inputs to this function are assumed to have passed validateVout and some inclusion proof, so validity checks wrt length are not necessary here