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