From 43008dd08eb4c8fad05e8d2b3786ea87dcf478fe Mon Sep 17 00:00:00 2001 From: Alex Beregszaszi Date: Thu, 17 Jan 2019 22:08:05 +0000 Subject: [PATCH] Validate value types in decoder of ABIEncoderV2. --- Changelog.md | 1 + libsolidity/codegen/ABIFunctions.cpp | 107 ++++++++++++++------- libsolidity/codegen/ABIFunctions.h | 14 ++- libsolidity/codegen/ExpressionCompiler.cpp | 47 ++++++--- test/libsolidity/ABIDecoderTests.cpp | 21 +++- 5 files changed, 135 insertions(+), 55 deletions(-) diff --git a/Changelog.md b/Changelog.md index e11faaf4d..6b000b55b 100644 --- a/Changelog.md +++ b/Changelog.md @@ -5,6 +5,7 @@ Language Features: Compiler Features: + * ABI Decoder: Raise a runtime error on dirty inputs when using the experimental decoder. * SMTChecker: Support arithmetic compound assignment operators. * Optimizer: Add rule for shifts by constants larger than 255 for Constantinople. * Optimizer: Add rule to simplify certain ANDs and SHL combinations diff --git a/libsolidity/codegen/ABIFunctions.cpp b/libsolidity/codegen/ABIFunctions.cpp index 7288b143c..d5a5dde0e 100644 --- a/libsolidity/codegen/ABIFunctions.cpp +++ b/libsolidity/codegen/ABIFunctions.cpp @@ -255,10 +255,9 @@ string ABIFunctions::EncodingOptions::toFunctionNameSuffix() const return suffix; } - -string ABIFunctions::cleanupFunction(Type const& _type, bool _revertOnFailure) +string ABIFunctions::cleanupFunction(Type const& _type) { - string functionName = string("cleanup_") + (_revertOnFailure ? "revert_" : "assert_") + _type.identifier(); + string functionName = string("cleanup_") + _type.identifier(); return createFunction(functionName, [&]() { Whiskers templ(R"( function (value) -> cleaned { @@ -269,7 +268,7 @@ string ABIFunctions::cleanupFunction(Type const& _type, bool _revertOnFailure) switch (_type.category()) { case Type::Category::Address: - templ("body", "cleaned := " + cleanupFunction(IntegerType(160), _revertOnFailure) + "(value)"); + templ("body", "cleaned := " + cleanupFunction(IntegerType(160)) + "(value)"); break; case Type::Category::Integer: { @@ -291,6 +290,10 @@ string ABIFunctions::cleanupFunction(Type const& _type, bool _revertOnFailure) case Type::Category::FixedPoint: solUnimplemented("Fixed point types not implemented."); break; + case Type::Category::Function: + solAssert(dynamic_cast(_type).kind() == FunctionType::Kind::External, ""); + templ("body", "cleaned := " + cleanupFunction(FixedBytesType(24)) + "(value)"); + break; case Type::Category::Array: case Type::Category::Struct: case Type::Category::Mapping: @@ -319,20 +322,13 @@ string ABIFunctions::cleanupFunction(Type const& _type, bool _revertOnFailure) StateMutability::Payable : StateMutability::NonPayable ); - templ("body", "cleaned := " + cleanupFunction(addressType, _revertOnFailure) + "(value)"); + templ("body", "cleaned := " + cleanupFunction(addressType) + "(value)"); break; } case Type::Category::Enum: { - size_t members = dynamic_cast(_type).numberOfMembers(); - solAssert(members > 0, "empty enum should have caused a parser error."); - Whiskers w("if iszero(lt(value, )) { } cleaned := value"); - w("members", to_string(members)); - if (_revertOnFailure) - w("failure", "revert(0, 0)"); - else - w("failure", "invalid()"); - templ("body", w.render()); + // Out of range enums cannot be truncated unambigiously and therefore it should be an error. + templ("body", "cleaned := value " + validatorFunction(_type) + "(value)"); break; } case Type::Category::InaccessibleDynamic: @@ -346,6 +342,56 @@ string ABIFunctions::cleanupFunction(Type const& _type, bool _revertOnFailure) }); } +string ABIFunctions::validatorFunction(Type const& _type, bool _revertOnFailure) +{ + string functionName = string("validator_") + (_revertOnFailure ? "revert_" : "assert_") + _type.identifier(); + return createFunction(functionName, [&]() { + Whiskers templ(R"( + function (value) { + if iszero() { } + } + )"); + templ("functionName", functionName); + if (_revertOnFailure) + templ("failure", "revert(0, 0)"); + else + templ("failure", "invalid()"); + + switch (_type.category()) + { + case Type::Category::Address: + case Type::Category::Integer: + case Type::Category::RationalNumber: + case Type::Category::Bool: + case Type::Category::FixedPoint: + case Type::Category::Function: + case Type::Category::Array: + case Type::Category::Struct: + case Type::Category::Mapping: + case Type::Category::FixedBytes: + case Type::Category::Contract: + { + templ("condition", "eq(value, " + cleanupFunction(_type) + "(value))"); + break; + } + case Type::Category::Enum: + { + size_t members = dynamic_cast(_type).numberOfMembers(); + solAssert(members > 0, "empty enum should have caused a parser error."); + templ("condition", "lt(value, " + to_string(members) + ")"); + break; + } + case Type::Category::InaccessibleDynamic: + templ("condition", "1"); + break; + default: + solAssert(false, "Validation of type " + _type.identifier() + " requested."); + } + + return templ.render(); + }); +} + string ABIFunctions::cleanupFromStorageFunction(Type const& _type, bool _splitFunctionTypes) { solAssert(_type.isValueType(), ""); @@ -544,21 +590,6 @@ string ABIFunctions::conversionFunction(Type const& _from, Type const& _to) }); } -string ABIFunctions::cleanupCombinedExternalFunctionIdFunction() -{ - string functionName = "cleanup_combined_external_function_id"; - return createFunction(functionName, [&]() { - return Whiskers(R"( - function (addr_and_selector) -> cleaned { - cleaned := (addr_and_selector) - } - )") - ("functionName", functionName) - ("clean", cleanupFunction(FixedBytesType(24))) - .render(); - }); -} - string ABIFunctions::abiEncodingFunction( Type const& _from, Type const& _to, @@ -1248,7 +1279,7 @@ string ABIFunctions::abiEncodingFunctionFunctionType( } )") ("functionName", functionName) - ("cleanExtFun", cleanupCombinedExternalFunctionIdFunction()) + ("cleanExtFun", cleanupFunction(_to)) .render(); }); } @@ -1306,14 +1337,15 @@ string ABIFunctions::abiDecodingFunctionValueType(Type const& _type, bool _fromM return createFunction(functionName, [&]() { Whiskers templ(R"( function (offset, end) -> value { - value := ((offset)) + value := (offset) + (value) } )"); templ("functionName", functionName); templ("load", _fromMemory ? "mload" : "calldataload"); - // Cleanup itself should use the type and not decodingType, because e.g. + // Validation should use the type and not decodingType, because e.g. // the decoding type of an enum is a plain int. - templ("cleanup", cleanupFunction(_type, true)); + templ("validator", validatorFunction(_type, true)); return templ.render(); }); @@ -1560,11 +1592,11 @@ string ABIFunctions::abiDecodingFunctionFunctionType(FunctionType const& _type, { return Whiskers(R"( function (offset, end) -> addr, function_selector { - addr, function_selector := ((offset)) + addr, function_selector := ((offset, end)) } )") ("functionName", functionName) - ("load", _fromMemory ? "mload" : "calldataload") + ("decodeFun", abiDecodingFunctionFunctionType(_type, _fromMemory, false)) ("splitExtFun", m_utils.splitExternalFunctionIdFunction()) .render(); } @@ -1572,12 +1604,13 @@ string ABIFunctions::abiDecodingFunctionFunctionType(FunctionType const& _type, { return Whiskers(R"( function (offset, end) -> fun { - fun := ((offset)) + fun := (offset) + (fun) } )") ("functionName", functionName) ("load", _fromMemory ? "mload" : "calldataload") - ("cleanExtFun", cleanupCombinedExternalFunctionIdFunction()) + ("validateExtFun", validatorFunction(_type, true)) .render(); } }); diff --git a/libsolidity/codegen/ABIFunctions.h b/libsolidity/codegen/ABIFunctions.h index e700ae63a..079168720 100644 --- a/libsolidity/codegen/ABIFunctions.h +++ b/libsolidity/codegen/ABIFunctions.h @@ -130,9 +130,17 @@ private: /// @returns the name of the cleanup function for the given type and /// adds its implementation to the requested functions. + /// The cleanup function defers to the validator function with "assert" + /// if there is no reasonable way to clean a value. + std::string cleanupFunction(Type const& _type); + + /// @returns the name of the validator function for the given type and + /// adds its implementation to the requested functions. /// @param _revertOnFailure if true, causes revert on invalid data, /// otherwise an assertion failure. - std::string cleanupFunction(Type const& _type, bool _revertOnFailure = false); + /// + /// This is used for data decoded from external sources. + std::string validatorFunction(Type const& _type, bool _revertOnFailure = false); /// Performs cleanup after reading from a potentially compressed storage slot. /// The function does not perform any validation, it just masks or sign-extends @@ -146,10 +154,10 @@ private: /// @returns the name of the function that converts a value of type @a _from /// to a value of type @a _to. The resulting vale is guaranteed to be in range /// (i.e. "clean"). Asserts on failure. + /// + /// This is used for data being encoded or general type conversions in the code. std::string conversionFunction(Type const& _from, Type const& _to); - std::string cleanupCombinedExternalFunctionIdFunction(); - /// @returns the name of the ABI encoding function with the given type /// and queues the generation of the function to the requested functions. /// @param _fromStack if false, the input value was just loaded from storage diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp index fef178349..74cba80f3 100644 --- a/libsolidity/codegen/ExpressionCompiler.cpp +++ b/libsolidity/codegen/ExpressionCompiler.cpp @@ -1406,6 +1406,7 @@ bool ExpressionCompiler::visit(MemberAccess const& _memberAccess) case Type::Category::Struct: { StructType const& type = dynamic_cast(*_memberAccess.expression().annotation().type); + TypePointer const& memberType = _memberAccess.annotation().type; switch (type.location()) { case DataLocation::Storage: @@ -1418,7 +1419,7 @@ bool ExpressionCompiler::visit(MemberAccess const& _memberAccess) case DataLocation::Memory: { m_context << type.memoryOffsetOfMember(member) << Instruction::ADD; - setLValue(_memberAccess, *_memberAccess.annotation().type); + setLValue(_memberAccess, *memberType); break; } case DataLocation::CallData: @@ -1427,21 +1428,28 @@ bool ExpressionCompiler::visit(MemberAccess const& _memberAccess) { m_context << Instruction::DUP1; m_context << type.calldataOffsetOfMember(member) << Instruction::ADD; - CompilerUtils(m_context).accessCalldataTail(*_memberAccess.annotation().type); + CompilerUtils(m_context).accessCalldataTail(*memberType); } else { m_context << type.calldataOffsetOfMember(member) << Instruction::ADD; // For non-value types the calldata offset is returned directly. - if (_memberAccess.annotation().type->isValueType()) + if (memberType->isValueType()) { - solAssert(_memberAccess.annotation().type->calldataEncodedSize() > 0, ""); - CompilerUtils(m_context).loadFromMemoryDynamic(*_memberAccess.annotation().type, true, true, false); + solAssert(memberType->calldataEncodedSize() > 0, ""); + solAssert(memberType->storageBytes() <= 32, ""); + if (memberType->storageBytes() < 32 && m_context.experimentalFeatureActive(ExperimentalFeature::ABIEncoderV2)) + { + m_context << u256(32); + CompilerUtils(m_context).abiDecodeV2({memberType}, false); + } + else + CompilerUtils(m_context).loadFromMemoryDynamic(*memberType, true, true, false); } else solAssert( - _memberAccess.annotation().type->category() == Type::Category::Array || - _memberAccess.annotation().type->category() == Type::Category::Struct, + memberType->category() == Type::Category::Array || + memberType->category() == Type::Category::Struct, "" ); } @@ -1588,12 +1596,25 @@ bool ExpressionCompiler::visit(IndexAccess const& _indexAccess) { ArrayUtils(m_context).accessIndex(arrayType, true); if (arrayType.baseType()->isValueType()) - CompilerUtils(m_context).loadFromMemoryDynamic( - *arrayType.baseType(), - true, - !arrayType.isByteArray(), - false - ); + { + solAssert(arrayType.baseType()->storageBytes() <= 32, ""); + if ( + !arrayType.isByteArray() && + arrayType.baseType()->storageBytes() < 32 && + m_context.experimentalFeatureActive(ExperimentalFeature::ABIEncoderV2) + ) + { + m_context << u256(32); + CompilerUtils(m_context).abiDecodeV2({arrayType.baseType()}, false); + } + else + CompilerUtils(m_context).loadFromMemoryDynamic( + *arrayType.baseType(), + true, + !arrayType.isByteArray(), + false + ); + } else solAssert( arrayType.baseType()->category() == Type::Category::Struct || diff --git a/test/libsolidity/ABIDecoderTests.cpp b/test/libsolidity/ABIDecoderTests.cpp index 4cb41c87b..9eb0d3c4c 100644 --- a/test/libsolidity/ABIDecoderTests.cpp +++ b/test/libsolidity/ABIDecoderTests.cpp @@ -117,7 +117,7 @@ BOOST_AUTO_TEST_CASE(cleanup) ABI_CHECK( callContractFunction( "f(uint16,int16,address,bytes3,bool)", - u256(0xffffff), u256(0x1ffff), u256(-1), string("abcd"), u256(4) + u256(0xffffff), u256(0x1ffff), u256(-1), string("abcd"), u256(1) ), encodeArgs(u256(0xffff), u256(-1), (u256(1) << 160) - 1, string("abc"), true) ); @@ -759,7 +759,6 @@ BOOST_AUTO_TEST_CASE(complex_struct) ) } - BOOST_AUTO_TEST_CASE(return_dynamic_types_cross_call_simple) { if (m_evmVersion == langutil::EVMVersion::homestead()) @@ -844,6 +843,24 @@ BOOST_AUTO_TEST_CASE(return_dynamic_types_cross_call_out_of_range) ) } +BOOST_AUTO_TEST_CASE(out_of_bounds_bool_value) +{ + string sourceCode = R"( + contract C { + function f(bool b) public pure returns (bool) { return b; } + } + )"; + bool newDecoder = false; + BOTH_ENCODERS( + compileAndRun(sourceCode); + ABI_CHECK(callContractFunction("f(bool)", true), encodeArgs(true)); + ABI_CHECK(callContractFunction("f(bool)", false), encodeArgs(false)); + ABI_CHECK(callContractFunctionNoEncoding("f(bool)", bytes(32, 0)), encodeArgs(0)); + ABI_CHECK(callContractFunctionNoEncoding("f(bool)", bytes(32, 0xff)), newDecoder ? encodeArgs() : encodeArgs(1)); + newDecoder = true; + ) +} + BOOST_AUTO_TEST_SUITE_END() }