fix: yaml-rules group_by SQL injection — whitelist columns, fail closed (P1 #23)
ober
16f5667d9e50a995fa2d2c6b3566883fdfa59d8a
--- a/examples/yaml_rules_check.ss +++ b/examples/yaml_rules_check.ss @@ -126,6 +126,57 @@ (check " desc has summary" (and (string-contains (hash-get (car r) "description") "ran /tmp/evil") #t) #t)) (store-close db)) +;; ── group_by SQL-injection hardening (P1 #23) ─────────────────────────────── +;; Rule files are hostile input: a bare group_by column used to be dropped +;; verbatim into the SELECT list, so "(SELECT ...)" / UNION payloads ran as SQL. +;; group_by is now whitelisted to real columns (or $. json paths) and fails +;; closed on anything else. +(displayln "group_by injection hardening:") +(check "subquery group_by rejected at parse" + (err? (parse-yaml-rule + (lines "name: inj" "type: threshold" "window: 10m" "threshold: 5" + "group_by:" " - \"(SELECT data FROM events LIMIT 1)\"" + "match:" " event_type: auth_event"))) + #t) +(check "UNION group_by rejected at parse" + (err? (parse-yaml-rule + (lines "name: inj2" "type: threshold" "window: 10m" "threshold: 5" + "group_by:" " - \"host UNION SELECT data FROM events\"" + "match:" " event_type: auth_event"))) + #t) + +;; defense in depth: even a hand-built rule that skips validate-rule cannot +;; inject — group-expr-sql fails closed when the rule is run. +(def (event-match et) + (yaml-rule-match + (unwrap (parse-yaml-rule (lines "name: x" "type: match" "match:" (str " event_type: " et)))))) +(let ((db (fresh)) (base 600000)) + (dotimes (k 5) (store-event db (+ 1 k) "h1" "s" (+ base (* k 1000)) "auth_event" "info" #f "sshd" "fail" + (jdata "username" "alice" "success" #f))) + (let ((evil (make-yaml-rule "evil" "d" "high" '() "threshold" "10m" + '("(SELECT data FROM events LIMIT 1)") 5 #f (event-match "auth_event") '()))) + (check "hand-built hostile group_by fails closed at run" + (try (begin (run-yaml-rule db evil) "ran") + (catch (e) "blocked")) + "blocked")) + (store-close db)) + +;; positive control: a legitimate bare-column group_by still groups correctly. +(def proccnt + (lines "name: proc_count_yaml" "description: 3+ events per process" "severity: medium" + "type: threshold" "window: 10m" "threshold: 3" + "group_by:" " - host" " - process_name" + "match:" " event_type: process_start")) +(let ((db (fresh)) (base 600000)) + (dotimes (k 3) (store-event db (+ 1 k) "h1" "s" (+ base (* k 1000)) "process_start" "info" #f "evil" "e" + (jdata))) + (dotimes (k 2) (store-event db (+ 10 k) "h1" "s" (+ base (* k 1000)) "process_start" "info" #f "good" "e" + (jdata))) + (let ((r (run-yaml-rule db (rule proccnt)))) + (check "bare-column group_by fires once" (length r) 1) + (check " group=evil" (hash-get (hash-get (car r) "details") "group_values") '("evil"))) + (store-close db)) + (newline) (if (= fails 0) (displayln "OK: yaml-rules engine matches secmon's behaviour.") --- a/jsecmon/yaml-rules.ss +++ b/jsecmon/yaml-rules.ss @@ -24,6 +24,7 @@ (library (jsecmon yaml-rules) (export yaml-rule? make-yaml-rule yaml-rule-name yaml-rule-description yaml-rule-severity yaml-rule-attack + yaml-rule-match parse-window-ms validate-rule parse-yaml-rule load-yaml-rules run-yaml-rule run-yaml-rules) @@ -119,11 +120,24 @@ (sql-quote (str "%" (cdr kv) "%")))) (hash-get spec "data_contains"))))) - ;; group_by expr -> SQL: a $. path is a json_extract, else a bare column. + ;; group_by may only name a fixed events-table column or a $. json path; rule + ;; files are hostile input in our threat model, so anything else is rejected + ;; before it can reach the SELECT list as raw SQL (P1 #23, SQL injection). + (def allowed-group-columns + '("id" "seq" "host" "source" "timestamp_ms" "event_type" "severity" + "pid" "process_name" "summary")) + (def (valid-group-expr? expr) + (and (string? expr) + (or (string-prefix? "$." expr) + (and (member expr allowed-group-columns) #t)))) + + ;; group_by expr -> SQL: a $. path is a json_extract, a whitelisted column is + ;; used verbatim, and anything else fails closed (error) rather than injecting. (def (group-expr-sql expr) - (if (string-prefix? "$." expr) - (str "json_extract(data, '" (sql-escape expr) "')") - expr)) + (cond ((string-prefix? "$." expr) + (str "json_extract(data, '" (sql-escape expr) "')")) + ((member expr allowed-group-columns) expr) + (else (error 'group-expr-sql (str "group_by column not allowed: " expr))))) (def (group-exprs rule) ;; group_by minus the implicit "host" (filter (lambda (g) (not (string=? g "host"))) (yaml-rule-group-by rule))) @@ -176,6 +190,8 @@ (def (validate-rule rule) ;; -> (ok #t) | (err reason) (cond ((string=? (yaml-rule-name rule) "") (err "name is required")) + ((find (lambda (g) (not (valid-group-expr? g))) (yaml-rule-group-by rule)) + => (lambda (bad) (err (str "invalid group_by column: " bad)))) (else (let ((ty (yaml-rule-type rule))) (cond