GHSA-rw77-vq4g-x3hp

## Summary

The `StopWords::add()` method in phpMyFAQ builds a SQL `INSERT` statement using `sprintf()` and inserts the user-supplied stop word value directly into the query string **without calling the application's database escaping function** on it. A sibling method, `StopWords::update()`, which modifies an *existing* stop word, correctly escapes the same kind of input. The omission is isolated to the `add()` (insert) code path.

An authenticated administrator who can reach the stop-word management feature can submit a crafted value as the "word" parameter that breaks out of the SQL string literal and injects arbitrary SQL, including statements to drop tables, exfiltrate data, or modify other rows in the database.

---

## Affected Code

**File:** `phpmyfaq/src/phpMyFAQ/StopWords.php` **Method:** `add()` (approx. lines 60–75 in the audited revision)

```php $sql = sprintf( "INSERT INTO %s VALUES(%d, '%s', '%s')", $this->getTableName(), $id, $this->configuration->getDb()->escape($this->language), // language IS escaped $word // <-- $word is NOT escaped ); ```

`$word` is taken directly from the administrative form input (the new stop word to add) and concatenated into the SQL string via `sprintf("'%s'", ...)` with no call to the database driver's `escape()` method.

### Contrast with the safe sibling method

**Method:** `update()` (line 82 in the audited revision)

```php $this->configuration->getDb()->escape($word) ```

`update()` — which modifies an existing stop word — correctly escapes `$word` before use. `add()` does not perform the same escaping on the equivalent value. This inconsistency between two methods handling the same data type is the root cause: the escaping convention used throughout the rest of the file was not applied uniformly to this one insertion path.

---

## Proof of Concept

**Precondition:** Attacker has valid administrator credentials (or has otherwise obtained an authenticated administrator session, e.g. via a separate session-hijacking or CSRF vector).

**Attack steps:**

1. Authenticate to the phpMyFAQ administration panel. 2. Navigate to the Stop Words management feature. 3. Submit a new stop word with the following value instead of a normal word:

``` test', 'en'); DROP TABLE faqstopwords; -- ```

4. The resulting SQL statement sent to the database becomes (table/column names approximate, based on the traced `sprintf` template):

```sql INSERT INTO faqstopwords VALUES(1, 'en', 'test', 'en'); DROP TABLE faqstopwords; --') ```

5. The injected `DROP TABLE faqstopwords;` statement executes as a second SQL statement (subject to the database driver/PDO configuration permitting multi-statement execution; even where multi-statement execution is disabled, the same injection point allows classic single-statement SQLi techniques such as `UNION`-based data extraction or boolean/time-based blind injection against other tables the database user can access).

---

## Impact

- **Confidentiality:** An attacker with this access can use UNION-based or blind SQL injection techniques to read data from other tables in the database (e.g. user credentials, FAQ content marked as private/internal, session data) that the database user account has permission to access. - **Integrity:** Arbitrary `INSERT`/`UPDATE`/`DELETE` statements can be appended, allowing modification of unrelated application data. - **Availability:** As demonstrated in the PoC, structural statements like `DROP TABLE` can be injected, directly impacting application availability.

**Mitigating factor:** Exploitation requires an authenticated administrator session. This is not exploitable by an anonymous or low-privilege user. This lowers the severity from Critical/High to Medium, consistent with phpMyFAQ's own threat model where administrators are a trusted role — but it remains a genuine defense-in-depth failure: a compromised or malicious admin account (or an admin tricked via a separate vector such as CSRF, if no CSRF protection exists on this specific form) can leverage this into full database compromise, which a properly parameterized query would have prevented even in that scenario.

---

## Root Cause

The codebase's established pattern for this class (`StopWords.php`) is to escape all string values via `$this->configuration->getDb()->escape($value)` before placing them into a `sprintf()`-built SQL string. This pattern is correctly applied to:

- `$this->language` in `add()` - `$word` in `update()`

It is **not** applied to `$word` in `add()`. This is a single-line omission, not a structural design flaw — the safe pattern already exists in the same file and the same class, just inconsistently applied across the two methods that handle the same input type.

---

## Recommended Fix

Apply the same escaping already used in `update()` and already used for `$this->language` in the same `add()` method:

```php $sql = sprintf( "INSERT INTO %s VALUES(%d, '%s', '%s')", $this->getTableName(), $id, $this->configuration->getDb()->escape($this->language), $this->configuration->getDb()->escape($word) // FIX: escape $word here ); ```

**Stronger recommended fix (defense in depth):** Migrate this query, and ideally all `sprintf()`-built SQL in this class, to parameterized/prepared statements (e.g. `PDO::prepare()` with bound parameters) rather than string-escaping plus `sprintf()`. Escaping is correct when applied consistently, but prepared statements remove this entire vulnerability class structurally and prevent any future omission of this kind from being exploitable.