diff --git a/CHANGELOG.md b/CHANGELOG.md index 832c462..fd8a66f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,22 @@ because it turns other people's test suites red. ### Security +- **A recipe that nests lists deeply is refused before it is read, whichever way + it is written.** The check that already refused deeply nested brackets counted + only brackets, and the same nesting written as `- - - - x` costs two bytes a + level and carries none. + + Measured against the previous build. A 500 kB recipe of nothing but dashes ran + for 88 seconds and ended in `fatal error: out of memory` with a page of Go + internals on standard error, leaving with the exit code a mistyped flag gets. + At 1 MB - the largest recipe the size limit allows - it ran for 70 seconds and + said nothing at all. The same file is now refused in under a second, with a + message naming the depth and the limit. + + The limit counts how deeply lists and mappings nest, in either style, and it + is 32. An ordinary recipe reaches one, and an archive declaring what it + contains reaches two. + - **A file is never written under a name something else already holds.** Every file this tool writes goes to a temporary name first and is renamed into place. Three of those temporary names were created in a way that follows a diff --git a/internal/cli/errors.go b/internal/cli/errors.go index d14c496..71fdd63 100644 --- a/internal/cli/errors.go +++ b/internal/cli/errors.go @@ -146,9 +146,11 @@ func classifyRequest(err error) (int, bool) { if errors.As(err, &recipeTooLarge) { return ExitRecipe, true } - // Same class, one more shape: a recipe whose brackets nest far enough to - // exhaust this machine. A fact about the document rather than a fault of - // ours, so it ends the way a recipe that will not parse does. + // Same class, one more shape: a recipe whose collections nest far enough to + // exhaust this machine - in either style, since 2026-09-06, because block + // lists reach the same depth in two bytes a level and carry no bracket. A + // fact about the document rather than a fault of ours, so it ends the way a + // recipe that will not parse does. var recipeTooDeep *recipe.TooDeepError if errors.As(err, &recipeTooDeep) { return ExitRecipe, true diff --git a/internal/guard/recipebombs_test.go b/internal/guard/recipebombs_test.go index 16981b9..9d0fea2 100644 --- a/internal/guard/recipebombs_test.go +++ b/internal/guard/recipebombs_test.go @@ -86,4 +86,56 @@ func TestAHostileRecipeCannotHangTheReader(t *testing.T) { t.Fatalf("a value holding brackets was refused as if they nested: %v", err) } }) + + t.Run("the same nesting written in block style is refused too", func(t *testing.T) { + // "- - - - x" is four nested sequences in eight bytes and holds not one + // bracket, so the limit that counted brackets walked straight past it. + // Measured on 2026-09-06 against the shipped binary, before this was + // counted: 500 kB of it took 88 seconds and ended in "fatal error: out + // of memory" with 41 kB of Go stack on standard error, and 1 MB - the + // most the size limit allows - left with exit 1 and no output at all. + // After: 756 ms and this refusal. + src := []byte(strings.Repeat("- ", 20000) + "x") + _, err := recipe.Parse(src, "blockbomb.yaml") + var deep *recipe.TooDeepError + if !errors.As(err, &deep) { + t.Fatalf("a block style bomb was answered with %v, expected a refusal about its depth", err) + } + }) + + t.Run("a long list at one level is not nesting", func(t *testing.T) { + // Every dash in the same column is the next entry of one sequence + // rather than a sequence inside the last one. Anything counting dashes + // instead of nesting refuses this at the thirty third target, which is + // an ordinary recipe. + var b strings.Builder + b.WriteString("version: 1\nseed: 7\noutput:\n dir: out\ntargets:\n") + for i := 0; i < 200; i++ { + b.WriteString(" - id: t") + b.WriteString(strconv.Itoa(i)) + b.WriteString("\n format: txt\n size: 100\n") + } + if _, err := recipe.Parse([]byte(b.String()), "long.yaml"); err != nil { + t.Fatalf("a recipe with 200 targets in one list was refused: %v", err) + } + }) + + t.Run("a list inside a list is two, not two hundred", func(t *testing.T) { + // The deepest shape this tool actually produces: targets, and an + // archive declaring what it contains. Two levels, whatever the length + // of either list. + var b strings.Builder + b.WriteString("version: 1\nseed: 7\noutput:\n dir: out\ntargets:\n") + for i := 0; i < 50; i++ { + b.WriteString(" - id: a") + b.WriteString(strconv.Itoa(i)) + b.WriteString("\n format: zip\n contains:\n") + for j := 0; j < 20; j++ { + b.WriteString(" - format: txt\n count: 1\n size: 8kb\n") + } + } + if _, err := recipe.Parse([]byte(b.String()), "nested.yaml"); err != nil { + t.Fatalf("an archive declaring its contents was refused: %v", err) + } + }) } diff --git a/internal/recipe/limits.go b/internal/recipe/limits.go index 65347f2..c417faf 100644 --- a/internal/recipe/limits.go +++ b/internal/recipe/limits.go @@ -25,30 +25,53 @@ import ( // apparent cliff sat exactly there - 1900 pairs produce a 4010 B message and // pass, 1950 produce 4110 B and blocked. The budget was taken back out, since // nothing could have made it fire. - +// +// THE FIRST VERSION OF THIS LIMIT COUNTED THE WRONG THING, and that cost more +// than the bomb it was written for. It counted flow collections - the ones +// written with brackets and braces - because that is how the bomb of +// 2026-09-02 was written. The same nesting written in block style walks past +// it: "- - - - x" is four nested sequences in eight bytes, and the lexer calls +// none of them a bracket. Measured on 2026-09-06 against the shipped binary, +// with tfg validate: +// +// "- " x20 000 40 kB 0.40 s refused, by a limit inside the parser +// "- " x100 000 200 kB 7.41 s refused +// "- " x250 000 500 kB 88.2 s fatal error: out of memory, exit 2 +// "- " x500 000 1 MB 70.7 s exit 1, and not one word of output +// +// The third of those printed 41 kB of Go runtime stack to standard error and +// left with the exit code the frozen table gives a MISTYPED FLAG. The machine +// it did that on has 31.9 GB. +// +// THE COST IS THE NESTING, NOT THE SIZE, and that is measured rather than +// assumed: 500 kB of flat mapping - a hundred thousand lines of "a: 1" - is +// refused cleanly in 0.62 s. So a limit on bytes could never have answered +// this, and neither could one on how many tokens a document holds. const ( - // MaxFlowDepth is how far flow collections - the ones written with - // brackets and braces - may nest. + // MaxNestingDepth is how far collections may nest, in any style. // - // This is the deterministic half, and it is deterministic because the - // lexer has already decided what is a bracket and what is a character - // inside a quoted value. Measured across shapes on 2026-09-02: + // This is the deterministic half, and it is deterministic because the lexer + // has already decided what opens a collection and what is a character + // inside a quoted value. Measured across shapes on 2026-09-02 and again on + // 2026-09-06 with tools/probes/yamldepth: // // block style, a thousand targets 0 // forty thousand brackets inside a string 0 // spread: [1B, 1kb, 1mb] 1 // a thousand targets written in flow style 1 - // the nesting bomb 20000 + // an ordinary recipe 1 + // the flow bomb 20000 + // the block bomb 500000 // - // So one is what real recipes reach and thirty two is far above anything - // a person writes. Counting rather than guessing also rules out the two + // So one is what real recipes reach and thirty two is far above anything a + // person writes. Counting rather than guessing also rules out the two // obvious mistakes: a bracket inside a quoted value is not a collection, // and neither is one inside a comment. - MaxFlowDepth = 32 + MaxNestingDepth = 32 ) -// TooDeepError is returned for a recipe whose flow collections nest past -// MaxFlowDepth. +// TooDeepError is returned for a recipe whose collections nest past +// MaxNestingDepth. type TooDeepError struct { Name string Depth int @@ -56,11 +79,12 @@ type TooDeepError struct { func (e *TooDeepError) Error() string { return fmt.Sprintf( - "%s nests brackets and braces %d deep and the limit is %d. Reading a deeply nested document costs memory that grows far faster than the document does, so a small file can exhaust this machine before anything is written. Write the targets out as an ordinary list instead", - e.Name, e.Depth, MaxFlowDepth) + "%s nests lists and mappings %d deep and the limit is %d. Reading a deeply nested document costs memory that grows far faster than the document does, so a small file can exhaust this machine before anything is written. Write the targets out as an ordinary list instead", + e.Name, e.Depth, MaxNestingDepth) } -// flowDepth is the deepest the flow collections in src nest. +// nestingDepth is the deepest the collections in src nest, counting both +// styles. // // The lexer is asked rather than the bytes, and that is the whole point of // doing it this way: it has already decided which brackets open a collection @@ -69,28 +93,77 @@ func (e *TooDeepError) Error() string { // depth nought, which a scan over the raw bytes could only manage by // reimplementing the quoting rules. // -// Cheap enough to run on every recipe: 0.002 s for a forty kilobyte document, -// 0.008 s for the bomb, and the cost grows with the input rather than with the -// shape - which is exactly what the parser underneath does not do. -func flowDepth(src []byte) int { - current, deepest := 0, 0 +// Cheap enough to run on every recipe, and this is the measurement the whole +// defence rests on, because it has to happen BEFORE the parser sees anything. +// Measured on 2026-09-06 with tools/probes/yamldepth, on the block bomb: +// +// 20 000 levels 40 kB 7 ms 5.5 MB +// 100 000 levels 200 kB 42 ms 28.9 MB +// 500 000 levels 1 MB 422 ms 145.2 MB +// +// The last line is the worst input the size limit allows, and it is what the +// parser answered with 70 seconds and no output at all. The memory is the +// tokeniser's own and it is bounded by MaxBytes, which is the same bargain this +// function has always made - it is not new work, only work that now counts one +// more thing. +func nestingDepth(src []byte) int { + flow, deepest := 0, 0 + + // The columns of the block sequences that are open. A dash further right + // than the innermost open one starts a sequence inside it, a dash at the + // same column is the next entry of that one, and a dash further left closes + // however many it has come back out of. That is the whole of block nesting + // as far as this needs to know. + var dashes []int + for _, t := range lexer.Tokenize(string(src)) { - current += depthChange(t.Type) - if current > deepest { - deepest = current + if t.Type == token.SequenceEntryType && flow == 0 && t.Position != nil { + dashes = openSequences(dashes, t.Position.Column) + } else { + flow += depthChange(t.Type) + } + if d := flow + len(dashes); d > deepest { + deepest = d } } return deepest } -// depthChange is what one token does to the nesting: a collection opening adds -// a level, one closing takes it away, and anything else leaves it where it was. +// openSequences is the block sequences still open after a dash at this column. +// +// A function of its own rather than the body of the loop above, for the reason +// written beside depthChange: together they nest three deep - the loop, the +// branch, the pop - and the shape guard counts how many functions sit that deep +// as well as how deep the deepest one is. +// +// WHAT THIS OVER-COUNTS, said out loud because a limit that hides its edges is +// worse than none. Two sequences that are siblings under different keys, the +// second indented further than the first, are read as nested: +// +// a: +// - x +// b: +// - y counted as depth two, and it is two sequences at depth one +// +// Reaching the limit that way needs thirty two keys each indented further than +// the one before, in one document, which is not a shape anybody writes and is +// not what our own canonical form produces. The alternative is to track where +// mappings open as well, which is more machinery for a case that costs a +// refusal naming the file rather than a wrong answer. +func openSequences(open []int, column int) []int { + for len(open) > 0 && open[len(open)-1] > column { + open = open[:len(open)-1] + } + if len(open) == 0 || open[len(open)-1] < column { + open = append(open, column) + } + return open +} + +// depthChange is what one token does to the flow nesting: a collection opening +// adds a level, one closing takes it away, and anything else leaves it where it +// was. // -// A function of its own rather than a switch inside the loop above, because -// together they nested three deep - the loop, the switch, the comparison - and -// the shape guard counts how many functions sit that deep as well as how deep -// the deepest one is. Splitting is what that guard asks for and it costs -// nothing here. // The default is not decoration. token.Type has thirty four members and a // switch on it without one is reported as incomplete, which is correct of the // linter and wrong about this function: everything that is not a bracket or a diff --git a/internal/recipe/recipe.go b/internal/recipe/recipe.go index f548b9c..cff7179 100644 --- a/internal/recipe/recipe.go +++ b/internal/recipe/recipe.go @@ -188,7 +188,7 @@ func Parse(src []byte, name string) (*Recipe, error) { // cost is and no amount of it can be given back afterwards. See limits.go // for the two shapes this and the budget below answer, and why one number // cannot answer both. - if depth := flowDepth(src); depth > MaxFlowDepth { + if depth := nestingDepth(src); depth > MaxNestingDepth { return nil, &TooDeepError{Name: name, Depth: depth} }