osctrl: follow-up fixes for #813 — per-service --auth defaults + carve path SQL escape

cmd/api/handlers/carves.go

@@ -250,15 +250,10 @@ func (h *HandlersApi) CarvesRunHandler(w http.ResponseWriter, r *http.Request) {
 		apiErrorResponse(w, "path can not be empty", http.StatusBadRequest, nil)
 		return
 	}
-	// Validate the path before it's spliced into the osquery SQL via
-	// carves.GenCarveQuery. Without this gate a CarveLevel operator
-	// could inject arbitrary osquery (e.g. `'; SELECT 1; --`) into the
-	// query that gets distributed to every targeted node — pivoting
-	// "carve a file" into "run any SELECT".
-	if !carves.ValidCarvePath(c.Path) {
-		apiErrorResponse(w, "invalid carve path", http.StatusBadRequest, fmt.Errorf("rejected path %q", c.Path))
-		return
-	}
+	// SQL injection on the carves.GenCarveQuery splice is defended at
+	// the splice site (single-quote escape, LIKE pattern escape) — no
+	// allowlist gate here so legitimate paths containing spaces or
+	// non-ASCII characters round-trip correctly.
 	// Make sure the user has permissions to run queries in the environments
 	for _, e := range c.Environments {
 		if !h.Users.CheckPermissions(ctx[ctxUser], users.QueryLevel, e) {

pkg/carves/utils.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"encoding/base64"
 	"fmt"
-	"regexp"
 	"strings"
 
 	"github.com/jmpsec/osctrl/pkg/utils"
@@ -79,38 +78,61 @@ func GenCarveName() string {
 	return "carve_" + utils.RandomForNames()
 }
 
-// validCarvePath restricts the characters that can appear in a carve
-// path. The carve string is concatenated into the osquery SQL that
-// every targeted node executes; without this gate a CarveLevel
-// operator could inject arbitrary osquery (e.g. `'; SELECT 1; --`) and
-// pivot from "exfil this path" to "run any SELECT against your nodes".
+// escapeSQLString returns s with every single quote doubled, so the
+// result is safe to interpolate inside a SQL string literal — osquery
+// (SQLite) follows the standard rule that '' inside a literal is one
+// literal quote, and there is no backslash escape to consider. This
+// is the SQL-injection defense for GenCarveQuery: a path containing
+// `'; DROP TABLE x; --` becomes `''; DROP TABLE x; --`, which the
+// parser sees as the contents of the string literal — there is no
+// way to escape the surrounding quotes.
+func escapeSQLString(s string) string {
+	return strings.ReplaceAll(s, "'", "''")
+}
+
+// escapeLikePattern returns s escaped for a SQL LIKE pattern, using
+// `\` as the escape character. Caller MUST emit the resulting query
+// with `ESCAPE '\'`. Order matters: escape the escape char first.
+func escapeLikePattern(s string) string {
+	s = strings.ReplaceAll(s, `\`, `\\`)
+	s = strings.ReplaceAll(s, `%`, `\%`)
+	s = strings.ReplaceAll(s, `_`, `\_`)
+	return s
+}
+
+// globToLike maps the carve-style globs `*` and `?` to LIKE wildcards
+// `%` and `_`, after escaping any literal LIKE metacharacters the
+// path may already contain, then escaping SQL string quotes. The
+// caller MUST emit the resulting pattern with `ESCAPE '\'`.
 //
-// The character class covers realistic carve targets across the three
-// platforms: absolute POSIX paths (Linux/macOS), Windows paths with
-// backslashes and drive letters, and glob wildcards (* and ?). It
-// explicitly excludes single quote, semicolon, and comment markers.
-var validCarvePath = regexp.MustCompile(`^[/A-Za-z0-9._\-\\:*?]+$`)
-
-// ValidCarvePath reports whether s is a safe value to splice into
-// GenCarveQuery. Callers MUST verify before calling GenCarveQuery —
-// the result is interpolated directly into SQL.
-func ValidCarvePath(s string) bool {
-	if s == "" {
-		return false
-	}
-	return validCarvePath.MatchString(s)
+// Order matters: LIKE-escape (which inserts `\`) runs before the
+// glob mapping (so existing `%`/`_` stay literal), and SQL-quote
+// escaping runs last so doubled quotes are not themselves escaped
+// by the LIKE pass.
+func globToLike(s string) string {
+	s = escapeLikePattern(s)
+	s = strings.ReplaceAll(s, "*", "%")
+	s = strings.ReplaceAll(s, "?", "_")
+	s = escapeSQLString(s)
+	return s
 }
 
-// Helper to generate the carve query.
+// GenCarveQuery builds the osquery SQL that selects matching `carves`
+// rows on every targeted node. The carve `file` is treated as an
+// untrusted SQL string literal: single quotes are doubled so a
+// CarveLevel operator cannot break out of the literal to pivot from
+// "carve this path" into arbitrary SELECTs (e.g. `'; SELECT 1; --`).
+//
+// In glob mode `*` and `?` map to LIKE wildcards `%` and `_`, with
+// any pre-existing `%`, `_`, or `\` in the path escaped via `ESCAPE '\'`
+// so they are treated as literals.
 //
-// `file` is interpolated into the SQL string verbatim. The caller MUST
-// have validated it via ValidCarvePath beforehand — passing an
-// unvalidated user-controlled value here lets the requesting operator
-// run arbitrary osquery on every targeted host, which is well beyond
-// the "carve a file" capability the endpoint advertises.
+// Paths containing spaces (e.g. `C:\Program Files\...`,
+// `/Library/Application Support/...`) and any UTF-8 characters are
+// supported.
 func GenCarveQuery(file string, glob bool) string {
 	if glob {
-		return "SELECT * FROM carves WHERE carve=1 AND path LIKE '" + file + "';"
+		return "SELECT * FROM carves WHERE carve=1 AND path LIKE '" + globToLike(file) + "' ESCAPE '\\';"
 	}
-	return "SELECT * FROM carves WHERE carve=1 AND path = '" + file + "';"
+	return "SELECT * FROM carves WHERE carve=1 AND path = '" + escapeSQLString(file) + "';"
 }

pkg/carves/utils_test.go

@@ -5,47 +5,108 @@ import (
 	"testing"
 )
 
-// TestValidCarvePath locks the character allowlist that gates GenCarveQuery.
-func TestValidCarvePath(t *testing.T) {
-	good := []string{
-		"/etc/passwd",
-		"/var/log/auth.log",
-		"C:\\Windows\\System32\\drivers\\etc\\hosts",
-		"/Users/alice/Library/Application_Support/com.example/cfg",
-		"/var/log/*.log",
-		"/var/log/auth?.log",
-	}
-	for _, p := range good {
-		if !ValidCarvePath(p) {
-			t.Errorf("ValidCarvePath(%q): expected true", p)
+// TestGenCarveQueryEscapes confirms that single quotes in the input
+// path are doubled in the SQL output, so a CarveLevel operator cannot
+// break out of the string literal and pivot to arbitrary osquery
+// (e.g. `'; SELECT 1; --`).
+func TestGenCarveQueryEscapes(t *testing.T) {
+	cases := []struct {
+		name    string
+		in      string
+		glob    bool
+		wantSub string
+	}{
+		{
+			name:    "exact: classic injection becomes literal",
+			in:      "'; SELECT 1; --",
+			glob:    false,
+			wantSub: "path = '''; SELECT 1; --'",
+		},
+		{
+			name:    "exact: single quote inside path is doubled",
+			in:      "/var/log/a'b",
+			glob:    false,
+			wantSub: "path = '/var/log/a''b'",
+		},
+		{
+			name:    "glob: injection in LIKE is also escaped",
+			in:      "'; DROP TABLE x; --",
+			glob:    true,
+			wantSub: "path LIKE '''; DROP TABLE x; --' ESCAPE '\\'",
+		},
+	}
+	for _, tc := range cases {
+		got := GenCarveQuery(tc.in, tc.glob)
+		if !strings.Contains(got, tc.wantSub) {
+			t.Errorf("%s: GenCarveQuery(%q, %v) = %q; want substring %q",
+				tc.name, tc.in, tc.glob, got, tc.wantSub)
+		}
+	}
+}
+
+// TestGenCarveQueryLegitimatePaths confirms paths that the old
+// regex-based gate would have rejected — Windows paths with spaces,
+// macOS Application Support paths, accented characters, parentheses —
+// all round-trip through the splice cleanly.
+func TestGenCarveQueryLegitimatePaths(t *testing.T) {
+	paths := []string{
+		`C:\Program Files\Common Files\app.log`,
+		`/Library/Application Support/com.example/cfg`,
+		`/home/álvaro/notes.txt`,
+		`/var/log/(archived)/old.log`,
+		`/var/log/auth.log`,
+		`C:\Users\bob\Documents\hello world.txt`,
+	}
+	for _, p := range paths {
+		// Exact match
+		q := GenCarveQuery(p, false)
+		if !strings.HasPrefix(q, "SELECT * FROM carves WHERE carve=1 AND path = '") ||
+			!strings.HasSuffix(q, "';") {
+			t.Errorf("exact: shape wrong for %q: %q", p, q)
+		}
+		// Glob form
+		gq := GenCarveQuery(p, true)
+		if !strings.Contains(gq, "path LIKE '") || !strings.HasSuffix(gq, "' ESCAPE '\\';") {
+			t.Errorf("glob: shape wrong for %q: %q", p, gq)
 		}
 	}
-	bad := []string{
-		"",
-		"'; SELECT 1; --",
-		"/var/log/a'b",
-		"/var/log/a;b",
-		"/var/log/a b", // space
-		"/var/log/a\"b",
-		"/var/log/a\nb",
-	}
-	for _, p := range bad {
-		if ValidCarvePath(p) {
-			t.Errorf("ValidCarvePath(%q): expected false", p)
+}
+
+// TestGenCarveQueryGlobMapping confirms that '*' and '?' in the input
+// are mapped to LIKE wildcards '%' and '_' in glob mode, while any
+// pre-existing '%' / '_' / '\' in the path are escaped so they are
+// treated as literals.
+func TestGenCarveQueryGlobMapping(t *testing.T) {
+	cases := []struct {
+		in      string
+		wantSub string
+	}{
+		{`/var/log/*.log`, `path LIKE '/var/log/%.log' ESCAPE '\'`},
+		{`/var/log/auth?.log`, `path LIKE '/var/log/auth_.log' ESCAPE '\'`},
+		// Literal % must be escaped to \% so it isn't treated as wildcard.
+		{`/tmp/100%done.txt`, `path LIKE '/tmp/100\%done.txt' ESCAPE '\'`},
+		// Literal _ must be escaped to \_.
+		{`/tmp/snake_case.txt`, `path LIKE '/tmp/snake\_case.txt' ESCAPE '\'`},
+		// Literal backslash must be escaped (Windows paths).
+		{`C:\logs\*.log`, `path LIKE 'C:\\logs\\%.log' ESCAPE '\'`},
+	}
+	for _, tc := range cases {
+		got := GenCarveQuery(tc.in, true)
+		if !strings.Contains(got, tc.wantSub) {
+			t.Errorf("GenCarveQuery(%q, true) = %q; want substring %q", tc.in, got, tc.wantSub)
 		}
 	}
 }
 
-// TestGenCarveQueryShape sanity-checks the SQL shape for both glob and
-// exact match. Real callers MUST validate file via ValidCarvePath first;
-// this test exercises the happy path only.
+// TestGenCarveQueryShape sanity-checks the SQL shape for the happy
+// path on both branches.
 func TestGenCarveQueryShape(t *testing.T) {
 	q1 := GenCarveQuery("/etc/passwd", false)
 	if !strings.Contains(q1, "path = '/etc/passwd'") {
 		t.Errorf("exact: got %q", q1)
 	}
 	q2 := GenCarveQuery("/var/log/*.log", true)
-	if !strings.Contains(q2, "path LIKE '/var/log/*.log'") {
+	if !strings.Contains(q2, "path LIKE '/var/log/%.log' ESCAPE '\\'") {
 		t.Errorf("glob: got %q", q2)
 	}
 }

pkg/config/flags.go

@@ -83,12 +83,33 @@ type ServiceParameters struct {
 	Debug *YAMLConfigurationDebug
 }
 
+// initAuthFlag returns the per-service `--auth` flag with the given
+// default. Kept out of initServiceFlags so each service can pick the
+// default that matches its semantics:
+//
+//   - osctrl-api uses JWT to authenticate operators / SPA clients
+//   - osctrl-admin uses DB-backed user/password (or SAML/OIDC) for
+//     its browser login form
+//   - osctrl-tls authenticates osquery agents via per-environment
+//     enroll secret, not via this flag; AuthNone is correct for it
+func initAuthFlag(params *ServiceParameters, defaultValue string) cli.Flag {
+	return &cli.StringFlag{
+		Name:        "auth",
+		Aliases:     []string{"A"},
+		Value:       defaultValue,
+		Usage:       "Authentication mechanism for the service",
+		Sources:     cli.EnvVars("SERVICE_AUTH"),
+		Destination: &params.Service.Auth,
+	}
+}
+
 // InitTLSFlags initializes all the flags needed for the TLS service
 func InitTLSFlags(params *ServiceParameters) []cli.Flag {
 	var allFlags []cli.Flag
 	// Add flags by category
 	allFlags = append(allFlags, initConfigFlags(params, ServiceTLS)...)
 	allFlags = append(allFlags, initServiceFlags(params)...)
+	allFlags = append(allFlags, initAuthFlag(params, AuthNone))
 	allFlags = append(allFlags, initLoggingFlags(params)...)
 	allFlags = append(allFlags, initMetricsFlags(params)...)
 	allFlags = append(allFlags, initWriterFlags(params)...)
@@ -110,6 +131,7 @@ func InitAdminFlags(params *ServiceParameters) []cli.Flag {
 	// Add flags by category
 	allFlags = append(allFlags, initConfigFlags(params, ServiceAdmin)...)
 	allFlags = append(allFlags, initServiceFlags(params)...)
+	allFlags = append(allFlags, initAuthFlag(params, AuthDB))
 	allFlags = append(allFlags, initLoggingFlags(params)...)
 	allFlags = append(allFlags, initRedisFlags(params)...)
 	allFlags = append(allFlags, initDBFlags(params)...)
@@ -131,6 +153,7 @@ func InitAPIFlags(params *ServiceParameters) []cli.Flag {
 	// Add flags by category
 	allFlags = append(allFlags, initConfigFlags(params, ServiceAPI)...)
 	allFlags = append(allFlags, initServiceFlags(params)...)
+	allFlags = append(allFlags, initAuthFlag(params, AuthJWT))
 	allFlags = append(allFlags, initLoggingFlags(params)...)
 	allFlags = append(allFlags, initRedisFlags(params)...)
 	allFlags = append(allFlags, initDBFlags(params)...)
@@ -191,14 +214,6 @@ func initServiceFlags(params *ServiceParameters) []cli.Flag {
 			Sources:     cli.EnvVars("SERVICE_HOST"),
 			Destination: &params.Service.Host,
 		},
-		&cli.StringFlag{
-			Name:        "auth",
-			Aliases:     []string{"A"},
-			Value:       AuthJWT,
-			Usage:       "Authentication mechanism for the service (jwt|none — `none` requires OSCTRL_INSECURE_NO_AUTH=1)",
-			Sources:     cli.EnvVars("SERVICE_AUTH"),
-			Destination: &params.Service.Auth,
-		},
 		&cli.StringFlag{
 			Name:        "log-level",
 			Value:       LogLevelInfo,