CVE-2023-47128: Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') (CWE-89)
value: Piccolo is an object-relational mapping and query builder which supports asyncio. Prior to version 1.1.1, the handling of named transaction `savepoints` in all database implementations is vulnerable to SQL Injection via f-strings. While the likelihood of an end developer exposing a `savepoints` `name` parameter to a user is highly unlikely, it would not be unheard of. If a malicious user was able to abuse this functionality they would have essentially direct access to the database and the ability to modify data to the level of permissions associated with the database user. A non exhaustive list of actions possible based on database permissions is: Read all data stored in the database, including usernames and password hashes; insert arbitrary data into the database, including modifying existing records; and gain a shell on the underlying server. Version 1.1.1 fixes this issue.
CWE description: The product constructs all or part of an SQL command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended SQL command when it is sent to a downstream component. Without sufficient removal or quoting of SQL syntax in user-controllable inputs, the generated SQL query can cause those inputs to be interpreted as SQL instead of ordinary user data.
CVSS v3 base score: 9.1
Published: 2023-11-10T18:15Z
Repository: https://github.com/piccolo-orm/piccolo
Language: Python
Files changed: piccolo/engine/base.py, piccolo/engine/postgres.py, piccolo/engine/sqlite.py, tests/engine/test_transaction.py
Commit date: 2023-11-10 17:20:01 +0000
Commit message: Merge pull request from GHSA-xq59-7jf3-rjc6

Co-authored-by: skelmis <ethan.mckee-harris@zxsecurity.co.nz>
Diff: diff --git a/piccolo/engine/base.py b/piccolo/engine/base.py
index 77de5ceb..95d1b8a2 100644
--- a/piccolo/engine/base.py
+++ b/piccolo/engine/base.py
@@ -1,10 +1,11 @@
 from __future__ import annotations
 
 import contextvars
 import logging
 import pprint
+import string
 import typing as t
 from abc import ABCMeta, abstractmethod
 
 from piccolo.querystring import QueryString
 from piccolo.utils.sync import run_sync
@@ -13,10 +14,24 @@ from piccolo.utils.warnings import Level, colored_string, colored_warning
 if t.TYPE_CHECKING:  # pragma: no cover
     from piccolo.query.base import Query
 
 
 logger = logging.getLogger(__name__)
+# This is a set to speed up lookups from O(n) when
+# using str vs O(1) when using set[str]
+VALID_SAVEPOINT_CHARACTERS: t.Final[set[str]] = set(
+    string.ascii_letters + string.digits + "-" + "_"
+)
+
+
+def validate_savepoint_name(savepoint_name: str) -> None:
+    """Validates a save point's name meets the required character set."""
+    if not all(i in VALID_SAVEPOINT_CHARACTERS for i in savepoint_name):
+        raise ValueError(
+            "Savepoint names can only contain the following characters:"
+            f" {VALID_SAVEPOINT_CHARACTERS}"
+        )
 
 
 class Batch:
     pass
 
diff --git a/piccolo/engine/postgres.py b/piccolo/engine/postgres.py
index 42a8d6bc..b5c17970 100644
--- a/piccolo/engine/postgres.py
+++ b/piccolo/engine/postgres.py
@@ -2,11 +2,11 @@ from __future__ import annotations
 
 import contextvars
 import typing as t
 from dataclasses import dataclass
 
-from piccolo.engine.base import Batch, Engine
+from piccolo.engine.base import Batch, Engine, validate_savepoint_name
 from piccolo.engine.exceptions import TransactionError
 from piccolo.query.base import DDL, Query
 from piccolo.querystring import QueryString
 from piccolo.utils.lazy_loader import LazyLoader
 from piccolo.utils.sync import run_sync
@@ -127,15 +127,17 @@ class Savepoint:
     def __init__(self, name: str, transaction: PostgresTransaction):
         self.name = name
         self.transaction = transaction
 
     async def rollback_to(self):
+        validate_savepoint_name(self.name)
         await self.transaction.connection.execute(
             f"ROLLBACK TO SAVEPOINT {self.name}"
         )
 
     async def release(self):
+        validate_savepoint_name(self.name)
         await self.transaction.connection.execute(
             f"RELEASE SAVEPOINT {self.name}"
         )
 
 
@@ -234,10 +236,11 @@ class PostgresTransaction:
         self._savepoint_id += 1
         return self._savepoint_id
 
     async def savepoint(self, name: t.Optional[str] = None) -> Savepoint:
         name = name or f"savepoint_{self.get_savepoint_id()}"
