From 749c2d28ec3cc22a0fd40c933092fd227aadd669 Mon Sep 17 00:00:00 2001 From: BYUNGKWON BAE Date: Tue, 27 Oct 2020 14:43:50 +0900 Subject: [PATCH] IS-1120: prevalidate a tx more strictly (#525) * Pre-validate on entered request parameters * Malformed number string * Not allow uppercase character * Malformed address * v3 protocol only * Required fields check * testcase --- iconservice/base/type_converter_templates.py | 1 + iconservice/icon_inner_service.py | 3 +- iconservice/icon_service_engine.py | 7 +- iconservice/iconscore/icon_pre_validator.py | 104 ++++++++- .../iiss/decentralized/test_call.py | 8 +- tests/integrate_test/test_integrate_base.py | 52 ++++- .../test_icon_origin_pre_validator.py | 201 ++++++++++++++++++ 7 files changed, 362 insertions(+), 14 deletions(-) create mode 100644 tests/unit_test/iconscore/test_icon_origin_pre_validator.py diff --git a/iconservice/base/type_converter_templates.py b/iconservice/base/type_converter_templates.py index 5941e76f9..3faa6e4b8 100644 --- a/iconservice/base/type_converter_templates.py +++ b/iconservice/base/type_converter_templates.py @@ -107,6 +107,7 @@ class ConstantKeys: FROM = "from" TO = "to" VALUE = "value" + NID = "nid" STEP_LIMIT = "stepLimit" FEE = "fee" NONCE = "nonce" diff --git a/iconservice/icon_inner_service.py b/iconservice/icon_inner_service.py index fed9bb4eb..0e764b4c7 100644 --- a/iconservice/icon_inner_service.py +++ b/iconservice/icon_inner_service.py @@ -409,8 +409,9 @@ async def validate_transaction(self, request: dict): def _validate_transaction(self, request: dict): try: + Logger.info(tag=_TAG, msg=f'validate_transaction Request: {request}') converted_request = TypeConverter.convert(request, ParamType.VALIDATE_TRANSACTION) - self._icon_service_engine.validate_transaction(converted_request) + self._icon_service_engine.validate_transaction(converted_request, request) response = MakeResponse.make_response(ExceptionCode.OK) except FatalException as e: self._log_exception(e, _TAG) diff --git a/iconservice/icon_service_engine.py b/iconservice/icon_service_engine.py index 0f0e0012b..06ad775df 100644 --- a/iconservice/icon_service_engine.py +++ b/iconservice/icon_service_engine.py @@ -1064,7 +1064,7 @@ def query(self, method: str, params: dict) -> Any: ret = self._call(context, method, params) return ret - def validate_transaction(self, request: dict) -> None: + def validate_transaction(self, request: dict, origin_request: dict) -> None: """Validate JSON-RPC transaction request before putting it into transaction pool @@ -1072,6 +1072,7 @@ def validate_transaction(self, request: dict) -> None: on JSON-RPC Server IconPreValidator focuses on business logic and semantic problems + :param origin_request: JSON_RPC Original request for more strict validate :param request: JSON-RPC request values in request have already been converted to original format in IconInnerService @@ -1089,6 +1090,8 @@ def validate_transaction(self, request: dict) -> None: context = self._context_factory.create(IconScoreContextType.QUERY, self._get_last_block()) context.set_step_counter() + origin_params = origin_request['params'] + try: self._push_context(context) @@ -1101,7 +1104,7 @@ def validate_transaction(self, request: dict) -> None: data = params['data'] input_size = get_input_data_size(context.revision, data) minimum_step += input_size * context.inv_container.step_costs.get(StepType.INPUT, 0) - + self._icon_pre_validator.origin_request_execute(origin_params, context.revision) self._icon_pre_validator.execute(context, params, step_price, minimum_step) if to.is_contract: diff --git a/iconservice/iconscore/icon_pre_validator.py b/iconservice/iconscore/icon_pre_validator.py index e24aba4e8..a710f242f 100644 --- a/iconservice/iconscore/icon_pre_validator.py +++ b/iconservice/iconscore/icon_pre_validator.py @@ -19,8 +19,10 @@ from iconcommons.logger import Logger from .icon_score_step import get_input_data_size -from ..base.address import Address, AddressPrefix, SYSTEM_SCORE_ADDRESS, generate_score_address + +from ..base.address import Address, AddressPrefix, SYSTEM_SCORE_ADDRESS, generate_score_address, is_icon_address_valid from ..base.exception import InvalidRequestException, InvalidParamsException, OutOfBalanceException +from ..base.type_converter_templates import ConstantKeys from ..icon_constant import ( FIXED_FEE, MAX_DATA_SIZE, DEFAULT_BYTE_SIZE, DATA_BYTE_ORDER, Revision, DeployState, DataType @@ -34,6 +36,43 @@ TAG = "PV" +REQUEST_PARAMS = ( + ConstantKeys.VERSION, + ConstantKeys.STEP_LIMIT, + ConstantKeys.NID, + ConstantKeys.TIMESTAMP, + ConstantKeys.VALUE, + ConstantKeys.NONCE, + ConstantKeys.FROM, + ConstantKeys.TO, + ConstantKeys.SIGNATURE, + ConstantKeys.DATA_TYPE, + ConstantKeys.DATA, +) + +REQUIRED_PARAMS = ( + ConstantKeys.VERSION, + ConstantKeys.STEP_LIMIT, + ConstantKeys.NID, + ConstantKeys.TIMESTAMP, + ConstantKeys.FROM, + ConstantKeys.TO, + ConstantKeys.SIGNATURE +) + +INT_PARAMS = ( + ConstantKeys.VERSION, + ConstantKeys.STEP_LIMIT, + ConstantKeys.NID, + ConstantKeys.TIMESTAMP, + ConstantKeys.VALUE, + ConstantKeys.NONCE +) + +ADDR_PARAMS = ( + ConstantKeys.FROM, + ConstantKeys.TO +) class IconPreValidator: @@ -47,6 +86,69 @@ def __init__(self) -> None: """ pass + def origin_request_execute(self, params: dict, revision: int): + if revision < Revision.IMPROVED_PRE_VALIDATOR.value: + return + self.origin_pre_validate_version(params) + self.origin_pre_validate_params(params) + self.origin_validate_fields(params) + + def origin_validate_fields(self, params: dict): + for param, value in params.items(): + self.origin_validate_param(param) + self.origin_validate_value(param, value) + + @classmethod + def origin_pre_validate_version(cls, params: dict): + version: str = params.get(ConstantKeys.VERSION, None) + if version != '0x3': + raise InvalidRequestException(f'Invalid message version, got {version}') + + @classmethod + def origin_pre_validate_params(cls, params: dict): + if len(params) > len(REQUEST_PARAMS): + raise InvalidRequestException('Unexpected Parameters') + + required_results = [ + required_key + for required_key + in REQUIRED_PARAMS + if required_key not in params + ] + + if required_results: + raise InvalidRequestException( + f'Not included required parameters, missing parameters {required_results}' + ) + + @classmethod + def origin_validate_param(cls, param: str): + if param not in REQUEST_PARAMS: + raise InvalidParamsException(f'Unexpected Parameters, got {param}') + + @classmethod + def origin_validate_value(cls, param: str, value: str): + if param in INT_PARAMS: + if not cls.is_integer_type(value): + raise InvalidRequestException(f'Unexpected INT Type, got {value}') + elif param in ADDR_PARAMS: + if not cls.is_address_type(value): + raise InvalidRequestException(f'Unexpected Address Type, got {value}') + + @classmethod + def is_integer_type(cls, value: str) -> bool: + try: + if value.startswith('0x'): + return value == hex(int(value, 16)) + except ValueError: + return False + else: + return False + + @classmethod + def is_address_type(cls, value: str) -> bool: + return is_icon_address_valid(value) + def execute(self, context: 'IconScoreContext', params: dict, step_price: int, minimum_step: int): """Validate a transaction on icx_sendTransaction If failed to validate a tx, raise an exception diff --git a/tests/integrate_test/iiss/decentralized/test_call.py b/tests/integrate_test/iiss/decentralized/test_call.py index 87977e65d..fbe4c85c9 100644 --- a/tests/integrate_test/iiss/decentralized/test_call.py +++ b/tests/integrate_test/iiss/decentralized/test_call.py @@ -78,15 +78,15 @@ def test_validate_data_type(self): value=value, disable_pre_validate=True ) - - self.icon_service_engine.validate_transaction(tx) + origin_params = {'params': self.make_origin_params(tx['params'])} + self.icon_service_engine.validate_transaction(tx, origin_params) invalid_data_types = ("abc", 1, 1.1, b"call", b"deploy", b"deposit", b"message") for data_type in invalid_data_types: tx["params"]["dataType"] = data_type with self.assertRaises(InvalidParamsException): - self.icon_service_engine.validate_transaction(tx) + self.icon_service_engine.validate_transaction(tx, origin_params) for data_type in DataType._TYPES: params = tx["params"] @@ -100,7 +100,7 @@ def test_validate_data_type(self): else: continue - self.icon_service_engine.validate_transaction(tx) + self.icon_service_engine.validate_transaction(tx, origin_params) def test_invalid_score_call_failure_on_invoke(self): self.set_revision(Revision.IMPROVED_PRE_VALIDATOR.value) diff --git a/tests/integrate_test/test_integrate_base.py b/tests/integrate_test/test_integrate_base.py index bc66deb35..8a3cc4bfe 100644 --- a/tests/integrate_test/test_integrate_base.py +++ b/tests/integrate_test/test_integrate_base.py @@ -342,6 +342,33 @@ def _write_precommit_state_in_leader(self, block_height: int, old_block_hash: by assert block_height == self._block_height self._prev_block_hash = new_block_hash + def make_origin_params(self, params: dict): + origin_items = [ + ('version', int, hex), + ('from', Address, str), + ('to', Address, str), + ('value', int, hex), + ('nonce', int, hex), + ('stepLimit', int, hex), + ('timestamp', int, hex), + ('nid', int, hex), + ('signature', str, str) + ] + origin_params = params.copy() + + if "txHash" in origin_params: + del origin_params["txHash"] + + for key, value_type, origin_type in origin_items: + value = origin_params.get(key, None) + if value is None and key == "nid": + origin_params[key] = hex(2) + + if isinstance(value, value_type): + origin_params[key] = origin_type(value) + + return origin_params + def rollback(self, block_height: int = -1, block_hash: Optional[bytes] = None): """Rollback the current state to the old one indicated by a given block @@ -473,6 +500,7 @@ def create_deploy_score_tx(self, "data": deploy_data } + origin_params = {'params': self.make_origin_params(request_params)} method = 'icx_sendTransaction' # Insert txHash into request params request_params['txHash'] = create_tx_hash() @@ -482,7 +510,7 @@ def create_deploy_score_tx(self, } if pre_validation_enabled: - self.icon_service_engine.validate_transaction(tx) + self.icon_service_engine.validate_transaction(tx, origin_params) return tx @@ -518,6 +546,9 @@ def create_score_call_tx(self, } } + origin_params = { + 'params': self.make_origin_params(request_params) + } method = 'icx_sendTransaction' # Insert txHash into request params request_params['txHash'] = create_tx_hash() @@ -527,7 +558,7 @@ def create_score_call_tx(self, } if pre_validation_enabled: - self.icon_service_engine.validate_transaction(tx) + self.icon_service_engine.validate_transaction(tx, origin_params) return tx @@ -560,6 +591,9 @@ def create_transfer_icx_tx(self, else: request_params["version"] = self._version + origin_params = { + 'params': self.make_origin_params(request_params) + } method = 'icx_sendTransaction' # Insert txHash into request params request_params['txHash'] = create_tx_hash() @@ -569,7 +603,7 @@ def create_transfer_icx_tx(self, } if not disable_pre_validate: - self.icon_service_engine.validate_transaction(tx) + self.icon_service_engine.validate_transaction(tx, origin_params) return tx def create_message_tx(self, @@ -598,6 +632,10 @@ def create_message_tx(self, "data": '0x' + data.hex(), } + origin_params = { + 'params': self.make_origin_params(request_params) + } + method = 'icx_sendTransaction' # Inserts txHash into request params request_params['txHash'] = create_tx_hash() @@ -607,7 +645,7 @@ def create_message_tx(self, } if pre_validation_enabled: - self.icon_service_engine.validate_transaction(tx) + self.icon_service_engine.validate_transaction(tx, origin_params) return tx def create_deposit_tx(self, @@ -639,7 +677,9 @@ def create_deposit_tx(self, "action": action, } } - + origin_params = { + 'params': self.make_origin_params(request_params) + } for k, v in params.items(): request_params["data"][k] = v @@ -652,7 +692,7 @@ def create_deposit_tx(self, } if pre_validation_enabled: - self.icon_service_engine.validate_transaction(tx) + self.icon_service_engine.validate_transaction(tx, origin_params) return tx diff --git a/tests/unit_test/iconscore/test_icon_origin_pre_validator.py b/tests/unit_test/iconscore/test_icon_origin_pre_validator.py new file mode 100644 index 000000000..1fa35c329 --- /dev/null +++ b/tests/unit_test/iconscore/test_icon_origin_pre_validator.py @@ -0,0 +1,201 @@ +import pytest +import os +import random + +from iconservice.iconscore.icon_pre_validator import IconPreValidator +from iconservice.base.address import ( + Address, + AddressPrefix +) +from iconservice.base.exception import ( + InvalidRequestException, + InvalidParamsException +) + + +def make_required_parameters(**kwargs) -> dict: + return { + 'version': kwargs['version'], + 'from': kwargs['_from'], + 'to': kwargs['to'], + 'stepLimit': kwargs['stepLimit'], + 'nid': '0x1', + 'timestamp': kwargs['timestamp'], + 'signature': kwargs['signature'], + } + + +def make_origin_parameters(option: dict = None) -> dict: + params = make_required_parameters( + version=hex(3), + _from=str(Address.from_data(AddressPrefix.EOA, os.urandom(20))), + to=str(Address.from_data(random.choice([AddressPrefix.EOA, AddressPrefix.CONTRACT]), os.urandom(20))), + stepLimit=hex(random.randint(10, 5000)), + timestamp=hex(random.randint(10, 5000)), + signature='VAia7YZ2Ji6igKWzjR2YsGa2m53nKPrfK7uXYW78QLE+ATehAVZPC40szvAiA6NEU5gCYB4c4qaQzqDh2ugcHgA=' + ) + + if option: + params.update(option) + return params + + +@pytest.fixture +def validator(): + validator = IconPreValidator() + return validator + + +@pytest.mark.parametrize( + 'malformed_value, expected', + [ + ('', False), + ('0x0001', False), + ('0x0000001', False), + ('100', False), + ('0x1e', True) + ] +) +def test_malformed_number_string(validator, malformed_value, expected): + assert validator.is_integer_type(malformed_value) == expected + + +@pytest.mark.parametrize( + 'malformed_value, expected', + [ + ('', False), + ('0x1A', False), + ('0x3E', False), + ('0xBE', False), + ('0Xff', False), + ('0xzz', False), + ('0x123k', False), + ('0xff', True), + ] +) +def test_malformed_uppercase_string(validator, malformed_value, expected): + assert validator.is_integer_type(malformed_value) == expected + + +@pytest.mark.parametrize( + 'malformed_value, expected', + [ + ('', False), + ('0xqw1234', False), + ('hxEfe3', False), + ('hx1234', False), + ('cx1234', False), + ('cxEab1', False), + ('1234', False), + ('hx3f945d146a87552487ad70a050eebfa2564e8e5c', True), + ('cxba62bb61baf8dd8b6a04633fe33806547785a00c', True) + ] +) +def test_malformed_address(validator, malformed_value, expected): + assert validator.is_address_type(malformed_value) == expected + + +@pytest.mark.parametrize( + 'msg', + [ + {}, + make_origin_parameters({'version': '0x1'}), + make_origin_parameters({'version': '0x2'}), + ] +) +def test_pre_validate_version(validator, msg): + with pytest.raises(InvalidRequestException): + validator.origin_pre_validate_version(msg) + + +@pytest.mark.parametrize( + 'msg', + [ + {}, + {'version': '0x3'}, + {'version': '0x3', 'from': 'temp', 'to': 'temp', 'stepLimit': 'temp', 'timestamp': 'test'}, + make_origin_parameters({ + 'value': '', + 'nonce': '', + 'data_type': '', + 'data': '', + 'item': '', + 'test': '', + 'failed': '', + 'failure': '' + }) + ] +) +def test_pre_validate_params(validator, msg): + with pytest.raises(InvalidRequestException): + validator.origin_pre_validate_params(msg) + + +@pytest.mark.parametrize( + 'param', + [ + '', + None, + 'foo', + 'bar' + ] +) +def test_validate_param(validator, param): + with pytest.raises(InvalidParamsException): + validator.origin_validate_param(param) + + +@pytest.mark.parametrize( + 'param, value', + [ + ('stepLimit', ''), + ('stepLimit', 'Fx35'), + ('to', '0xFF'), + ('value', '0X1e'), + ('to', 'cX3F9e') + ] +) +def test_validate_value(validator, param, value): + with pytest.raises(InvalidRequestException): + validator.origin_validate_value(param, value) + + +@pytest.mark.parametrize( + 'msg', + [ + make_origin_parameters({'test': 'foo'}), + make_origin_parameters({'to': 'hxEE'}), + make_origin_parameters({'stepLimit': '0xEf'}), + ] +) +def test_validate_fields(validator, msg): + with pytest.raises((InvalidRequestException, InvalidParamsException)): + validator.origin_validate_fields(msg) + + +@pytest.mark.parametrize( + 'msg', + [ + {}, + make_origin_parameters({'version': '0x1'}), + make_origin_parameters({'to': 'hxFF'}), + make_origin_parameters({'value': '0x001'}) + ] +) +def test_validate_request_execute(validator, msg): + with pytest.raises((InvalidParamsException, InvalidRequestException)): + validator.origin_request_execute(msg, 12) + + +@pytest.mark.parametrize( + 'revision, expected', + [ + (3, None), + (11, None), + (9, None) + ] +) +def test_execute_revision(validator, revision, expected): + assert validator.origin_request_execute( + make_origin_parameters(), revision + ) == expected \ No newline at end of file