From 8cabf7ae6c1fc800050f4228a29e0e4442704f25 Mon Sep 17 00:00:00 2001 From: Benjamin Samuels <1222451+bsamuels453@users.noreply.github.com> Date: Thu, 20 Jun 2024 13:05:00 -0700 Subject: [PATCH 1/8] add kraken rules --- go/eth-rpc-tracetransaction.go | 15 ++++++++++++ go/eth-rpc-tracetransaction.yaml | 42 ++++++++++++++++++++++++++++++++ go/eth-txreceipt-status.go | 16 ++++++++++++ go/eth-txreceipt-status.yaml | 32 ++++++++++++++++++++++++ 4 files changed, 105 insertions(+) create mode 100644 go/eth-rpc-tracetransaction.go create mode 100644 go/eth-rpc-tracetransaction.yaml create mode 100644 go/eth-txreceipt-status.go create mode 100644 go/eth-txreceipt-status.yaml diff --git a/go/eth-rpc-tracetransaction.go b/go/eth-rpc-tracetransaction.go new file mode 100644 index 0000000..282f50d --- /dev/null +++ b/go/eth-rpc-tracetransaction.go @@ -0,0 +1,15 @@ +package main + +func Test() { + // ruleid: eth-rpc-tracetransaction + data, err := client.TraceTransaction(ctx, "hash", nil) + // ruleid: eth-rpc-tracetransaction + data, err := client.TraceBlockByNumber(ctx, 5, nil) + // ruleid: eth-rpc-tracetransaction + data, err := client.TraceBlockByHash(ctx, []byte{0x05}, nil) + // ruleid: eth-rpc-tracetransaction + data, err := client.TraceBlock(ctx, []byte{0x05}, nil) + // ruleid: eth-rpc-tracetransaction + data, err := client.TraceChain(ctx, 5, nil) + +} diff --git a/go/eth-rpc-tracetransaction.yaml b/go/eth-rpc-tracetransaction.yaml new file mode 100644 index 0000000..cb0a84b --- /dev/null +++ b/go/eth-rpc-tracetransaction.yaml @@ -0,0 +1,42 @@ +rules: + - id: eth-rpc-tracetransaction + message: >- + Using built-in transaction tracers can be dangerous if measures are not taken to filter out reverted call frames. + Review the related code to ensure the following properties: + 1. Reverted call frames and their associated subtraces are filtered out from any analysis. + 2. The transaction being traced is from a finalized block. + severity: WARNING + languages: [go] + pattern-either: + # Calls directly into Geth's API + - pattern: $RECEIVER.TraceTransaction($CTX, $FILTER, $TRACECONF) + - pattern: $RECEIVER.TraceBlockByNumber($CTX, $FILTER, $TRACECONF) + - pattern: $RECEIVER.TraceBlockByHash($CTX, $FILTER, $TRACECONF) + - pattern: $RECEIVER.TraceBlock($CTX, $FILTER, $TRACECONF) + - pattern: $RECEIVER.TraceChain($CTX, ...) + # RPC calls over HTTP API to geth/node provider + - pattern-regex: .*debug_traceBlock.* + - pattern-regex: .*debug_traceTransaction.* + - pattern-regex: .*debug_traceCall.* + - pattern-regex: .*debug_traceBlockByNumber.* + - pattern-regex: .*debug_traceBlockByHash.* + # RPC calls over HTTP API to non-geth client/node provider + - pattern-regex: .*trace_block.* + - pattern-regex: .*trace_transaction.* + - pattern-regex: .*trace_replayBlockTransactions.* + - pattern-regex: .*trace_replayTransaction.* + - pattern-regex: .*trace_filter.* + - pattern-regex: .*trace_call.* + - pattern-regex: .*trace_callMany.* + - pattern-regex: .*trace_get.* + metadata: + category: security + technology: [ethereum, blockchain, geth] + subcategory: [audit] + cwe: "CWE-1284: Improper Validation of Specified Quantity in Input" + confidence: LOW + impact: HIGH + likelihood: MEDIUM + description: Detects attempts to extract trace information from an EVM transaction or block + references: + - https://blog.trailofbits.com/2023/08/23/the-engineers-guide-to-blockchain-finality/ diff --git a/go/eth-txreceipt-status.go b/go/eth-txreceipt-status.go new file mode 100644 index 0000000..108cec2 --- /dev/null +++ b/go/eth-txreceipt-status.go @@ -0,0 +1,16 @@ +package main + +import ( + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/common/hexutil" + "github.com/ethereum/go-ethereum/crypto" + "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/rlp" +) + + +func Test() { + var debug Receipt + // ruleid: eth-txreceipt-status + a := debug.Status +} diff --git a/go/eth-txreceipt-status.yaml b/go/eth-txreceipt-status.yaml new file mode 100644 index 0000000..fb6cbcb --- /dev/null +++ b/go/eth-txreceipt-status.yaml @@ -0,0 +1,32 @@ +rules: + - id: eth-txreceipt-status + message: >- + A transaction receipt's status is inspected using `$RECEIVER.Status()`. For bridges and exchanges, this is a high-risk pattern because even though the transaction was successful, calls within the transaction may have failed. Review the related code to ensure the following properties: + 1. The receipt's success is not being used as a verification measure. + 2. The transaction being inspected is from a finalized block. + severity: WARNING + languages: [go] + patterns: + - pattern: | + import "github.com/ethereum/go-ethereum/core/types" + ... + $RECEIVER.$FUNCTION + - metavariable-pattern: + metavariable: $FUNCTION + pattern: Status + - focus-metavariable: $FUNCTION + metadata: + category: security + confidence: LOW + impact: HIGH + likelihood: MEDIUM + technology: + - ethereum + - blockchain + - geth + subcategory: + - audit + cwe: "CWE-437: Incomplete Model of Endpoint Features" + description: Detects when a transaction receipt's status is read + references: + - https://blog.trailofbits.com/2023/08/23/the-engineers-guide-to-blockchain-finality/ From cde90c806e39b57dedb1fa2addb51e317a690969 Mon Sep 17 00:00:00 2001 From: Benjamin Samuels <1222451+bsamuels453@users.noreply.github.com> Date: Thu, 20 Jun 2024 13:06:28 -0700 Subject: [PATCH 2/8] update readme --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 2d95df4..f5ef7a2 100644 --- a/README.md +++ b/README.md @@ -51,6 +51,8 @@ $ semgrep --config /path/to/semgrep-rules/hanging-goroutine.yml -o leaks.txt' | ID | Playground | Impact | Confidence | Description | | -- | :--------: | :----: | :--------: | ----------- | +| [eth-rpc-tracetransaction](go/eth-rpc-tracetransaction.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.eth-rpc-tracetransaction.eth-rpc-tracetransaction) | 🟥 | 🌕 | Detects attempts to extract trace information from an EVM transaction or block | +| [eth-txreceipt-status](go/eth-txreceipt-status.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.eth-txreceipt-status.eth-txreceipt-status) | 🟥 | 🌕 | Detects when a transaction receipt's status is read | | [hanging-goroutine](go/hanging-goroutine.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.hanging-goroutine.hanging-goroutine) | 🟩 | 🌗 | Goroutine leaks | | [invalid-usage-of-modified-variable](go/invalid-usage-of-modified-variable.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.invalid-usage-of-modified-variable.invalid-usage-of-modified-variable) | 🟧 | 🌘 | Possible unintentional assignment when an error occurs | | [iterate-over-empty-map](go/iterate-over-empty-map.yaml) | [🛝🔗](https://semgrep.dev/playground/r/trailofbits.go.iterate-over-empty-map.iterate-over-empty-map) | 🟩 | 🌗 | Probably redundant iteration over an empty map | From 104a852633faa1603b0535a244c15c367d74437f Mon Sep 17 00:00:00 2001 From: Benjamin Samuels <1222451+bsamuels453@users.noreply.github.com> Date: Thu, 20 Jun 2024 13:14:59 -0700 Subject: [PATCH 3/8] add negative tests --- go/eth-rpc-tracetransaction.go | 4 ++++ go/eth-txreceipt-status-negative.go | 7 +++++++ 2 files changed, 11 insertions(+) create mode 100644 go/eth-txreceipt-status-negative.go diff --git a/go/eth-rpc-tracetransaction.go b/go/eth-rpc-tracetransaction.go index 282f50d..41b89eb 100644 --- a/go/eth-rpc-tracetransaction.go +++ b/go/eth-rpc-tracetransaction.go @@ -12,4 +12,8 @@ func Test() { // ruleid: eth-rpc-tracetransaction data, err := client.TraceChain(ctx, 5, nil) + // ok: eth-rpc-tracetransaction + data, err := client.TraceSomething(ctx, 5, nil) + // ok: eth-rpc-tracetransaction + data, err := client.TraceTransaction(ctx, "hash") } diff --git a/go/eth-txreceipt-status-negative.go b/go/eth-txreceipt-status-negative.go new file mode 100644 index 0000000..d8d892c --- /dev/null +++ b/go/eth-txreceipt-status-negative.go @@ -0,0 +1,7 @@ +package main + + +func Test() { + // ok: eth-txreceipt-status + a := debug.Status +} From 17c678baa01caf062dbbd59253ebfa8564889835 Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Fri, 21 Jun 2024 11:29:16 +0200 Subject: [PATCH 4/8] more efficient eth-txreceipt-status.yaml --- go/eth-txreceipt-status.yaml | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/go/eth-txreceipt-status.yaml b/go/eth-txreceipt-status.yaml index fb6cbcb..0f397c9 100644 --- a/go/eth-txreceipt-status.yaml +++ b/go/eth-txreceipt-status.yaml @@ -7,14 +7,11 @@ rules: severity: WARNING languages: [go] patterns: - - pattern: | + - pattern-inside: | import "github.com/ethereum/go-ethereum/core/types" ... - $RECEIVER.$FUNCTION - - metavariable-pattern: - metavariable: $FUNCTION - pattern: Status - - focus-metavariable: $FUNCTION + - pattern: | + $RECEIVER.Status metadata: category: security confidence: LOW From 7c5b82d81bcf8ed097ab487d224ff70838fd546b Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Fri, 21 Jun 2024 11:34:31 +0200 Subject: [PATCH 5/8] clear formatting --- go/eth-rpc-tracetransaction.yaml | 24 +++++++++++++----------- go/eth-txreceipt-status.yaml | 21 +++++++++------------ 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/go/eth-rpc-tracetransaction.yaml b/go/eth-rpc-tracetransaction.yaml index cb0a84b..255cdc0 100644 --- a/go/eth-rpc-tracetransaction.yaml +++ b/go/eth-rpc-tracetransaction.yaml @@ -7,6 +7,18 @@ rules: 2. The transaction being traced is from a finalized block. severity: WARNING languages: [go] + metadata: + category: security + technology: [ethereum, blockchain, geth] + subcategory: [audit] + cwe: "CWE-1284: Improper Validation of Specified Quantity in Input" + confidence: LOW + impact: HIGH + likelihood: MEDIUM + description: Detects attempts to extract trace information from an EVM transaction or block + references: + - https://blog.trailofbits.com/2023/08/23/the-engineers-guide-to-blockchain-finality/ + pattern-either: # Calls directly into Geth's API - pattern: $RECEIVER.TraceTransaction($CTX, $FILTER, $TRACECONF) @@ -29,14 +41,4 @@ rules: - pattern-regex: .*trace_call.* - pattern-regex: .*trace_callMany.* - pattern-regex: .*trace_get.* - metadata: - category: security - technology: [ethereum, blockchain, geth] - subcategory: [audit] - cwe: "CWE-1284: Improper Validation of Specified Quantity in Input" - confidence: LOW - impact: HIGH - likelihood: MEDIUM - description: Detects attempts to extract trace information from an EVM transaction or block - references: - - https://blog.trailofbits.com/2023/08/23/the-engineers-guide-to-blockchain-finality/ + \ No newline at end of file diff --git a/go/eth-txreceipt-status.yaml b/go/eth-txreceipt-status.yaml index 0f397c9..2dc8933 100644 --- a/go/eth-txreceipt-status.yaml +++ b/go/eth-txreceipt-status.yaml @@ -6,24 +6,21 @@ rules: 2. The transaction being inspected is from a finalized block. severity: WARNING languages: [go] - patterns: - - pattern-inside: | - import "github.com/ethereum/go-ethereum/core/types" - ... - - pattern: | - $RECEIVER.Status metadata: category: security confidence: LOW impact: HIGH likelihood: MEDIUM - technology: - - ethereum - - blockchain - - geth - subcategory: - - audit + technology: [ethereum, blockchain, geth] + subcategory: [audit] cwe: "CWE-437: Incomplete Model of Endpoint Features" description: Detects when a transaction receipt's status is read references: - https://blog.trailofbits.com/2023/08/23/the-engineers-guide-to-blockchain-finality/ + + patterns: + - pattern-inside: | + import "github.com/ethereum/go-ethereum/core/types" + ... + - pattern: | + $RECEIVER.Status From 9b4abb8378f66e9f23a7ae9c4b93391d84c016c6 Mon Sep 17 00:00:00 2001 From: GrosQuildu Date: Fri, 21 Jun 2024 12:34:23 +0200 Subject: [PATCH 6/8] fix prettier --- go/eth-rpc-tracetransaction.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/go/eth-rpc-tracetransaction.yaml b/go/eth-rpc-tracetransaction.yaml index 255cdc0..c594ad6 100644 --- a/go/eth-rpc-tracetransaction.yaml +++ b/go/eth-rpc-tracetransaction.yaml @@ -41,4 +41,3 @@ rules: - pattern-regex: .*trace_call.* - pattern-regex: .*trace_callMany.* - pattern-regex: .*trace_get.* - \ No newline at end of file From 70d914b348e6d38fa002afa95a70bda5dcef217f Mon Sep 17 00:00:00 2001 From: Benjamin Samuels <1222451+bsamuels453@users.noreply.github.com> Date: Fri, 21 Jun 2024 08:18:38 -0700 Subject: [PATCH 7/8] add tests --- go/eth-rpc-tracetransaction.go | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/go/eth-rpc-tracetransaction.go b/go/eth-rpc-tracetransaction.go index 41b89eb..b61cbc9 100644 --- a/go/eth-rpc-tracetransaction.go +++ b/go/eth-rpc-tracetransaction.go @@ -1,19 +1,48 @@ package main +import ( + "fmt" + "io" + "net/http" + "strings" +) + func Test() { // ruleid: eth-rpc-tracetransaction data, err := client.TraceTransaction(ctx, "hash", nil) // ruleid: eth-rpc-tracetransaction data, err := client.TraceBlockByNumber(ctx, 5, nil) // ruleid: eth-rpc-tracetransaction - data, err := client.TraceBlockByHash(ctx, []byte{0x05}, nil) + data, err := client.TraceBlockByHash(ctx, []byte{0x05}, nil) // ruleid: eth-rpc-tracetransaction - data, err := client.TraceBlock(ctx, []byte{0x05}, nil) + data, err := client.TraceBlock(ctx, []byte{0x05}, nil) // ruleid: eth-rpc-tracetransaction data, err := client.TraceChain(ctx, 5, nil) + url := "https://eth-mainnet.g.alchemy.com/v2/docs-demo" + // ruleid: eth-rpc-tracetransaction + payload := strings.NewReader("{\"id\":1,\"jsonrpc\":\"2.0\",\"method\":\"trace_transaction\",\"params\":[\"0x8fc90a6c3ee3001cdcbbb685b4fbe67b1fa2bec575b15b0395fea5540d0901ae\"]}") + + req, _ := http.NewRequest("POST", url, payload) + + req.Header.Add("accept", "application/json") + req.Header.Add("content-type", "application/json") + + res, _ := http.DefaultClient.Do(req) + + defer res.Body.Close() + body, _ := io.ReadAll(res.Body) + + fmt.Println(string(body)) + // ok: eth-rpc-tracetransaction data, err := client.TraceSomething(ctx, 5, nil) // ok: eth-rpc-tracetransaction data, err := client.TraceTransaction(ctx, "hash") + // ok: eth-rpc-tracetransaction + data, err := client.TraceBlockByNumber(ctx, 5) + // ok: eth-rpc-tracetransaction + data, err := client.TraceBlockByHash(ctx, []byte{0x05}) + // ok: eth-rpc-tracetransaction + data, err := client.TraceBlock(ctx, []byte{0x05}) } From bd098af807f18a4a284652d921449616ab297630 Mon Sep 17 00:00:00 2001 From: Benjamin Samuels <1222451+bsamuels453@users.noreply.github.com> Date: Fri, 21 Jun 2024 08:27:04 -0700 Subject: [PATCH 8/8] add recommended changes --- go/eth-rpc-tracetransaction.yaml | 2 +- go/eth-txreceipt-status-negative.go | 7 ------- go/eth-txreceipt-status.go | 16 +++++++++------- go/eth-txreceipt-status.yaml | 2 +- 4 files changed, 11 insertions(+), 16 deletions(-) delete mode 100644 go/eth-txreceipt-status-negative.go diff --git a/go/eth-rpc-tracetransaction.yaml b/go/eth-rpc-tracetransaction.yaml index c594ad6..2bda9d5 100644 --- a/go/eth-rpc-tracetransaction.yaml +++ b/go/eth-rpc-tracetransaction.yaml @@ -15,7 +15,7 @@ rules: confidence: LOW impact: HIGH likelihood: MEDIUM - description: Detects attempts to extract trace information from an EVM transaction or block + description: Detects attempts to extract trace information from an EVM transaction or block. In exchange or bridge applications, extra logic must be implemented encapsulating these endpoints to prevent the values transferred during reverted call frames from being counted. references: - https://blog.trailofbits.com/2023/08/23/the-engineers-guide-to-blockchain-finality/ diff --git a/go/eth-txreceipt-status-negative.go b/go/eth-txreceipt-status-negative.go deleted file mode 100644 index d8d892c..0000000 --- a/go/eth-txreceipt-status-negative.go +++ /dev/null @@ -1,7 +0,0 @@ -package main - - -func Test() { - // ok: eth-txreceipt-status - a := debug.Status -} diff --git a/go/eth-txreceipt-status.go b/go/eth-txreceipt-status.go index 108cec2..af39fd2 100644 --- a/go/eth-txreceipt-status.go +++ b/go/eth-txreceipt-status.go @@ -1,16 +1,18 @@ package main -import ( - "github.com/ethereum/go-ethereum/common" - "github.com/ethereum/go-ethereum/common/hexutil" - "github.com/ethereum/go-ethereum/crypto" - "github.com/ethereum/go-ethereum/core/types" - "github.com/ethereum/go-ethereum/rlp" -) +import "github.com/ethereum/go-ethereum/core/types" +type Thing struct { + Id Int + Status bool +} func Test() { var debug Receipt // ruleid: eth-txreceipt-status a := debug.Status + + var debug2 Thing + // ok: eth-txreceipt-status + b := debug2.Status } diff --git a/go/eth-txreceipt-status.yaml b/go/eth-txreceipt-status.yaml index 2dc8933..0c6487a 100644 --- a/go/eth-txreceipt-status.yaml +++ b/go/eth-txreceipt-status.yaml @@ -23,4 +23,4 @@ rules: import "github.com/ethereum/go-ethereum/core/types" ... - pattern: | - $RECEIVER.Status + ($RECEIVER : Receipt).Status