fix(gitsafe): close custom-rule shadow bypass + restore real-diff parsing
ober
35b182b7df0d2ec2e5d0ed8ca9b0361f13d66ea3
--- a/gitsafe/config.ss +++ b/gitsafe/config.ss @@ -31,7 +31,8 @@ (except (jerboa prelude) meta atom?) (std text json) (std misc ports) - (std misc string)) + (std misc string) + (only (gitsafe patterns) all-patterns secret-pattern-id)) ;; --- Config struct --- (defstruct gitsafe-config @@ -142,6 +143,32 @@ *untrusted-min-file-size-mb* mb)) + ;; IDs of the built-in secret patterns, as strings. + (def *builtin-pattern-ids* + (map (lambda (p) (symbol->string (secret-pattern-id p))) (all-patterns))) + + ;; Extract a custom pattern's "id" string from a raw JSON object, which may + ;; be a hash-table or an alist depending on the (std text json) version. + (def (custom-pattern-id obj) + (cond + [(and (hash-table? obj) (hash-key? obj "id")) + (let ([v (hash-ref obj "id" #f)]) (and (string? v) v))] + [(and (list? obj) (assoc "id" obj)) + => (lambda (kv) (let ([v (cdr kv)]) (and (string? v) v)))] + [else #f])) + + ;; Custom patterns are honored from an untrusted config because a *new* rule + ;; can only add detection. But a custom pattern whose id collides with a + ;; built-in pattern id shadows that built-in rule's metadata (path/keywords/ + ;; entropy/allowlist) in rule-meta, which an attacker abuses to silence a + ;; built-in detector (e.g. id "aws-access-key" with an impossible path). Drop + ;; colliding ids so untrusted custom rules can add but never override. + (def (sanitize-custom-patterns custom) + (filter (lambda (cp) + (let ([id (custom-pattern-id cp)]) + (not (and id (member id *builtin-pattern-ids*))))) + custom)) + ;; --- Parse severity string to symbol --- (def (parse-severity s) (match s @@ -290,12 +317,13 @@ ;; detection. Narrow excludes/allowlists and custom rules (which ;; only add detection) still apply; match-all globs, severity ;; overrides, pattern disabling, entropy disabling, sub-floor size - ;; caps, empty allowlist strings, and in-tree baselines do not. + ;; caps, empty allowlist strings, in-tree baselines, and custom + ;; rules that shadow a built-in pattern id do not. (make-gitsafe-config 'medium #t '() - custom + (sanitize-custom-patterns custom) (filter (lambda (g) (not (match-all-glob? g))) excludes) (filter (lambda (g) (not (match-all-glob? g))) al-files) (filter non-empty-string? al-strs) --- a/gitsafe/git.ss +++ b/gitsafe/git.ss @@ -214,8 +214,11 @@ [else #f])) ;; A +++ line is a file header only at a genuine file boundary: a --- header - ;; immediately precedes it, and that --- header follows a "diff --git" line. - ;; Content lines always carry a leading +/-/space, so they can never fake a + ;; immediately precedes it, and that --- header appears in the header block + ;; opened by a "diff --git" line (git's metadata lines -- index, new file + ;; mode, rename, etc. -- may sit between "diff --git" and "--- ", so + ;; after-diff-git? spans the whole block, not just the next line). Content + ;; lines always carry a leading +/-/space, so they can never fake a ;; "diff --git " boundary; this keeps an added line whose content is "++ b/x" ;; (emitted as "+++ b/x") from being misparsed as a new-file boundary that ;; would flush the hunk and silently drop every subsequent added line. @@ -280,8 +283,16 @@ (> (string-length line) 0) (char=? (string-ref line 0) #\-)) (loop rest current-file hunks cur-hunk new-line-no #f #f)] + ;; Any other line. Between "diff --git" and the "--- "/"+++ " header + ;; pair git emits metadata lines (new file mode, old mode, index, + ;; similarity/rename/copy, "Binary files ... differ"); these must NOT + ;; clear after-diff-git?, or the genuine "--- " header that follows + ;; them would be missed and the whole file's hunks dropped. Content + ;; lines never reach here with after-diff-git? set: inside a hunk the + ;; @@ header already cleared it, and a raw "diff --git " line can only + ;; be a genuine boundary (content carries a leading +/-/space). [else - (loop rest current-file hunks cur-hunk new-line-no #f #f)]))))) + (loop rest current-file hunks cur-hunk new-line-no #f after-diff-git?)]))))) ;; --- Public API --- --- a/test/test-gitsafe.ss +++ b/test/test-gitsafe.ss @@ -286,6 +286,48 @@ (delete-test-file! secret-path) (check-predicate aws pair?))))) + (test-case "untrusted config: custom rule shadowing a built-in id cannot suppress it" + ;; patterns.custom is honored from an in-repo config because a NEW rule + ;; only adds detection. But a custom rule whose id collides with a + ;; built-in rule (here "aws-access-key") shadows that rule's metadata in + ;; rule-meta; an impossible path/keyword/entropy/allowlist would then + ;; silence the built-in detector. Such colliding rules must be dropped. + (let ([cfg-path "test/fixtures/tmp-shadow-custom.gitsafe.json"]) + (write-test-file! cfg-path + "{\"patterns\":{\"custom\":[{\"id\":\"aws-access-key\",\"pattern\":\"AKIA[0-9A-Z]{16}\",\"path\":\"^NEVER_MATCH$\"}]}}\n") + (let* ([c (load-config cfg-path)] + [findings (scan-content "app.env" + "aws_key = AKIAIOSFODNN7EXAMPLE\n" c)] + [aws (filter (lambda (f) (eq? (finding-pattern-id f) 'aws-access-key)) + findings)]) + (delete-test-file! cfg-path) + (check-predicate aws pair?)))) + + (test-case "untrusted config: new-id custom rule still adds detection" + (let ([cfg-path "test/fixtures/tmp-newid-custom.gitsafe.json"]) + (write-test-file! cfg-path + "{\"patterns\":{\"custom\":[{\"id\":\"internal-key\",\"pattern\":\"ISK_[A-Za-z0-9]{8}\",\"severity\":\"high\"}]}}\n") + (let* ([c (load-config cfg-path)] + [findings (scan-content "service.conf" "token = ISK_AbCdEf12\n" c)]) + (delete-test-file! cfg-path) + (check-predicate + (filter (lambda (f) (eq? (finding-pattern-id f) 'internal-key)) findings) + pair?)))) + + (test-case "trusted config: custom rule may still override built-in metadata" + ;; An operator-chosen (--config) config may tune a built-in rule; the + ;; shadow-protection only applies to untrusted in-repo configs. + (let ([cfg-path "test/fixtures/tmp-trusted-shadow.gitsafe.json"]) + (write-test-file! cfg-path + "{\"patterns\":{\"custom\":[{\"id\":\"aws-access-key\",\"pattern\":\"AKIA[0-9A-Z]{16}\",\"path\":\"^NEVER_MATCH$\"}]}}\n") + (let* ([c (load-config cfg-path #t)] + [findings (scan-content "app.env" + "aws_key = AKIAIOSFODNN7EXAMPLE\n" c)] + [aws (filter (lambda (f) (eq? (finding-pattern-id f) 'aws-access-key)) + findings)]) + (delete-test-file! cfg-path) + (check-equal? '() aws)))) + )) ;; ============================================================ @@ -569,12 +611,74 @@ "@@ -0,0 +1,1 @@\n" "+key = AKIAIOSFODNN7EXAMPLE\n")] [hunks (parse-unified-diff-all diff)] + [findings (scan-diff-hunks hunks (default-config))] + [aws (filter (lambda (f) (eq? (finding-pattern-id f) 'aws-access-key)) + findings)]) + (check-equal? 1 (length hunks)) + (check-predicate (diff-hunk-file (car hunks)) + (lambda (f) (and (string? f) (not (string-empty? f))))) + (check-predicate aws pair?))) + + (test-case "real git new-file diff (mode+index headers) is parsed and scanned" + ;; Regression: the boundary tracker must span git's metadata lines + ;; (new file mode, index) that sit between "diff --git" and "--- /dev/null", + ;; or every newly-added file's hunks are dropped and nothing is detected. + (let* ([diff (string-append + "diff --git a/app.env b/app.env\n" + "new file mode 100644\n" + "index 0000000..09ed4c1\n" + "--- /dev/null\n" + "+++ b/app.env\n" + "@@ -0,0 +1,2 @@\n" + "+hello\n" + "+aws_key = AKIAIOSFODNN7EXAMPLE\n")] + [hunks (parse-unified-diff-all diff)] [findings (scan-diff-hunks hunks (default-config))] [aws (filter (lambda (f) (eq? (finding-pattern-id f) 'aws-access-key)) findings)]) (check-equal? 1 (length hunks)) - (check-predicate (diff-hunk-file (car hunks)) - (lambda (f) (and (string? f) (not (string-empty? f))))) + (check-equal? "app.env" (diff-hunk-file (car hunks))) + (check-predicate aws pair?))) + + (test-case "real git modified-file diff (index header) is parsed and scanned" + (let* ([diff (string-append + "diff --git a/mod.env b/mod.env\n" + "index a507e8f..b608e9f 100644\n" + "--- a/mod.env\n" + "+++ b/mod.env\n" + "@@ -1 +1,2 @@\n" + " line1\n" + "+aws_secret = AKIAIOSFODNN7EXAMPLE\n")] + [hunks (parse-unified-diff-all diff)] + [findings (scan-diff-hunks hunks (default-config))] + [aws (filter (lambda (f) (eq? (finding-pattern-id f) 'aws-access-key)) + findings)]) + (check-equal? 1 (length hunks)) + (check-predicate aws pair?))) + + (test-case "real multi-file diff with metadata headers parses both files" + (let* ([diff (string-append + "diff --git a/a.env b/a.env\n" + "new file mode 100644\n" + "index 0000000..1111111\n" + "--- /dev/null\n" + "+++ b/a.env\n" + "@@ -0,0 +1,1 @@\n" + "+first\n" + "diff --git a/b.env b/b.env\n" + "new file mode 100644\n" + "index 0000000..2222222\n" + "--- /dev/null\n" + "+++ b/b.env\n" + "@@ -0,0 +1,1 @@\n" + "+aws_key = AKIAIOSFODNN7EXAMPLE\n")] + [hunks (parse-unified-diff-all diff)] + [findings (scan-diff-hunks hunks (default-config))] + [aws (filter (lambda (f) (eq? (finding-pattern-id f) 'aws-access-key)) + findings)]) + (check-equal? 2 (length hunks)) + (check-equal? "a.env" (diff-hunk-file (car hunks))) + (check-equal? "b.env" (diff-hunk-file (cadr hunks))) (check-predicate aws pair?))) (test-case "display-findings: SARIF includes rule results and fingerprints"