diff --git a/cmd/api/handlers/carves.go b/cmd/api/handlers/carves.go index 661c5014..837b19fa 100644 --- a/cmd/api/handlers/carves.go +++ b/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) { diff --git a/pkg/carves/utils.go b/pkg/carves/utils.go index bd224a96..b020ee8a 100644 --- a/pkg/carves/utils.go +++ b/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) + "';" } diff --git a/pkg/carves/utils_test.go b/pkg/carves/utils_test.go index 03824410..fca19bf9 100644 --- a/pkg/carves/utils_test.go +++ b/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) } } diff --git a/pkg/config/flags.go b/pkg/config/flags.go index 96fbc053..d06febc6 100644 --- a/pkg/config/flags.go +++ b/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: ¶ms.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: ¶ms.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: ¶ms.Service.Auth, - }, &cli.StringFlag{ Name: "log-level", Value: LogLevelInfo,