From b5cc44e14522ede88f538a6257a6b7e06798c2ad Mon Sep 17 00:00:00 2001 From: Liran Cohen Date: Mon, 8 Jun 2026 02:47:33 +0000 Subject: [PATCH] fix(scan): reject non-canonical operation counts (#99) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding #15. ExtractFromScript parsed the anchor-string op count with strconv.Atoi (only err/negative checked). The Sidetree reference accepts only /^[1-9][0-9]*$/; Atoi also accepts "+5", "007", "0", so ion-node indexed operations from crafted anchors canonical ION ignores — silently diverging DID resolution. - internal/scan/opreturn.go: canonicalCount(s) enforces ^[1-9][0-9]*$ (positive decimal, no sign, no leading zeros, no overflow), replacing the lenient Atoi. Also rejects "0"-count anchors at scan time (they no longer consume a per-block tx-cap slot — partial overlap with #26). - internal/scan/opreturn_test.go: TestExtractCanonicalOpCount table-tests accepted (5/1/100/10000) vs rejected (0, 007, +5, -5, whitespace, 0x5, overflow, empty). go test -race ./... green. Co-authored-by: Liran Cohen Co-authored-by: Claude Opus 4.8 (1M context) --- docs/hardening-backlog.md | 2 +- internal/scan/opreturn.go | 26 +++++++++++++++++++++-- internal/scan/opreturn_test.go | 39 ++++++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/docs/hardening-backlog.md b/docs/hardening-backlog.md index bee779f..c236f93 100644 --- a/docs/hardening-backlog.md +++ b/docs/hardening-backlog.md @@ -42,7 +42,7 @@ All items below survived an independent skeptic re-reading the cited code. | 12 | 🟡 medium | medium | data-integrity | `retry-double-anchor-double-pay` | open | | 13 | 🟡 medium | medium | data-integrity | `writer-check-failopen-permanent` | open | | 14 | 🟡 medium | small | operational | `no-metrics-no-pending-growth-visibility` | open | -| 15 | 🟡 medium | trivial | correctness | `scan-operation-count-non-canonical-accepted` | open | +| 15 | 🟡 medium | trivial | correctness | `scan-operation-count-non-canonical-accepted` | ✅ done (#99) | | 16 | 🟡 medium | medium | operational | `concurrency-flag-dead-block-download-sequential` | open | | 17 | 🟡 medium | small | test-gap | `test-gap-cas-error-classification` | ✅ done (#87) | | 18 | 🟡 medium | medium | reliability | `no-rbf-no-fee-bump-stuck-tx` | open | diff --git a/internal/scan/opreturn.go b/internal/scan/opreturn.go index c14bcf8..9eeccd6 100644 --- a/internal/scan/opreturn.go +++ b/internal/scan/opreturn.go @@ -66,8 +66,8 @@ func (s *Scanner) ExtractFromScript(pkScript []byte) (operations.AnchorString, b } countStr, cid := payload[:dot], payload[dot+1:] - n, err := strconv.Atoi(countStr) - if err != nil || n < 0 { + n, ok := canonicalCount(countStr) + if !ok { return "", false } if !validCID(cid) { @@ -83,6 +83,28 @@ func (s *Scanner) ExtractFromScript(pkScript []byte) (operations.AnchorString, b return anchor, true } +// canonicalCount parses the operation-count field exactly as canonical ION does — +// the reference accepts only /^[1-9][0-9]*$/ (a positive decimal with no sign and no +// leading zeros). strconv.Atoi is too lenient (it accepts "+5", "007", "0"), so an +// ION node using it would index operations from crafted anchors that canonical ION +// ignores, silently diverging DID resolution. Returns ok=false on an empty string, +// a leading zero/sign/non-digit, or an int overflow. +func canonicalCount(s string) (int, bool) { + if len(s) == 0 || s[0] < '1' || s[0] > '9' { + return 0, false + } + for i := 1; i < len(s); i++ { + if s[i] < '0' || s[i] > '9' { + return 0, false + } + } + n, err := strconv.Atoi(s) + if err != nil { // a very long all-digit string overflows int + return 0, false + } + return n, true +} + // opReturnData returns the first data push of an OP_RETURN script. ok is false // for any script that does not begin with OP_RETURN or carries no data push. func opReturnData(pkScript []byte) ([]byte, bool) { diff --git a/internal/scan/opreturn_test.go b/internal/scan/opreturn_test.go index 72a3099..b15d616 100644 --- a/internal/scan/opreturn_test.go +++ b/internal/scan/opreturn_test.go @@ -151,3 +151,42 @@ func TestScanTxFirstValid(t *testing.T) { t.Errorf("ScanTx returned ops=%d, want 7 (first valid output)", a.Operations()) } } + +// TestExtractCanonicalOpCount guards finding #15: the operation-count field must +// match canonical ION's /^[1-9][0-9]*$/. strconv.Atoi's leniency ("+5", "007", "0") +// would index operations from crafted anchors canonical ION ignores, diverging DID +// resolution. +func TestExtractCanonicalOpCount(t *testing.T) { + s := New("ion:") + cases := []struct { + count string + wantOK bool + wantN int + }{ + {"5", true, 5}, + {"1", true, 1}, + {"100", true, 100}, + {"10000", true, 10000}, + {"0", false, 0}, // reference requires a leading [1-9] + {"007", false, 0}, // leading zeros + {"+5", false, 0}, // sign + {"-5", false, 0}, // sign + {" 5", false, 0}, // whitespace + {"5 ", false, 0}, + {"0x5", false, 0}, + {"99999999999999999999999999", false, 0}, // int overflow + {"", false, 0}, // empty count + } + for _, c := range cases { + t.Run(c.count, func(t *testing.T) { + script := opReturnScript(t, []byte("ion:"+c.count+"."+cidV0)) + a, ok := s.ExtractFromScript(script) + if ok != c.wantOK { + t.Fatalf("count %q: ok = %v, want %v", c.count, ok, c.wantOK) + } + if ok && a.Operations() != c.wantN { + t.Errorf("count %q: Operations() = %d, want %d", c.count, a.Operations(), c.wantN) + } + }) + } +}