fix: bound and validate rule-embedded scripts

Signed-off-by: Joseph Kato <joseph@jdkato.io>

Author: jdkato

internal/check/metric.go

@@ -2,11 +2,13 @@ package check
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"regexp"
 	"strings"
 
 	"github.com/d5/tengo/v2"
+	"github.com/d5/tengo/v2/parser"
 	"github.com/d5/tengo/v2/stdlib"
 
 	"github.com/errata-ai/vale/v3/internal/core"
@@ -50,7 +52,12 @@ func NewMetric(_ *core.Config, generic baseCheck, path string) (Metric, error) {
 // Run calculates the readability level of the given text.
 func (o Metric) Run(_ nlp.Block, f *core.File, _ *core.Config) ([]core.Alert, error) {
 	alerts := []core.Alert{}
-	ctx := context.Background()
+
+	// A formula is compiled and run as a Tengo program, so it needs the same
+	// deadline a script rule gets; see tengoTimeout. Both evalMath calls share
+	// it, which bounds the rule as a whole rather than each half of it.
+	ctx, cancel := context.WithTimeout(context.Background(), tengoTimeout)
+	defer cancel()
 
 	parameters, err := f.ComputeMetrics()
 	if err != nil {
@@ -73,15 +80,17 @@ func (o Metric) Run(_ nlp.Block, f *core.File, _ *core.Config) ([]core.Alert, er
 	// We need this to allow showing the result in a rule's message.
 	res, err := evalMath(ctx, o.Formula, parameters)
 	if err != nil {
-		return alerts, core.NewE201FromTarget(err.Error(), "formula", o.path)
+		return alerts, ruleError(
+			o.Name, "formula", o.path, err, errors.Is(ctx.Err(), context.DeadlineExceeded))
 	}
 
 	// The binary result of our formula:
 	eqb := fmt.Sprintf("%f %s", res, o.Condition)
 
 	match, err := evalMath(ctx, eqb, parameters)
 	if err != nil {
-		return alerts, core.NewE201FromTarget(err.Error(), "condition", o.path)
+		return alerts, ruleError(
+			o.Name, "condition", o.path, err, errors.Is(ctx.Err(), context.DeadlineExceeded))
 	}
 
 	if match.(bool) {
@@ -105,6 +114,40 @@ func (o Metric) Pattern() string {
 	return o.Formula
 }
 
+// checkExpression rejects anything that is not a single expression.
+//
+// A rule's formula is pasted into a Tengo program by boilerplate above, and
+// `%s` escapes nothing. A formula that closes the parenthesis it was handed can
+// therefore append statements of its own, which turns a `metric` rule -- meant
+// to be arithmetic over a document's counts -- into arbitrary code running in
+// the same VM a `script` rule gets. `0); for { } ; x := (0` is the whole exploit,
+// and the same applies to `condition`, which is spliced after a number.
+//
+// Parsing the formula on its own settles it. An injection cannot survive the
+// trip: the `)` it depends on has no opener until the boilerplate supplies one,
+// so it fails to parse here, where it is still just a string.
+func checkExpression(expr string) error {
+	fileSet := parser.NewFileSet()
+	srcFile := fileSet.AddFile("expression", -1, len(expr))
+
+	parsed, err := parser.NewParser(srcFile, []byte(expr), nil).ParseFile()
+	if err != nil {
+		return fmt.Errorf("invalid expression %q: %w", expr, err)
+	}
+
+	if len(parsed.Stmts) != 1 {
+		return fmt.Errorf(
+			"expected a single expression, found %d statements in %q",
+			len(parsed.Stmts), expr)
+	}
+	if _, ok := parsed.Stmts[0].(*parser.ExprStmt); !ok {
+		return fmt.Errorf(
+			"expected an expression, found %T in %q", parsed.Stmts[0], expr)
+	}
+
+	return nil
+}
+
 func evalMath(
 	ctx context.Context,
 	expr string,
@@ -115,6 +158,10 @@ func evalMath(
 		return nil, fmt.Errorf("empty expression")
 	}
 
+	if err := checkExpression(expr); err != nil {
+		return nil, err
+	}
+
 	script := tengo.NewScript([]byte(fmt.Sprintf(boilerplate, expr)))
 	script.SetImports(stdlib.GetModuleMap("math"))
 

internal/check/metric_test.go

@@ -0,0 +1,97 @@
+package check
+
+import (
+	"context"
+	"strings"
+	"testing"
+	"time"
+)
+
+// A metric's formula is pasted into a Tengo program, so a formula that closes
+// the parenthesis it is handed can append statements and get the whole VM. The
+// timeout bounds how long that costs; this is what stops it being possible.
+func TestEvalMathRejectsInjectedStatements(t *testing.T) {
+	injections := []string{
+		// Closes the boilerplate's parenthesis, loops, reopens it.
+		"0); for { } ; x := (0",
+		// The same shape without the loop: still two statements smuggled in.
+		"0); x := 1; y := (0",
+		// A bare statement rather than an expression.
+		"x := 1",
+	}
+
+	for _, expr := range injections {
+		t.Run(expr, func(t *testing.T) {
+			_, err := evalMath(context.Background(), expr, map[string]interface{}{})
+			if err == nil {
+				t.Fatalf("%q was accepted", expr)
+			}
+			if strings.Contains(err.Error(), "deadline") {
+				t.Errorf("%q ran and was stopped by the timeout; it should not "+
+					"have compiled: %v", expr, err)
+			}
+		})
+	}
+}
+
+// The formulas that ship with Vale have to keep working, including the
+// multi-line ones and the ones calling into `math`.
+func TestEvalMathAcceptsRealFormulas(t *testing.T) {
+	params := map[string]interface{}{
+		"words": 100.0, "sentences": 10.0, "syllables": 150.0,
+		"long_words": 20.0, "polysyllabic_words": 5.0, "characters": 500.0,
+	}
+
+	formulas := []string{
+		"words / sentences",
+		"(words / sentences) + ((long_words * 100) / words)",
+		"(0.39 * (words / sentences)) + (11.8 * (syllables / words)) - 15.59",
+		"1.0430 * math.sqrt((polysyllabic_words * 30.0) / sentences) + 3.1291",
+		// The block-scalar forms arrive with surrounding whitespace.
+		"\n  words / sentences\n",
+	}
+
+	for _, expr := range formulas {
+		t.Run(strings.TrimSpace(expr), func(t *testing.T) {
+			if _, err := evalMath(context.Background(), expr, params); err != nil {
+				t.Errorf("rejected a valid formula: %v", err)
+			}
+		})
+	}
+}
+
+// `condition` is spliced in after the computed value, so it is the same hole
+// by another route and has to be closed by the same check.
+func TestEvalMathGuardsTheConditionPath(t *testing.T) {
+	// What Metric.Run builds: the result, then the rule's condition.
+	good := "12.500000 > 10"
+	if _, err := evalMath(context.Background(), good, map[string]interface{}{}); err != nil {
+		t.Errorf("rejected a valid condition: %v", err)
+	}
+
+	bad := "12.500000 > 0); for { } ; x := (0"
+	_, err := evalMath(context.Background(), bad, map[string]interface{}{})
+	if err == nil {
+		t.Fatal("an injected condition was accepted")
+	}
+	if strings.Contains(err.Error(), "deadline") {
+		t.Errorf("the injected condition ran: %v", err)
+	}
+}
+
+// Rejection has to happen before execution, not by running the program and
+// waiting for the deadline: a formula stopped by the timeout still ran.
+func TestEvalMathRejectsWithoutRunning(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), tengoTimeout)
+	defer cancel()
+
+	start := time.Now()
+	if _, err := evalMath(ctx, "0); for { } ; x := (0", map[string]interface{}{}); err == nil {
+		t.Fatal("expected the expression to be rejected")
+	}
+
+	if elapsed := time.Since(start); elapsed > time.Second {
+		t.Errorf("took %s, so it was executed and timed out rather than refused",
+			elapsed)
+	}
+}

internal/check/script.go

@@ -1,8 +1,12 @@
 package check
 
 import (
+	"context"
+	"errors"
+	"fmt"
 	"os"
 	"strings"
+	"time"
 
 	"github.com/d5/tengo/v2"
 	"github.com/d5/tengo/v2/stdlib"
@@ -11,6 +15,44 @@ import (
 	"github.com/errata-ai/vale/v3/internal/nlp"
 )
 
+// tengoTimeout bounds one execution of a rule's embedded program, for both the
+// `script` rules below and the `metric` formulas beside them.
+//
+// These are the two checks whose body is arbitrary code, and both usually
+// arrive inside a downloaded style package rather than being written by the
+// person running Vale. Restricting the imports -- see NewScript -- stops such a
+// program reaching the filesystem or the network, but says nothing about how
+// long it may take, and `for {}` compiles as readily as anything else. Without
+// a deadline that hangs Vale with no output and no error, which in CI looks
+// like the tool having crashed rather than a rule misbehaving.
+//
+// Generous on purpose: a script matches against a single block and a formula
+// evaluates once per file, so this sits orders of magnitude above what a
+// working rule needs and only a runaway one should ever reach it.
+const tengoTimeout = 2 * time.Second
+
+// ruleError reports a rule's runtime failure against the rule itself.
+//
+// The path alone identifies the file but not which check inside it stopped, and
+// a package may define several. The name is what appears in a rule's output and
+// in a user's config, so it is the handle they already have for switching the
+// thing off.
+//
+// A deadline is also restated: `context deadline exceeded` is Go's wording for
+// an internal mechanism, and a style author reading it has no reason to connect
+// it to a rule of theirs that never returns.
+func ruleError(name, field, path string, err error, timedOut bool) error {
+	msg := err.Error()
+	if timedOut {
+		msg = fmt.Sprintf("did not finish within %s", tengoTimeout)
+	}
+	if name != "" {
+		msg = name + ": " + msg
+	}
+
+	return core.NewE201FromTarget(msg, field, path)
+}
+
 // Script is Tango-based script.
 //
 // see https://github.com/d5/tengo.
@@ -85,8 +127,12 @@ func (s Script) Run(blk nlp.Block, _ *core.File, _ *core.Config) ([]core.Alert,
 		return alerts, core.NewE201FromTarget(err.Error(), "script", s.path)
 	}
 
-	if err := compiled.Run(); err != nil {
-		return alerts, core.NewE201FromTarget(err.Error(), "script", s.path)
+	ctx, cancel := context.WithTimeout(context.Background(), tengoTimeout)
+	defer cancel()
+
+	if err := compiled.RunContext(ctx); err != nil {
+		return alerts, ruleError(
+			s.Name, "script", s.path, err, errors.Is(ctx.Err(), context.DeadlineExceeded))
 	}
 
 	for _, match := range parseMatches(compiled.Get("matches").Array()) {

internal/check/script_test.go

@@ -0,0 +1,118 @@
+package check
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/d5/tengo/v2"
+	"github.com/d5/tengo/v2/stdlib"
+
+	"github.com/errata-ai/vale/v3/internal/nlp"
+)
+
+// compileScript builds a rule's program the way NewScript does, without the
+// config plumbing needed to read one off disk.
+func compileScript(t *testing.T, src string) *tengo.Compiled {
+	t.Helper()
+
+	program := tengo.NewScript([]byte(src))
+	program.SetImports(stdlib.GetModuleMap("text", "fmt", "math"))
+
+	if err := program.Add("scope", ""); err != nil {
+		t.Fatal(err)
+	}
+
+	compiled, err := program.Compile()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	return compiled
+}
+
+// A script rule is the only check whose body is arbitrary code, and it usually
+// arrives inside a downloaded style package. Denying it the `os` module bounds
+// what it can reach; this bounds how long it can take. Without the deadline
+// this test does not fail, it hangs.
+func TestScriptRunStopsAtTheTimeout(t *testing.T) {
+	s := Script{
+		compiled: compileScript(t, "matches := []\nfor { }"),
+		path:     "Runaway.yml",
+	}
+
+	start := time.Now()
+	_, err := s.Run(nlp.Block{Text: "some text to match against"}, nil, nil)
+	elapsed := time.Since(start)
+
+	if err == nil {
+		t.Fatal("a script that never returns should have been stopped")
+	}
+	if elapsed > tengoTimeout*3 {
+		t.Errorf("took %s to give up on a %s timeout", elapsed, tengoTimeout)
+	}
+}
+
+// A rule that runs away has to say which rule it was: the file may hold
+// several checks, and the name is the handle a user has for switching one off.
+// The deadline is restated for the same reason -- Go's own wording for it names
+// a mechanism the reader has never heard of.
+func TestScriptRunTimeoutNamesTheRule(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "Runaway.yml")
+
+	err := os.WriteFile(path,
+		[]byte("extends: script\nlevel: error\nscript: |\n  for { }\n"), 0o600)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	s := Script{compiled: compileScript(t, "matches := []\nfor { }"), path: path}
+	s.Name = "Runaway.Loop"
+
+	_, err = s.Run(nlp.Block{Text: "some text"}, nil, nil)
+	if err == nil {
+		t.Fatal("expected an error")
+	}
+
+	for _, want := range []string{"Runaway.Loop", "did not finish within"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("error is missing %q:\n%v", want, err)
+		}
+	}
+	if strings.Contains(err.Error(), "context deadline exceeded") {
+		t.Errorf("error still reports Go's internal wording:\n%v", err)
+	}
+}
+
+// The deadline must not cost a working rule its result.
+func TestScriptRunReturnsMatchesWithinTheTimeout(t *testing.T) {
+	src := `
+text := import("text")
+
+matches := []
+idx := text.index(scope, "storage")
+if idx >= 0 {
+	matches = append(matches, {begin: idx, end: idx + 7})
+}
+`
+
+	s := Script{
+		compiled: compileScript(t, src),
+		path:     "Storage.yml",
+	}
+
+	alerts, err := s.Run(nlp.Block{Text: "the storage layer"}, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(alerts) != 1 {
+		t.Fatalf("got %d alerts, want 1", len(alerts))
+	}
+	if got := alerts[0].Match; got != "storage" {
+		t.Errorf("matched %q, want %q", got, "storage")
+	}
+}