diff --git a/AGENTS.md b/AGENTS.md index 56a9ade4faf..15435846d00 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -95,9 +95,13 @@ make fmt # Format all code ## Gno Security Semantics - Before writing or reviewing any caller-authentication, access-control, or cross-realm code in Gno (`/r/`, `/p/`, `/e/` packages), read `docs/resources/gno-interrealm.md`. Do not pattern-match from Solidity `msg.sender` or other-language intuition. +- For the complete AI-focused checklist (10 cases), read `docs/resources/gno-ai-contract-review.md` before reviewing any realm. - `runtime.PreviousRealm()` only shifts on explicit cross-calls (`fn(cross, ...)`) into crossing functions (`func fn(cur realm, ...){...}`). A `PreviousRealm().PkgPath() == "..."` check inside a non-crossing function does not identify the immediate caller and is a security bug. -- When editing a realm that accepts payment via `banker.OriginSend()`, the caller guard must be `runtime.PreviousRealm().IsUserCall()`, not `IsUser()`. `IsUser()` accepts `maketx run` ephemeral realms, which can consume the origin-send envelope before calling your function and bypass the payment check. See [docs/resources/effective-gno.md § Verifying inbound Coin payments](docs/resources/effective-gno.md#verifying-inbound-coin-payments). +- In crossing functions (`func F(cur realm, ...)`), **always** use `cur.IsCurrent()` before calling `cur.Previous()`. Never use `chain/runtime/unsafe.PreviousRealm()` in a crossing function — it bypasses the frame verification that `cur.IsCurrent()` provides. Any import of `chain/runtime/unsafe` alongside `cur realm` parameters is a red flag. +- When editing a realm that accepts payment via `banker.OriginSend()`, the caller guard must be `cur.Previous().IsUserCall()`, not `IsUser()`. `IsUser()` accepts `maketx run` ephemeral realms, which can consume the origin-send envelope before calling your function and bypass the payment check. See [docs/resources/effective-gno.md § Verifying inbound Coin payments](docs/resources/effective-gno.md#verifying-inbound-coin-payments). - When you see an existing realm using `IsUser()` plus `banker.OriginSend()`, flag it and cross-check nearby `OriginSend` usage. +- Never return a pointer to a `/p/`-type instance stored in realm state if that type has any exported mutation method (e.g. `avl.Tree.Set`, `avl.Tree.Remove`). Readonly taint does not block method dispatch — borrow rule #2 fires and the write commits under your realm's authority. Return values or narrow read-only views instead. +- `Render(path string)` receives attacker-controlled input. Never write path segments, user-supplied keys, or free-form string values directly into markdown output. Use `sanitize.InlineText` from `gno.land/p/nt/markdown/sanitize/v0` for inline content. --- diff --git a/docs/resources/gno-ai-contract-review.md b/docs/resources/gno-ai-contract-review.md new file mode 100644 index 00000000000..47d4599e840 --- /dev/null +++ b/docs/resources/gno-ai-contract-review.md @@ -0,0 +1,213 @@ +# Gno Contract Review Guide for AI Agents + +A concise reference for AI agents performing security review of `.gno` realm code. +For the full threat model and worked examples, see [`gno-security-guide.md`](./gno-security-guide.md). + +--- + +## Quick Checks + +These are the highest-yield issues to look for in any realm: + +### 1. Caller identity — use `cur realm`, not `address` parameters + +```go +// WRONG: address parameter is attacker-controlled +func AdminAction(caller address) { ... } + +// RIGHT: derive identity from the live crossing frame +func AdminAction(cur realm) { + if !cur.IsCurrent() { panic("spoofed realm") } + addr := cur.Previous().Address() + ... +} +``` + +### 2. Payment guards — `IsUserCall()`, not `IsUser()` + +```go +// WRONG: MsgRun ephemeral realms pass IsUser() +if !IsUser() { panic("not a user") } + +// RIGHT +if !cur.Previous().IsUserCall() { panic("not a direct user call") } +``` + +### 3. No exported pointers to mutable state + +```go +// WRONG: attacker can call mutator methods on returned pointer +func GetAccount() *Account { return gAccount } + +// RIGHT: return a copy, or expose read-only accessors +func GetBalance() int { return gAccount.balance } +``` + +### 4. No caller-supplied callbacks invoked with realm authority + +```go +// WRONG: if fn is a top-level /p/-declared function, it inherits +// the caller's m.Realm and can write to your state +func ApplyHook(fn func()) { fn() } + +// RIGHT: type the callback with your own /r/-declared type so +// /p/ code can't supply a matching implementation +func ApplyHook(fn func(*MyState)) { fn(gState) } +``` + +### 5. Interface parameters need canonical-type assertion + +```go +// WRONG: Evil{Teller} embedding bypasses interface checks +func DoBanking(t grc20.Teller) { t.Transfer(...) } + +// RIGHT: assert the concrete type before dispatch +func DoBanking(t grc20.Teller) { + if !grc20.IsCanonicalTeller(t) { panic("not a canonical Teller") } + t.Transfer(...) +} +``` + +### 6. Do not store `realm` values + +`realm` values are ephemeral — store `Address()` or `PkgPath()` strings instead. + +```go +// WRONG: panics at attach time +var savedRealm realm + +// RIGHT +var savedAddr address +func Save(cur realm) { savedAddr = cur.Previous().Address() } +``` + +### 7. `/p/`-type with callback iterators + +If a realm field is a `/p/`-type with methods like `Iterate(cb func(*Node) bool)`, +attackers can supply a top-level `/p/`-function that runs with your realm's authority. +Keep such fields unexported **and** do not return aliased pointers to them. + +### 8. `/p/`-type with mutation methods returned as pointer + +This is the subtlest case. A `/p/` library type whose fields are all **unexported** can +still be a write-authority leak if it has exported mutation methods and you return a pointer +to an instance stored in your realm. + +```go +// avl.Tree fields are all unexported — looks safe. +// But Tree has exported mutation methods: Set, Remove, etc. +var store = avl.NewTree() + +// WRONG: attacker calls store.Set(key, value) on the returned pointer. +// Borrow rule #2 fires (tree was allocated in this realm) → m.Realm = /r/V +// for the method body → the write inside Set commits under your authority. +func GetStore() *avl.Tree { return store } + +// RIGHT: never return the tree pointer. Expose only what you control. +func GetValue(key string) (any, bool) { return store.Get(key) } +``` + +The rule: **any exported method on a `/p/` type that writes to its receiver is a +mutator**. If you return a pointer to an instance, that mutator is now callable by +anyone with the authority of your realm. + +#### Sub-case: exported pointer fields + +The same path exists through exported pointer fields of `/p/` structs: + +```go +// p/mylib +type Container struct { + Items *avl.Tree // exported pointer field +} + +// r/V +var c = &Container{Items: avl.NewTree()} +func GetContainer() *Container { return c } + +// Attacker: c.Items.Set(key, value) → borrow rule #2 on Items +// (Items was allocated in /r/V) → write commits. +``` + +Readonly taint on `c` does NOT block this: method dispatch is not a write operation, +so the taint check does not fire. Borrow rule #2 fires first on method entry and +authorizes the writes inside the method body. + +**Rule**: treat every exported pointer field of a `/p/` type as if it were a direct +pointer to mutable state. If the pointed-to type has any mutation method, it is a +live mutator handle. Never return the containing struct as a pointer. + +### 9. `unsafe.PreviousRealm()` — old API, skips frame verification + +Using `chain/runtime/unsafe.PreviousRealm()` directly bypasses the `cur.IsCurrent()` +safety check. It should never appear alongside a `cur realm` parameter. + +```go +// WRONG: cur is accepted but ignored; no IsCurrent() guard +import "chain/runtime/unsafe" +func Set(cur realm, key, value string) { + caller := unsafe.PreviousRealm().Address() + ... +} + +// RIGHT +func Set(cur realm, key, value string) { + if !cur.IsCurrent() { panic("spoofed realm") } + caller := cur.Previous().Address() + ... +} +``` + +Flag any import of `chain/runtime/unsafe` in a realm that also has `cur realm` parameters. + +### 10. Unsanitized user input in `Render` + +`Render(path string)` receives attacker-controlled input. Writing path segments, keys, +or user-supplied values directly into markdown output enables injection (broken table +cells, injected links, heading overrides). + +```go +// WRONG: path, keys, and values written raw +func Render(path string) string { + return "# Vault: " + path + "\n" // heading injection +} + +// ALSO WRONG: table cell content not escaped +b.WriteString("| " + key + " | " + val + " |\n") // | in key breaks table + +// RIGHT: escape pipe characters at minimum; use sanitize.InlineText for +// full inline markdown escaping +import "gno.land/p/nt/markdown/sanitize/v0" +b.WriteString("| " + sanitize.InlineText(key) + " | " + sanitize.InlineText(val) + " |\n") +``` + +--- + +## Review Checklist + +- [ ] Authenticated mutators take `cur realm` and call `cur.IsCurrent()` +- [ ] No import of `chain/runtime/unsafe` alongside `cur realm` parameters +- [ ] Payment-guarded functions use `cur.Previous().IsUserCall()` +- [ ] No exported function returns a pointer to internal mutable state +- [ ] No exported function returns a `/p/`-type pointer whose type has mutation methods +- [ ] No exported `/p/`-struct field is itself a pointer to a type with mutation methods +- [ ] No method accepts a `func(...)` callback with a `/p/`-typed parameter and invokes it +- [ ] Interface parameters from external callers are guarded with canonical-type asserts +- [ ] No `realm`-typed value in package-level vars, struct fields, or closure captures +- [ ] `/p/`-type fields with callback iterators are unexported +- [ ] Data types holding sensitive state are declared in this realm (`/r/`), not in shared `/p/` +- [ ] `Render` sanitizes path segments, keys, and user-supplied values before writing to output + +--- + +## Relationship to Other Docs + +| Resource | Purpose | +|----------|---------| +| [`gno-security-guide.md`](./gno-security-guide.md) | Deep technical explanation of the threat model, borrow rules, and anti-patterns | +| [`gno-security.md`](./gno-security.md) | Numbered threat-class taxonomy | +| [`gno-interrealm.md`](./gno-interrealm.md) | Cross-realm call mechanics (`cur realm`, `IsCurrent()`, borrow rules) | +| [`effective-gno.md`](./effective-gno.md) | Idiomatic Gno patterns including payment guards | +| `misc/audit-pattern-harness/` | Automated pattern detection tooling with sanitized fixtures | + +This guide distills the above into the shortest checklist that catches the most critical issues. diff --git a/docs/resources/gno-security-guide.md b/docs/resources/gno-security-guide.md index 0d62083e246..8f4154b2573 100644 --- a/docs/resources/gno-security-guide.md +++ b/docs/resources/gno-security-guide.md @@ -219,6 +219,57 @@ back to `/r/V`, and the write commits. **Rule**: getters return either values (copies), unexported method results, or read-only views. Never a pointer to internal mutable state. +#### 5.1a `/p/`-type with unexported fields but exported mutation methods + +The taint-bypasses-method-dispatch gap applies even when the `/p/`-type +has *no exported fields at all*. If every field is unexported but the +type has exported methods that mutate the receiver, returning a pointer +to an instance stored in your realm is equivalent to publishing those +mutators as your own API. + +`avl.Tree` is the canonical example. All fields (`root`, `size`) are +unexported — a naive reviewer sees no exposed state. But `Tree.Set`, +`Tree.Remove`, and `Tree.ReverseIterate` all mutate or traverse the +tree. An attacker who receives a `*avl.Tree` pointer can call +`tree.Set(key, value)`, borrow rule #2 fires (the tree was allocated +in `/r/V`), and the write commits under victim authority. + +```go +var store = avl.NewTree() + +// WRONG — all fields unexported, but exported methods are mutators +func GetStore() *avl.Tree { return store } + +// Attacker: GetStore().Set("k", "injected") → commits under /r/V +``` + +#### 5.1b Exported pointer fields on `/p/` structs + +The same path exists one level of indirection deeper. If a `/p/` struct +has an exported field that is itself a pointer type whose type has +mutation methods, returning a pointer to the containing struct gives +indirect access to that inner mutator. + +```go +// p/mylib +type Container struct { + Items *avl.Tree // exported pointer field — mutation methods reachable + Label string +} + +// r/V +var c = &Container{Items: avl.NewTree()} +func GetContainer() *Container { return c } + +// Attacker: GetContainer().Items.Set(key, value) +// Readonly taint on c does NOT block method dispatch. +// Borrow rule #2 fires on Items (allocated in /r/V) → write commits. +``` + +**Rule**: treat every exported pointer field of a `/p/` type as a live +mutator handle if the pointed-to type has any mutation method. Never +return the containing struct as a pointer. + ### 5.2 Embedding a `/p/`-type with concrete-callback higher-order methods The (B)-class vector. Even if your container is `/r/`-declared, if @@ -327,6 +378,40 @@ a call frame`. **Rule**: if you need to remember a caller across transactions, store the `Address()` or `PkgPath()` (plain strings), not the realm value. +### 5.8 `unsafe.PreviousRealm()` alongside a `cur realm` parameter + +`chain/runtime/unsafe.PreviousRealm()` is the pre-`cur realm` API for +obtaining the previous realm. Using it in a crossing function that already +receives `cur realm` is always wrong: it bypasses the `IsCurrent()` frame +verification that makes `cur.Previous()` safe, and silently ignores the +`cur` capability token the runtime minted for exactly this purpose. + +```go +// WRONG: cur is accepted but never used; no IsCurrent() guard +import "chain/runtime/unsafe" + +func Set(cur realm, key, value string) { + caller := unsafe.PreviousRealm().Address() // skips frame check + ... +} + +// RIGHT +func Set(cur realm, key, value string) { + if !cur.IsCurrent() { panic("spoofed realm") } + caller := cur.Previous().Address() + ... +} +``` + +Any import of `chain/runtime/unsafe` in a realm that also declares +crossing functions (`func F(cur realm, ...)`) is a red flag. The +`unsafe` package is appropriate only in non-crossing helpers or +in realms that have not yet been migrated to the `cur realm` API. + +**Rule**: in crossing functions, always derive caller identity from +`cur.Previous()` under a `cur.IsCurrent()` guard. Delete the +`chain/runtime/unsafe` import. + --- ## 6. Properties That Make the Boundary Stronger Than Expected