+        validate_savepoint_name(name)
         await self.connection.execute(f"SAVEPOINT {name}")
         return Savepoint(name=name, transaction=self)
 
     ###########################################################################
 
diff --git a/piccolo/engine/sqlite.py b/piccolo/engine/sqlite.py
index 9084baf7..7d0b3eae 100644
--- a/piccolo/engine/sqlite.py
+++ b/piccolo/engine/sqlite.py
@@ -8,11 +8,11 @@ import sqlite3
 import typing as t
 import uuid
 from dataclasses import dataclass
 from decimal import Decimal
 
-from piccolo.engine.base import Batch, Engine
+from piccolo.engine.base import Batch, Engine, validate_savepoint_name
 from piccolo.engine.exceptions import TransactionError
 from piccolo.query.base import DDL, Query
 from piccolo.querystring import QueryString
 from piccolo.utils.encoding import dump_json, load_json
 from piccolo.utils.lazy_loader import LazyLoader
@@ -307,15 +307,17 @@ class Savepoint:
     def __init__(self, name: str, transaction: SQLiteTransaction):
         self.name = name
         self.transaction = transaction
 
     async def rollback_to(self):
+        validate_savepoint_name(self.name)
         await self.transaction.connection.execute(
             f"ROLLBACK TO SAVEPOINT {self.name}"
         )
 
     async def release(self):
+        validate_savepoint_name(self.name)
         await self.transaction.connection.execute(
             f"RELEASE SAVEPOINT {self.name}"
         )
 
 
@@ -411,10 +413,11 @@ class SQLiteTransaction:
         self._savepoint_id += 1
         return self._savepoint_id
 
     async def savepoint(self, name: t.Optional[str] = None) -> Savepoint:
         name = name or f"savepoint_{self.get_savepoint_id()}"
+        validate_savepoint_name(name)
         await self.connection.execute(f"SAVEPOINT {name}")
         return Savepoint(name=name, transaction=self)
 
     ###########################################################################
 
diff --git a/tests/engine/test_transaction.py b/tests/engine/test_transaction.py
index bf248961..3cba32c8 100644
--- a/tests/engine/test_transaction.py
+++ b/tests/engine/test_transaction.py
@@ -1,9 +1,11 @@
 import asyncio
 import typing as t
 from unittest import TestCase
 
+import pytest
+
 from piccolo.engine.postgres import Atomic
 from piccolo.engine.sqlite import SQLiteEngine, TransactionType
 from piccolo.table import drop_db_tables_sync
 from piccolo.utils.sync import run_sync
 from tests.base import engines_only
@@ -294,5 +296,16 @@ class TestSavepoint(TestCase):
         run_sync(run_test())
 
         self.assertListEqual(
             Manager.select(Manager.name).run_sync(), [{"name": "Manager 1"}]
         )
+
+    def test_savepoint_sqli_checks(self):
+        # Added to test the fix for GHSA-xq59-7jf3-rjc6
+        async def run_test():
+            async with Manager._meta.db.transaction() as transaction:
+                await transaction.savepoint(
+                    "my_savepoint; SELECT * FROM Manager"
+                )
+
+        with pytest.raises(ValueError):
+            run_sync(run_test())

Vulnerable code: from piccolo.engine.base import Batch, Engine
from piccolo.engine.base import Batch, Engine
Fixed code: import string
# This is a set to speed up lookups from O(n) when
# using str vs O(1) when using set[str]
VALID_SAVEPOINT_CHARACTERS: t.Final[set[str]] = set(
    string.ascii_letters + string.digits + "-" + "_"
)


def validate_savepoint_name(savepoint_name: str) -> None:
    """Validates a save point's name meets the required character set."""
    if not all(i in VALID_SAVEPOINT_CHARACTERS for i in savepoint_name):
        raise ValueError(
            "Savepoint names can only contain the following characters:"
            f" {VALID_SAVEPOINT_CHARACTERS}"
        )
from piccolo.engine.base import Batch, Engine, validate_savepoint_name
        validate_savepoint_name(self.name)
        validate_savepoint_name(self.name)
        validate_savepoint_name(name)
from piccolo.engine.base import Batch, Engine, validate_savepoint_name
        validate_savepoint_name(self.name)
        validate_savepoint_name(self.name)
        validate_savepoint_name(name)
import pytest


    def test_savepoint_sqli_checks(self):
        # Added to test the fix for GHSA-xq59-7jf3-rjc6
        async def run_test():
            async with Manager._meta.db.transaction() as transaction:
                await transaction.savepoint(
                    "my_savepoint; SELECT * FROM Manager"
                )

        with pytest.raises(ValueError):
            run_sync(run_test())