diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 006b927..eaee781 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -37,15 +37,8 @@ on: its own ref. type: string default: "v1" - holmes-upstream-ref: - description: >- - Upstream clj-holmes rules repo. Pinned rather than floating: the stock action - fetched #main at runtime, leaving detection rules unpinned in a workflow that - SHA-pins everything else. - type: string - default: "git://clj-holmes/clj-holmes-rules#main" ignored-paths: - description: "Paths both clj-holmes and semgrep must skip (e.g. deliberately-vulnerable fixtures)" + description: "Paths semgrep must skip (e.g. deliberately-vulnerable fixtures)" type: string default: "" secrets: @@ -61,8 +54,8 @@ on: permissions: # contents: read only. `pull-requests: write` was here for clj-holmes-action, # which posted PR summary comments; that action is gone. Nothing left in this - # workflow writes to a PR — clj-holmes and semgrep emit SARIF to files that go - # up as artifacts, gitleaks/actionlint/zizmor are plain binaries, and + # workflow writes to a PR — semgrep emits SARIF to a file that goes up as an + # artifact, gitleaks/actionlint/zizmor are plain binaries, and # action-shellcheck annotates via workflow commands, which needs no permission. # # This is not cosmetic: a reusable workflow cannot request more than its caller @@ -189,102 +182,6 @@ jobs: echo "Linting: ${paths[*]}" clj-kondo --lint "${paths[@]}" --fail-level error - clj-holmes: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - persist-credentials: false - - name: Install clj-holmes - # Direct binary install rather than clj-holmes-action: the action's - # entrypoint fetches its rules from an unpinned #main at runtime, which - # left detection rules floating in a workflow that SHA-pins every other - # third-party action. Same install pattern the gitleaks job uses. - shell: bash - run: | - set -euo pipefail - VER=1.4.3 - curl -fsSL "https://github.com/clj-holmes/clj-holmes/releases/download/v${VER}/clj-holmes-ubuntu-latest" \ - -o /tmp/clj-holmes - sudo install -m 755 /tmp/clj-holmes /usr/local/bin/clj-holmes - - name: Fetch upstream rules - # Upstream rules ONLY. The cleancoders custom rules are semgrep rules, - # not clj-holmes rules: clj-holmes reads only *.clj — it silently skips - # .cljs and .cljc, and its edamame call omits :read-cond so .cljc fails - # to parse even when renamed, with the failure swallowed. Upstream has - # been unmaintained since Oct 2022, so that is not getting fixed. - # - # What clj-holmes still earns its place for: MD5, SHA-1, Blowfish, - # DESede, ECB, weak SSL context, insecure hostname verifiers, XXE, and - # read-string — all hard-failing, all .clj-only. - shell: bash - env: - HOLMES_UPSTREAM_REF: ${{ inputs.holmes-upstream-ref }} - run: | - set -euo pipefail - clj-holmes fetch-rules -r "$HOLMES_UPSTREAM_REF" -o /tmp/rules - # A scan with no rules exits 0 and looks like a clean build. Upstream - # ships ~10 rules; floor well below that so pruning one does not - # false-alarm, while still catching catastrophic loss (fetch failed, - # ref renamed, empty tarball). - count=$(find /tmp/rules -name '*.yml' | wc -l) - echo "loaded $count rules" - if [ "$count" -lt 5 ]; then - echo "::error::only $count rules loaded; refusing to scan" - exit 1 - fi - - name: clj-holmes SAST - shell: bash - env: - IGNORED: ${{ inputs.ignored-paths }} - run: | - set -euo pipefail - # --no-fail-on-result on purpose. `--fail-on-result -t sarif` exits 3 - # even with ZERO findings (clj-holmes bug; unmaintained since 2022, so - # it will not be fixed). -t json and -t stdout exit 0 correctly, which - # pins it to that one combination. We need SARIF for the evidence - # artifact, so we take the exit decision ourselves below. - # - # This also fixes a second problem: with SARIF going to a file, a - # failing scan printed nothing, so the log gave no reason. Now the - # findings are echoed before the job fails. - # --no-verbose is REQUIRED, not cosmetic. clj-holmes renders a progrock - # progress bar whose ETA calculation overflows an int on large repos: - # IllegalArgumentException: Value out of range for int: 35050732419 - # progrock.core$interval_str -> clj_holmes.logic.progress - # That kills the scan with exit 255 and writes no SARIF at all. It is - # silent on small repos and fatal on real ones (~1300 Clojure files). - # The stock clj-holmes-action always passed --no-verbose; dropping it - # when replacing that action is what reintroduced this. - args=(scan -p . -d /tmp/rules --no-fail-on-result --no-verbose -t sarif -o clj-holmes.sarif) - # NOT `[ -n "$IGNORED" ] && args+=(...)`: that whole statement returns 1 - # when the input is empty (the default), and `set -e` would exit here. - if [ -n "$IGNORED" ]; then - args+=(-i "$IGNORED") - fi - clj-holmes "${args[@]}" - - if [ ! -f clj-holmes.sarif ]; then - echo "::error::clj-holmes produced no SARIF; the scan did not complete" - exit 1 - fi - count=$(jq '[.runs[].results[]?] | length' clj-holmes.sarif) - if [ "$count" -gt 0 ]; then - jq -r '.runs[].results[] - | " \(.ruleId) \(.locations[0].physicalLocation.artifactLocation.uri):\(.locations[0].physicalLocation.region.startLine // 0)"' \ - clj-holmes.sarif - echo "::error::clj-holmes found $count finding(s); see above and the clj-holmes-sarif artifact" - exit 1 - fi - echo "clj-holmes: no findings" - - name: Upload SARIF - if: always() # evidence of a FAILING scan is the evidence most worth keeping - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 (node24) - with: - name: clj-holmes-sarif - path: clj-holmes.sarif - retention-days: 90 - shellcheck: needs: detect if: needs.detect.outputs.has-shellcheck-target == 'true' @@ -386,8 +283,14 @@ jobs: semgrep: runs-on: ubuntu-latest - # PRIMARY engine for the cleancoders custom rules. semgrep reads .clj, .cljs - # AND .cljc; clj-holmes reads only .clj and is unmaintained since Oct 2022. + # The ONLY Clojure scanner. clj-holmes was dropped: it read just .clj + # (silently skipping .cljs and .cljc), was unmaintained since Oct 2022, and + # cost three bugs — exiting 3 on zero findings with -t sarif, hiding findings + # in a file, and crashing on a progress-bar int overflow that blocked a + # production deploy. Its unique detections (read-string, clojure.xml XXE, + # weak crypto, insecure TLS) are now cc-* rules that measurably outperform + # it: 8 of 8 fixture cases versus its 6, and they cover .cljs/.cljc too. + # # semgrep's weakness is that it cannot resolve namespace aliases, so each # rule enumerates them and spec-fixtures/ exercises more than one spelling — # see bin/test-rules.sh, which fails if that stops working. @@ -399,10 +302,10 @@ jobs: # surfaces it without blocking; semgrep-blocking: true fails the build. continue-on-error: ${{ !inputs.semgrep-blocking }} container: - # Digest-pinned, not :latest. semgrep is now the PRIMARY detection engine, - # so an unpinned image would let the thing that decides whether the build - # is secure change underneath us — the same hole as the unpinned upstream - # clj-holmes rules. zizmor flags this as unpinned-images (high). + # Digest-pinned, not :latest. semgrep is now the ONLY Clojure detection + # engine, so an unpinned image would let the thing that decides whether the + # build is secure change underneath us. zizmor flags this as + # unpinned-images (high). image: semgrep/semgrep@sha256:98c2572fced2474539fd27cab3207ebd8e95e4e7aab4c3b381fdc5e2641d9941 # latest @ 2026-07-22 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 diff --git a/README.md b/README.md index cd37e39..c2e260a 100644 --- a/README.md +++ b/README.md @@ -4,19 +4,26 @@ Shared reusable GitHub Actions workflows for cleancoders repos. ## `security.yml` — reusable security-scan workflow -Runs eight scanners. **Hard-fail** (block the caller): `clj-kondo`, `clj-holmes`, +Runs seven scanners. **Hard-fail** (block the caller): `clj-kondo`, `shellcheck`, `gitleaks`, `actionlint`. **Advisory by default** (report, never block): `clj-watson`, `semgrep`, `zizmor` — each can be made blocking per-consumer via the `clj-watson-blocking` / `semgrep-blocking` / `zizmor-blocking` inputs. -**semgrep is the primary Clojure detection engine.** It carries the 12 -cleancoders `cc-*` rules in `security-rules/semgrep/` and reads `.clj`, `.cljs`, -and `.cljc`. `clj-holmes` runs upstream rules only — it covers weak crypto, -XXE, and `read-string`, but reads **only `.clj`**: it silently skips `.cljs` and -`.cljc`, and its parser rejects reader conditionals. Upstream has been -unmaintained since October 2022. That split is deliberate; see -`docs/superpowers/specs/2026-07-27-cwe-owasp-coverage-design.md` (Revision 2). +**semgrep is the only Clojure detection engine.** It carries the 16 cleancoders +`cc-*` rules in `security-rules/semgrep/` and reads `.clj`, `.cljs`, and +`.cljc`. + +`clj-holmes` was removed. It read only `.clj` — silently skipping `.cljs` and +`.cljc`, and rejecting reader conditionals outright — had been unmaintained +since October 2022, and produced three failures in its first week: exiting 3 on +zero findings under `-t sarif`, writing findings to a file so a red build gave +no reason, and crashing on a progress-bar integer overflow that blocked a +production deploy. Its unique detections are now `cc-read-string`, +`cc-clojure-xml-xxe`, `cc-weak-crypto`, and `cc-insecure-tls`, which catch 8 of +8 fixture cases against its 6 — it shipped rules for Blowfish and DESede and +matched neither. Rationale: `docs/superpowers/specs/2026-07-27-cwe-owasp-coverage-design.md` +(Revision 2). ### Usage @@ -47,8 +54,7 @@ jobs: | `zizmor-blocking` | `false` | When `true`, zizmor Actions-security findings fail the workflow. Advisory by default because zizmor's defaults light up existing repos. | | `extra-rules-dir` | `".security-rules"` | Consumer-supplied semgrep rules, added as an extra `--config`. Self-skips when the directory is absent. | | `rules-ref` | `"v1"` | Ref of this repo to source the `cc-*` rules from. **Must match the ref you consume the workflow at** — a reusable workflow cannot determine its own ref, so consuming `@v2` or a SHA without setting this gets you `v1` rules. | -| `holmes-upstream-ref` | `"git://clj-holmes/clj-holmes-rules#main"` | Upstream clj-holmes rules source. Override to pin a SHA. | -| `ignored-paths` | `""` | Paths both clj-holmes and semgrep must skip, e.g. deliberately-vulnerable fixtures. | +| `ignored-paths` | `""` | Paths semgrep must skip, e.g. deliberately-vulnerable fixtures. | ### Coverage @@ -65,16 +71,20 @@ is authoritative for its own. |------|-------|-----|------------|----------| | `cc-cljs-eval` | `cljs-dom-xss` | 94 | A05 | yes | | `cc-cljs-innerhtml` | `cljs-dom-xss` | 79 | A05 | yes | +| `cc-clojure-xml-xxe` | `xxe` | 611 | A02 | no (triage) | | `cc-dangerously-set-html` | `cljs-dom-xss` | 79 | A05 | yes | | `cc-explain-data-response` | `spec-malli-leak` | 209 | A10 | yes | | `cc-generic-catch` | `fail-open` | 636, 396 | A10 | no (triage) | | `cc-hiccup-raw` | `hiccup-injection` | 79 | A05 | yes | +| `cc-insecure-tls` | `insecure-tls-verification` | 295 | A07 | yes | | `cc-load-string` | `dynamic-eval` | 94 | A05 | yes | | `cc-nippy-thaw` | `java-deserialization` | 502 | A08 | yes | | `cc-path-traversal` | `path-traversal` | 22 | A01 | no (triage) | +| `cc-read-string` | `read-string-rce` | 94 | A05 | yes | | `cc-shell-exec` | `command-injection` | 78, 77 | A05 | yes | | `cc-snakeyaml-unsafe` | `java-deserialization` | 502 | A08 | yes | | `cc-sql-string-concat` | `sql-injection` | 89 | A05 | yes | +| `cc-weak-crypto` | `weak-crypto` | 327, 328 | A04 | yes | @@ -91,14 +101,19 @@ A coverage table that overstates is worse than none, so: miss. `spec-fixtures/` exercises more than one alias per sink and `bin/test-rules.sh` fails if any stops matching, so the enumeration is test-guarded rather than aspirational — but it is still enumeration. -3. **clj-holmes rules apply to `.clj` only** — not `.cljs`, not `.cljc`. That - covers the crypto, XXE, and `read-string` rows. +3. **`cc-weak-crypto` and `cc-insecure-tls` match algorithm names textually** + (`pattern-regex`), because semgrep does not bind metavariables inside Clojure + string literals. They can therefore fire inside a comment or an unrelated + string. Weaker than the structural rules, but the thing being checked *is* a + literal. 4. **OWASP A06 Insecure Design is uncovered.** It is a threat-modeling category. 5. **10 of 19 applicable CWE Top 25 entries depend on a manual `/security-audit` run** — access control above all. CI cannot invoke it. Expected cadence is once per release; nothing enforces that. -6. **`cc-path-traversal` and `cc-generic-catch` do not block** (`severity: - WARNING`). Without dataflow they cannot be precise enough to gate a build. +6. **`cc-path-traversal`, `cc-generic-catch`, and `cc-clojure-xml-xxe` do not + block** (`severity: WARNING`). Without dataflow they cannot be precise enough + to gate a build — `cc-clojure-xml-xxe` in particular fires on every XML parse + because verifying that the factory was hardened requires tracking the object. 7. **Rules track `rules-ref`, default `v1`.** Consuming another ref without setting it gets v1 rules. diff --git a/security-rules/semgrep/cc-clojure-xml-xxe.yaml b/security-rules/semgrep/cc-clojure-xml-xxe.yaml new file mode 100644 index 0000000..4effde1 --- /dev/null +++ b/security-rules/semgrep/cc-clojure-xml-xxe.yaml @@ -0,0 +1,23 @@ +rules: + - id: cc-clojure-xml-xxe + languages: [clojure] + severity: WARNING + message: >- + clojure.xml/parse and clojure.data.xml/parse use JVM SAX defaults that permit + external-entity resolution, so parsing untrusted XML is XXE. Harden the factory + explicitly (disallow-doctype-decl true, external entities false). + LOW PRECISION — this fires on every parse call, hardened or not: verifying + that the factory was configured requires dataflow neither engine has. Triage + with /security-audit rather than treating it as a defect. + metadata: + cwe: ["CWE-611: Improper Restriction of XML External Entity Reference"] + owasp: ["A02:2025 - Security Misconfiguration"] + class: xxe + confidence: LOW + pattern-either: + - pattern: (clojure.xml/parse ...) + - pattern: (xml/parse ...) + - pattern: (cxml/parse ...) + - pattern: (clojure.data.xml/parse ...) + - pattern: (data.xml/parse ...) + - pattern: (dxml/parse ...) diff --git a/security-rules/semgrep/cc-insecure-tls.yaml b/security-rules/semgrep/cc-insecure-tls.yaml new file mode 100644 index 0000000..ab8b341 --- /dev/null +++ b/security-rules/semgrep/cc-insecure-tls.yaml @@ -0,0 +1,23 @@ +rules: + - id: cc-insecure-tls + languages: [clojure] + severity: ERROR + message: >- + TLS configured without authentication: an obsolete protocol, a permissive + hostname verifier, or a trust-all manager. This stops passive eavesdropping + but not an active machine-in-the-middle. Use the platform default verifier and + TLSv1.2+; pin a custom trust store if a private CA is genuinely required. + metadata: + cwe: ["CWE-295: Improper Certificate Validation"] + owasp: ["A07:2025 - Authentication Failures"] + class: insecure-tls-verification + confidence: HIGH + alias-exempt: "Java interop on javax.net.ssl; no Clojure alias" + # pattern-regex for the protocol string: semgrep does not bind metavariables + # inside Clojure string literals, so (SSLContext/getInstance "$P") never matches. + pattern-either: + - pattern-regex: \(SSLContext/getInstance\s+"(?i)(SSL|SSLv2|SSLv3|TLSv1|TLSv1\.1)" + - pattern: (reify HostnameVerifier (verify [...] true)) + - pattern: (reify javax.net.ssl.HostnameVerifier (verify [...] true)) + - pattern: (setDefaultHostnameVerifier ...) + - pattern: "{... :insecure? true ...}" diff --git a/security-rules/semgrep/cc-read-string.yaml b/security-rules/semgrep/cc-read-string.yaml new file mode 100644 index 0000000..5b408ce --- /dev/null +++ b/security-rules/semgrep/cc-read-string.yaml @@ -0,0 +1,19 @@ +rules: + - id: cc-read-string + languages: [clojure] + severity: ERROR + message: >- + clojure.core/read-string honors the #= reader-eval tag, so reading + attacker-controlled text is remote code execution. Use clojure.edn/read-string. + Note edn is only safe with an empty :readers map — a custom reader on + attacker-controlled tag data executes too. + metadata: + cwe: ["CWE-94: Improper Control of Generation of Code ('Code Injection')"] + owasp: ["A05:2025 - Injection"] + class: read-string-rce + confidence: MEDIUM + alias-exempt: "clojure.core/read-string is referred by default; no alias to enumerate" + pattern-either: + - pattern: (read-string $X) + - pattern: (clojure.core/read-string $X) + pattern-not: (clojure.edn/read-string $X) diff --git a/security-rules/semgrep/cc-weak-crypto.yaml b/security-rules/semgrep/cc-weak-crypto.yaml new file mode 100644 index 0000000..4ac3eba --- /dev/null +++ b/security-rules/semgrep/cc-weak-crypto.yaml @@ -0,0 +1,23 @@ +rules: + - id: cc-weak-crypto + languages: [clojure] + severity: ERROR + message: >- + Broken or reversible cryptographic primitive. Use SHA-256+ for digests, + AES/GCM for encryption, and bcrypt or argon2 for passwords — a bare digest is + never a password hash regardless of algorithm strength. + metadata: + cwe: + - "CWE-327: Use of a Broken or Risky Cryptographic Algorithm" + - "CWE-328: Use of Weak Hash" + owasp: ["A04:2025 - Cryptographic Failures"] + class: weak-crypto + confidence: HIGH + alias-exempt: "Java interop on javax.crypto / java.security; no Clojure alias" + # pattern-regex, not a metavariable: semgrep does not bind metavariables inside + # string literals in Clojure, so (Cipher/getInstance "$ALG") never matches. The + # thing being checked IS the literal, so a regex is the honest instrument. + pattern-either: + - pattern-regex: \(MessageDigest/getInstance\s+"(?i)(MD2|MD4|MD5|SHA-?1)" + - pattern-regex: \(Cipher/getInstance\s+"[^"]*(?i)(ECB|Blowfish|DESede|RC2|RC4)[^"]*" + - pattern-regex: \(Cipher/getInstance\s+"(?i)DES(/[^"]*)?" diff --git a/spec-fixtures/expectations.tsv b/spec-fixtures/expectations.tsv index 9be2672..b600437 100644 --- a/spec-fixtures/expectations.tsv +++ b/spec-fixtures/expectations.tsv @@ -11,3 +11,7 @@ cc-generic-catch triage_low.clj cc-nippy-thaw deser.clj cc-snakeyaml-unsafe deser.clj cc-explain-data-response error_leak.clj +cc-read-string crypto_tls.clj +cc-clojure-xml-xxe crypto_tls.clj +cc-weak-crypto crypto_tls.clj +cc-insecure-tls crypto_tls.clj diff --git a/spec-fixtures/safe/crypto_tls.clj b/spec-fixtures/safe/crypto_tls.clj new file mode 100644 index 0000000..3eaa856 --- /dev/null +++ b/spec-fixtures/safe/crypto_tls.clj @@ -0,0 +1,27 @@ +(ns fixtures.crypto-tls-safe + (:require [clojure.edn :as edn]) + (:import [javax.crypto Cipher] + [javax.net.ssl SSLContext] + [java.security MessageDigest])) + +;; edn/read-string does not honor #= — both the aliased and fully-qualified +;; spellings must stay clean. +(defn parse-config [s] (edn/read-string s)) +(defn parse-config2 [s] (clojure.edn/read-string s)) + +;; Modern primitives. +(defn digest [] (MessageDigest/getInstance "SHA-256")) +(defn digest-512 [] (MessageDigest/getInstance "SHA-512")) +(defn cipher-gcm [] (Cipher/getInstance "AES/GCM/NoPadding")) +(defn cipher-cbc [] (Cipher/getInstance "AES/CBC/PKCS5Padding")) + +;; Protocol negotiation left to the platform, or pinned forward. +(defn ctx-default [] (SSLContext/getInstance "TLS")) +(defn ctx-13 [] (SSLContext/getInstance "TLSv1.3")) +(defn ctx-12 [] (SSLContext/getInstance "TLSv1.2")) + +;; NOTE: no hardened clojure.xml/parse call appears here. cc-clojure-xml-xxe is +;; severity WARNING precisely because it cannot tell a hardened factory from a +;; default one — verifying that needs dataflow. Putting a hardened parse in this +;; corpus would fail the false-positive check for a limitation the rule already +;; documents, so the class is triaged by /security-audit instead. diff --git a/spec-fixtures/vulnerable/crypto_tls.clj b/spec-fixtures/vulnerable/crypto_tls.clj new file mode 100644 index 0000000..d22abfc --- /dev/null +++ b/spec-fixtures/vulnerable/crypto_tls.clj @@ -0,0 +1,35 @@ +(ns fixtures.crypto-tls + (:require [clojure.xml :as xml] + [clojure.data.xml :as dxml]) + (:import [javax.crypto Cipher] + [javax.net.ssl SSLContext HostnameVerifier] + [java.security MessageDigest])) + +;; --- read-string: honors #= reader-eval, so this is RCE --------------------- +(defn parse-config [s] + (read-string s)) + +;; --- XXE: JVM SAX defaults resolve external entities ----------------------- +;; Two aliases for the same sink family, guarding the enumerated alias list. +(defn parse-feed [f] (xml/parse f)) +(defn parse-doc [f] (dxml/parse f)) + +;; --- weak crypto ----------------------------------------------------------- +;; Blowfish and DESede are here specifically because clj-holmes shipped rules +;; for both and matched neither; these fixtures pin that regression closed. +(defn digest-md5 [] (MessageDigest/getInstance "MD5")) +(defn digest-sha1 [] (MessageDigest/getInstance "SHA-1")) +(defn cipher-ecb [] (Cipher/getInstance "AES/ECB/PKCS5Padding")) +(defn cipher-bf [] (Cipher/getInstance "Blowfish")) +(defn cipher-3des [] (Cipher/getInstance "DESede/CBC/PKCS5Padding")) +(defn cipher-des [] (Cipher/getInstance "DES")) + +;; --- TLS without authentication ------------------------------------------- +(defn ctx-ssl [] (SSLContext/getInstance "SSL")) +(defn ctx-tls1 [] (SSLContext/getInstance "TLSv1")) +(defn trust-any-host [] + (reify HostnameVerifier + (verify [_ _ _] true))) +(defn fetch [url] + ;; clj-http style: skips certificate validation entirely + {:url url :insecure? true})