From 922ed84053ed612443dddbdfccaf8153b32a79fa Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Wed, 9 Sep 2026 16:13:17 +0200 Subject: [PATCH] feat(repos): a WordPress plugin with a verified CVE, and the WordPress DB API contributor-scan 5.1's detection half. Paid Memberships Pro 2.9.7 pinned at 1a900be3, carrying CVE-2023-23488 -- unauthenticated SQL injection through the `code` parameter of the /pmpro/v1/order REST route. **Verified in the code, not taken from the advisory.** At this commit classes/class.memberorder.php:936 reads $id = $wpdb->get_var("SELECT id FROM $wpdb->pmpro_membership_orders WHERE code = '" . $code . "' LIMIT 1"); and tag 2.9.8 replaces it with `$wpdb->prepare( "... code = %s ...", $code )`. 637 PHP files and 117k lines, so it is also the first PHP checkout large enough to say anything about an envelope. **WordPress never calls mysqli; it goes through $wpdb**, so the facts could not have found this. Added qualified: `$wpdb->get_var`, `->get_results`, `->get_row`, `->get_col`, `->query`, `->replace`. Qualified because a bare `get_var` or `query` matches any method of that name anywhere -- the generic-name trap that made `query.get` match every `.get(` in a repository this morning. `->insert`, `->update` and `->delete` are deliberately absent: they take arrays and WordPress prepares them itself. **And the thing that nearly went wrong.** The obvious next step when writing PHP facts is to add WordPress's sanitizers -- sanitize_text_field, esc_html, esc_attr. Doing so would have made this CVE invisible, because its handler calls `sanitize_text_field( $params['code'] )` and then concatenates the result into a query. sanitize_text_field is a real sanitizer that does nothing whatsoever about SQL injection. `TaintSpec.sanitizers` is one flat list per language whose comment states the assumption outright -- "a sanitizer that breaks one flow breaks it whatever the sink's family" -- and PHP falsifies it. So only SQL-specific cleansers are listed, which leaves XSS under-sanitized instead, and the real fix is recorded as task 5.6: a discharge counts for the obligation it discharges. That is task 2.6's finding arriving in the taint family, where the dominance arbiter already reads kind from the facts and the taint spec has no equivalent. The end-to-end scan of this checkout is not yet reported: the first run returned 1577 regions with 0 scanned, and the CPG builds standalone at 4.3MB with no errors, so the two do not yet agree. Numbers when they do. Gates byte-identical. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG --- .kiro/specs/contributor-scan/tasks.md | 16 +++++++++ benchmarks/repos/pmpro.toml | 37 ++++++++++++++++++++ src/openultrasast/ruleset/semantic/php.toml | 34 +++++++++++++++++- tests/test_model_specs.py | 38 +++++++++++++++++++++ 4 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 benchmarks/repos/pmpro.toml diff --git a/.kiro/specs/contributor-scan/tasks.md b/.kiro/specs/contributor-scan/tasks.md index 46e424d..21d65d9 100644 --- a/.kiro/specs/contributor-scan/tasks.md +++ b/.kiro/specs/contributor-scan/tasks.md @@ -460,6 +460,22 @@ - _Requirements: 4.3, 5.6, 9.1_ - _Depends: 5.4_ +- [ ] 5.6 A sanitizer cleanses the family it cleanses, not every family + - `TaintSpec.sanitizers` is one flat list per language, and its comment states the assumption plainly: + "a sanitizer that breaks one flow breaks it whatever the sink's family". PHP falsifies it. `esc_html` + stops XSS and does nothing for SQL injection; `esc_sql` is the reverse; `sanitize_text_field` stops + neither and is the most-used of the three. + - **CVE-2023-23488 is the proof.** Its handler calls `sanitize_text_field( $params['code'] )` and then + concatenates the result into a query. Adding WordPress's sanitizers to the flat list -- the obvious thing + to do when writing PHP facts -- would make that CVE and its whole class invisible. Task 5.1 therefore + carries only SQL-specific cleansers, which leaves XSS under-sanitized instead. + - This is 2.6's finding arriving in the taint family: a discharge counts for the obligation it discharges. + The dominance arbiter now reads `requires`/kind from the facts; the taint spec has no equivalent. + - Observable: `esc_html` marks an output_encoding flow sanitized and leaves an injection flow entailed, on + a pair that carries both. + - _Requirements: 8.1, 9.1_ + - _Depends: 5.1_ + ## Group 6 — Regression baselines - [ ] 6.1 Repository baseline and delta diff --git a/benchmarks/repos/pmpro.toml b/benchmarks/repos/pmpro.toml new file mode 100644 index 0000000..4e41661 --- /dev/null +++ b/benchmarks/repos/pmpro.toml @@ -0,0 +1,37 @@ +# Paid Memberships Pro 2.9.7 -- a WordPress plugin with an unauthenticated SQL injection. +# +# This is the WordPress DETECTION checkout, and the first one whose bug class the PHP fact tables were built +# for. 637 PHP files and 117k lines, so it is also the first PHP envelope measurement. +# +# The CVE was verified in the code rather than taken from an advisory. At this commit +# `classes/class.memberorder.php:936` reads: +# +# $id = $wpdb->get_var("SELECT id FROM $wpdb->pmpro_membership_orders WHERE code = '" . $code . "' LIMIT 1"); +# +# and 2.9.8 replaces it with `$wpdb->get_var( $wpdb->prepare( "... code = %s LIMIT 1", $code ) )`. The value +# reaches it from the `/pmpro/v1/order` REST route, whose handler does +# `$code = sanitize_text_field( $params['code'] )` -- which is a real WordPress sanitizer that does nothing +# whatsoever about SQL injection, and is why the flat sanitizer list must not carry it. + +name = "pmpro" +url = "https://github.com/strangerstudios/paid-memberships-pro.git" +commit = "1a900be3f09f243876150fc3c04a97bcb24ef84b" +language = "php" +license = "GPL-2.0" +license_file = "license.txt" +measures = ["detection", "envelope"] +note = """ +Tag 2.9.7, dated 2022-12-01; the fix is tag 2.9.8. Nothing is vendored -- the recipe fetches on request and +CI never does. 637 PHP files, 117302 lines, which makes this the first PHP checkout large enough to say +anything about the envelope. +""" + +[[known]] +id = "CVE-2023-23488" +family = "injection" +weakness = "unauthenticated SQL injection: the `code` parameter of the /pmpro/v1/order REST route is concatenated into a query without preparation" +file = "classes/class.memberorder.php" +function = "getMemberOrderByCode" +line = 936 +reference = "fixed in tag 2.9.8, which wraps the same query in $wpdb->prepare with a %s placeholder; the vulnerable line is verified in the checkout" +in_scope = true diff --git a/src/openultrasast/ruleset/semantic/php.toml b/src/openultrasast/ruleset/semantic/php.toml index 3630147..aa469b9 100644 --- a/src/openultrasast/ruleset/semantic/php.toml +++ b/src/openultrasast/ruleset/semantic/php.toml @@ -37,6 +37,26 @@ patterns = ["php://input"] # --- injection sinks --------------------------------------------------------------------------------- +# WordPress does not call mysqli directly; it goes through $wpdb. Verified against a CPG: `$wpdb->get_var(..)` +# is a call NAMED `get_var` whose CODE carries the qualifier, so the qualified form is what belongs here. A +# bare `get_var` or `query` would match any method of that name anywhere -- the generic-name trap that made +# `query.get` match every `.get(` in a repository. +# +# `$wpdb->insert`, `->update` and `->delete` are deliberately absent: they take arrays and WordPress prepares +# them itself, so they are not injection sinks. +[[sink]] +id = "wpdb" +cwe = "CWE-89" +calls = [ + "$wpdb->get_var", + "$wpdb->get_results", + "$wpdb->get_row", + "$wpdb->get_col", + "$wpdb->query", + "$wpdb->replace", +] +rule_ids = [] + [[sink]] id = "sql" cwe = "CWE-89" @@ -135,7 +155,19 @@ calls = ["htmlspecialchars", "htmlentities", "html_entity_decode_safe"] [[sanitizer]] id = "sql_escape" -calls = ["mysqli_real_escape_string", "mysql_real_escape_string", "pg_escape_string", "pg_escape_literal"] +calls = ["mysqli_real_escape_string", "mysql_real_escape_string", "pg_escape_string", "pg_escape_literal", "esc_sql"] + +# `$wpdb->prepare` is WordPress's parameterized form and appears ON the flow path, so it reads as a cleansing +# call rather than as a shape qualifier. +# +# NOTE what is NOT here. `sanitize_text_field`, `esc_html` and `esc_attr` are real WordPress sanitizers and +# none of them stops SQL injection -- CVE-2023-23488 is exactly a `sanitize_text_field` value concatenated +# into a query. Adding them would make that CVE, and its whole class, invisible. The sanitizer list is +# currently flat across families, which cannot express "escapes for HTML but not for SQL"; until it can, only +# SQL-specific cleansers belong here. +[[sanitizer]] +id = "wpdb_prepare" +calls = ["$wpdb->prepare"] [[sanitizer]] id = "shell_escape" diff --git a/tests/test_model_specs.py b/tests/test_model_specs.py index 04820da..4134120 100644 --- a/tests/test_model_specs.py +++ b/tests/test_model_specs.py @@ -284,3 +284,41 @@ def test_a_php_file_can_become_a_region() -> None: assert regions, "a php file with no entry point should still yield a file-level region" assert regions[0].language == "php" + + +def test_wordpress_database_calls_are_matched_qualified() -> None: + """contributor-scan: WordPress never calls mysqli directly, it goes through $wpdb. + + Verified against a CPG: `$wpdb->get_var(..)` is a call NAMED `get_var` whose CODE carries the qualifier. + The qualified form is what belongs in the table -- a bare `get_var` or `query` would match any method of + that name anywhere, which is the generic-name trap that made `query.get` match every `.get(` in a + repository. + """ + from openultrasast.model.specs import taint_specs + + injection = taint_specs(language="php")["injection"] + + assert "$wpdb->get_var" in injection.sinks + assert "$wpdb->query" in injection.sinks + assert "get_var" not in injection.sinks, "unqualified would match any method of that name" + assert "query" not in injection.sinks + # insert/update/delete take arrays and WordPress prepares them itself. + assert "$wpdb->insert" not in injection.sinks + + +def test_a_sanitizer_that_does_not_stop_sql_injection_is_not_listed() -> None: + """CVE-2023-23488 is a `sanitize_text_field` value concatenated into a query. + + `sanitize_text_field`, `esc_html` and `esc_attr` are real WordPress sanitizers and none of them stops SQL + injection. Listing them -- the obvious thing to do when writing PHP facts -- would make that CVE and its + whole class invisible, because the sanitizer list is flat across families and cannot say "escapes for + HTML but not for SQL". Recorded as task 5.6. + """ + from openultrasast.model.specs import taint_specs + + sanitizers = set(taint_specs(language="php")["injection"].sanitizers) + + assert "sanitize_text_field" not in sanitizers + assert "esc_html" not in sanitizers + assert "$wpdb->prepare" in sanitizers, "the parameterized form does stop it" + assert "esc_sql" in sanitizers