mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG
This commit is contained in:
co-authored by
Claude Opus 5
parent
47b00d739b
commit
922ed84053
@@ -460,6 +460,22 @@
|
|||||||
- _Requirements: 4.3, 5.6, 9.1_
|
- _Requirements: 4.3, 5.6, 9.1_
|
||||||
- _Depends: 5.4_
|
- _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
|
## Group 6 — Regression baselines
|
||||||
|
|
||||||
- [ ] 6.1 Repository baseline and delta
|
- [ ] 6.1 Repository baseline and delta
|
||||||
|
|||||||
@@ -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
|
||||||
@@ -37,6 +37,26 @@ patterns = ["php://input"]
|
|||||||
|
|
||||||
# --- injection sinks ---------------------------------------------------------------------------------
|
# --- 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]]
|
[[sink]]
|
||||||
id = "sql"
|
id = "sql"
|
||||||
cwe = "CWE-89"
|
cwe = "CWE-89"
|
||||||
@@ -135,7 +155,19 @@ calls = ["htmlspecialchars", "htmlentities", "html_entity_decode_safe"]
|
|||||||
|
|
||||||
[[sanitizer]]
|
[[sanitizer]]
|
||||||
id = "sql_escape"
|
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]]
|
[[sanitizer]]
|
||||||
id = "shell_escape"
|
id = "shell_escape"
|
||||||
|
|||||||
@@ -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, "a php file with no entry point should still yield a file-level region"
|
||||||
assert regions[0].language == "php"
|
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
|
||||||
|
|||||||
Reference in New Issue
Block a user