From a6a0271ae24bd824a30bafd0629e3fcb6f3602b7 Mon Sep 17 00:00:00 2001 From: Danno Ferrin Date: Thu, 1 Aug 2024 22:32:32 -0600 Subject: [PATCH] EOF validation updates (#7419) * Ensure forward calls stack height range is preserved. * add subcontainer and top container size checks Signed-off-by: Danno Ferrin --- .../besu/evmtool/PrettyPrintSubCommand.java | 5 +++++ .../besu/evmtool/pretty-print/dangling-data.json | 8 ++++++++ .../hyperledger/besu/evm/code/CodeV1Validation.java | 2 ++ .../org/hyperledger/besu/evm/code/EOFLayout.java | 7 +++++-- .../org/hyperledger/besu/evm/code/CodeV1Test.java | 9 +++++++-- .../org/hyperledger/besu/evm/code/EOFLayoutTest.java | 12 ++++++++++++ 6 files changed, 39 insertions(+), 4 deletions(-) create mode 100644 ethereum/evmtool/src/test/resources/org/hyperledger/besu/evmtool/pretty-print/dangling-data.json diff --git a/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/PrettyPrintSubCommand.java b/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/PrettyPrintSubCommand.java index 465f2c4895..2e1656b13d 100644 --- a/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/PrettyPrintSubCommand.java +++ b/ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/PrettyPrintSubCommand.java @@ -110,6 +110,11 @@ public class PrettyPrintSubCommand implements Runnable { if (validatedCode instanceof CodeInvalid codeInvalid) { parentCommand.out.println("EOF code is invalid - " + codeInvalid.getInvalidReason()); } + if (layout.container().size() != container.size()) { + parentCommand.out.println( + "EOF code is invalid - dangling data after container - " + + container.slice(layout.container().size()).toHexString()); + } } else { parentCommand.out.println("EOF layout is invalid - " + layout.invalidReason()); } diff --git a/ethereum/evmtool/src/test/resources/org/hyperledger/besu/evmtool/pretty-print/dangling-data.json b/ethereum/evmtool/src/test/resources/org/hyperledger/besu/evmtool/pretty-print/dangling-data.json new file mode 100644 index 0000000000..27833a1bb9 --- /dev/null +++ b/ethereum/evmtool/src/test/resources/org/hyperledger/besu/evmtool/pretty-print/dangling-data.json @@ -0,0 +1,8 @@ +{ + "cli": [ + "pretty-print", + "ef00010100040200010001040000000080000000ff" + ], + "stdin": "", + "stdout": "EOF layout is invalid - Dangling data after end of all sections\n" +} \ No newline at end of file diff --git a/evm/src/main/java/org/hyperledger/besu/evm/code/CodeV1Validation.java b/evm/src/main/java/org/hyperledger/besu/evm/code/CodeV1Validation.java index 3c85bd1eb2..f968bbf36a 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/code/CodeV1Validation.java +++ b/evm/src/main/java/org/hyperledger/besu/evm/code/CodeV1Validation.java @@ -486,6 +486,8 @@ public class CodeV1Validation implements EOFValidator { "Code that was not forward referenced in section 0x%x pc %d", codeSectionToValidate, currentPC); } + currentMin = min(stack_min[currentPC], currentMin); + currentMax = max(stack_max[currentPC], currentMax); if (stackInputs > currentMin) { return format( diff --git a/evm/src/main/java/org/hyperledger/besu/evm/code/EOFLayout.java b/evm/src/main/java/org/hyperledger/besu/evm/code/EOFLayout.java index 1249e475b4..39723f7fd3 100644 --- a/evm/src/main/java/org/hyperledger/besu/evm/code/EOFLayout.java +++ b/evm/src/main/java/org/hyperledger/besu/evm/code/EOFLayout.java @@ -191,6 +191,7 @@ public record EOFLayout( * strict and excess data is in the container * @return the eof layout */ + @SuppressWarnings("ReferenceEquality") public static EOFLayout parseEOF(final Bytes container, final boolean strictSize) { Queue parseQueue = new ArrayDeque<>(); parseQueue.add(new EOFParseStep(container, strictSize, -1, null, null)); @@ -213,8 +214,10 @@ public record EOFLayout( + " - " + parsedContainer.invalidReason); } - if (step.container.size() < parsedContainer.container.size()) { - return invalidLayout(container, parsedContainer.version, "excess data in subcontainer"); + // This ReferenceEquality check is correct + if ((strictSize || result != parsedContainer) + && step.container.size() != parsedContainer.container.size()) { + return invalidLayout(container, parsedContainer.version, "subcontainer size mismatch"); } if (step.index >= 0) { step.parentSubcontainers[step.index] = parsedContainer; diff --git a/evm/src/test/java/org/hyperledger/besu/evm/code/CodeV1Test.java b/evm/src/test/java/org/hyperledger/besu/evm/code/CodeV1Test.java index f32a3f4866..08cbbedf75 100644 --- a/evm/src/test/java/org/hyperledger/besu/evm/code/CodeV1Test.java +++ b/evm/src/test/java/org/hyperledger/besu/evm/code/CodeV1Test.java @@ -30,7 +30,7 @@ import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; /** - * These tests focus on code only validations, which are checked within the code runs themselves. + * These tests focus on code-only validations, which are checked within the code runs themselves. * Tests that depend on the EOF container (such as CallF into other sections) are in EOFLayoutTest. */ class CodeV1Test { @@ -520,7 +520,12 @@ class CodeV1Test { "Stack underflow", "Operation 0x50 requires stack of 1 but may only have 0 items", 0, - List.of(List.of("50 00", 0, 0x80, 1)))); + List.of(List.of("50 00", 0, 0x80, 1))), + Arguments.of( + "double rjumpi", + "Operation 0xF3 requires stack of 2 but may only have 1 items", + 0, + List.of(List.of("5f 5f e10005 5f 5f e10000 f3", 0, 0x80, 1)))); } static Stream stackRJumpForward() { diff --git a/evm/src/test/java/org/hyperledger/besu/evm/code/EOFLayoutTest.java b/evm/src/test/java/org/hyperledger/besu/evm/code/EOFLayoutTest.java index 145adfb2dd..006fe1632b 100644 --- a/evm/src/test/java/org/hyperledger/besu/evm/code/EOFLayoutTest.java +++ b/evm/src/test/java/org/hyperledger/besu/evm/code/EOFLayoutTest.java @@ -333,6 +333,18 @@ public class EOFLayoutTest { null, 1 }, + { + "EF00 01 010004 0200010001 0300010015 040000 00 00800000 00 (EF0001 010004 0200010001 040000 00 00800000 00ff)", + "dangling data in subcontainer", + "subcontainer size mismatch", + 1 + }, + { + "EF00 01 010004 0200010001 0300010014 040000 00 00800000 00 (EF0001 010004 0200010001 040000 00 00800000 00ff)", + "dangling data in container", + "Dangling data after end of all sections", + 1 + }, }); }