From f777be05b4851336a224c06a2b665561f678b95a Mon Sep 17 00:00:00 2001 From: ispyisail Date: Wed, 23 Sep 2026 03:23:40 +1200 Subject: [PATCH] Enforce read-only SQL with SQLite, not with a first-word check projectDataBase::isReadOnlySelect() decides whether a query only reads by looking at its first keyword and rejecting internal semicolons. SQLite has allowed a CTE prefix in front of a data-modifying statement since 3.8.3, so WITH x AS (SELECT 1) DELETE FROM element begins with WITH, contains no semicolon, passes the check, and deletes every row. UPDATE and INSERT go through the same way. This is not only reachable from the custom-query box. ProjectDBModel:: fromXml() reads a 's saved straight out of the .qet and fillValue() executes it, so a project file can carry the statement. Reproduced against a build of this branch's parent, with no scripting and no CLI flag beyond the export itself: a project whose stored table query was replaced with the DELETE above exported a bill of materials of 0 rows instead of 14, exit code 0, nothing logged. A silently empty or -- with UPDATE -- silently altered BOM is the kind of output someone orders parts from. Fixed by asking SQLite about the statement it actually compiled. sqlite3_prepare_v2() compiles without running, sqlite3_stmt_readonly() reports on the compiled statement rather than on how it was spelled, and the prepare tail catches a second statement structurally. The same project now exports its 14 rows again and logs a reason for the refusal, while an ordinary WITH ... SELECT in a project file still runs untouched -- the fix is not "ban CTEs". isReadOnlySelect() stays in front of it rather than being replaced: SQLite considers ATTACH, BEGIN and several PRAGMAs read-only too, since none of them change the contents of the database, so dropping the statement-type allowlist would have widened what is accepted while fixing what is executed. The check lives in its own translation unit depending on nothing but QString and SQLite, so tests/qttest/tst_sqlreadonly.cpp can link it alone and exercise the security property without standing up a QETProject: 18 assertions covering the three CTE-prefixed writes named in the review, bare writes, trailing statements, comment-only input (which compiles to a null statement sqlite3_stmt_readonly() must not be handed) and a null connection (refused, not waved through). Confirmed the suite discriminates by deliberately disabling the new check and watching exactly the nine write-refusal assertions go red while the accept cases stayed green. ctest 13/13, qet-coherence-check and qet-pdflink-check clean on the example corpus. Reported in PR #980's review thread by @elevatormind and confirmed against this code by @scorpio810; fixed here on its own because the flaw is in already-released code and needs none of that branch to reach. Co-Authored-By: Claude Opus 5 (1M context) --- cmake/qet_compilation_vars.cmake | 2 + sources/dataBase/projectdatabase.cpp | 29 +++- sources/dataBase/projectdatabase.h | 7 +- sources/dataBase/sqlreadonly.cpp | 134 ++++++++++++++++++ sources/dataBase/sqlreadonly.h | 43 ++++++ tests/qttest/CMakeLists.txt | 16 +++ tests/qttest/tst_sqlreadonly.cpp | 198 +++++++++++++++++++++++++++ 7 files changed, 426 insertions(+), 3 deletions(-) create mode 100644 sources/dataBase/sqlreadonly.cpp create mode 100644 sources/dataBase/sqlreadonly.h create mode 100644 tests/qttest/tst_sqlreadonly.cpp diff --git a/cmake/qet_compilation_vars.cmake b/cmake/qet_compilation_vars.cmake index 738764732..385658ffd 100644 --- a/cmake/qet_compilation_vars.cmake +++ b/cmake/qet_compilation_vars.cmake @@ -312,6 +312,8 @@ set(QET_SRC_FILES ${QET_DIR}/sources/dataBase/projectdatabase.cpp ${QET_DIR}/sources/dataBase/projectdatabase.h + ${QET_DIR}/sources/dataBase/sqlreadonly.cpp + ${QET_DIR}/sources/dataBase/sqlreadonly.h ${QET_DIR}/sources/dataBase/ui/elementquerywidget.cpp ${QET_DIR}/sources/dataBase/ui/elementquerywidget.h diff --git a/sources/dataBase/projectdatabase.cpp b/sources/dataBase/projectdatabase.cpp index 20d2c150d..adfd45283 100644 --- a/sources/dataBase/projectdatabase.cpp +++ b/sources/dataBase/projectdatabase.cpp @@ -17,6 +17,8 @@ */ #include "projectdatabase.h" +#include "sqlreadonly.h" + #include "../diagram.h" #include "../diagramposition.h" #include "../elementprovider.h" @@ -208,6 +210,12 @@ bool projectDataBase::isReadOnlySelect(const QString &query, QString *error) */ QSqlQuery projectDataBase::newQuery(const QString &query, QString *error) { QString reason; + + // First gate: which kind of statement is acceptable here at all. A + // textual check is the right tool for that and the wrong tool for + // anything else -- see isReadOnlySelect()'s own comment. It is what + // keeps ATTACH, BEGIN and PRAGMA out, none of which SQLite itself + // considers writes. if (!isReadOnlySelect(query, &reason)) { qWarning().noquote() << "projectDataBase::newQuery: rejected query:" << reason << "--" << query; if (error) { @@ -215,6 +223,24 @@ QSqlQuery projectDataBase::newQuery(const QString &query, QString *error) { } return QSqlQuery(m_data_base); } + + // Second gate, and the one that actually enforces read-only: SQLite is + // asked about the statement it compiled, instead of the text being read + // for clues. The first gate cannot see through a CTE prefix -- + // "WITH x AS (SELECT 1) DELETE FROM element" starts with WITH, contains + // no semicolon, and deletes every row. That matters beyond the + // custom-query box, because this path is reachable from a file: a + // 's saved is read straight out of the .qet by + // ProjectDBModel::fromXml() and executed by fillValue(), so opening or + // exporting a project someone else produced would have been enough. + if (!QETSql::isSingleReadOnlyStatement(sqliteHandle(&m_data_base), query, &reason)) { + qWarning().noquote() << "projectDataBase::newQuery: rejected query:" << reason << "--" << query; + if (error) { + *error = reason; + } + return QSqlQuery(m_data_base); + } + return QSqlQuery(query, m_data_base); } @@ -1245,7 +1271,6 @@ void projectDataBase::bindDiagramInfoValues(QSqlQuery &query, Diagram *diagram) } } -#ifdef QET_EXPORT_PROJECT_DB /** @brief projectDataBase::sqliteHandle @param db @@ -1263,7 +1288,7 @@ sqlite3 *projectDataBase::sqliteHandle(QSqlDatabase *db) return handle; } - +#ifdef QET_EXPORT_PROJECT_DB /** * @brief projectDataBase::exportDb * Export the db, to a file. diff --git a/sources/dataBase/projectdatabase.h b/sources/dataBase/projectdatabase.h index abd5e966c..b8b1ead9c 100644 --- a/sources/dataBase/projectdatabase.h +++ b/sources/dataBase/projectdatabase.h @@ -135,9 +135,14 @@ class projectDataBase : public QObject m_cascade_remove_conductor_query, m_cascade_remove_element_query; + public: + // Deliberately outside the QET_EXPORT_PROJECT_DB guard below: + // newQuery() needs the raw connection to ask SQLite whether a + // query only reads, and that check runs in every build. + static sqlite3 *sqliteHandle(QSqlDatabase *db); + #ifdef QET_EXPORT_PROJECT_DB public: - static sqlite3 *sqliteHandle(QSqlDatabase *db); static void exportDb(projectDataBase *db, QWidget *parent = nullptr, const QString &caption = QString(), diff --git a/sources/dataBase/sqlreadonly.cpp b/sources/dataBase/sqlreadonly.cpp new file mode 100644 index 000000000..48cc19ccc --- /dev/null +++ b/sources/dataBase/sqlreadonly.cpp @@ -0,0 +1,134 @@ +/* + Copyright 2006-2026 The QElectroTech Team + This file is part of QElectroTech. + + QElectroTech is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 2 of the License, or + (at your option) any later version. + + QElectroTech is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with QElectroTech. If not, see . +*/ +#include "sqlreadonly.h" + +#include + +#include + +namespace QETSql { + +/** + @brief QETSql::isSingleReadOnlyStatement + Ask SQLite itself whether @p query is exactly one statement, and whether + that statement only reads. + + Why SQLite is asked rather than the text inspected: a check on the + query's first keyword cannot see what the statement actually does. + SQLite has allowed a CTE prefix in front of a data-modifying statement + since 3.8.3, so + + @code + WITH x AS (SELECT 1) DELETE FROM element + @endcode + + begins with WITH, contains no semicolon, and deletes every row. + sqlite3_stmt_readonly() reports on the statement SQLite compiled, not + on how it was spelled, so the same query is correctly refused here + while an ordinary WITH ... SELECT still passes. + + The statement is compiled and immediately finalised; sqlite3_prepare_v2() + does not run it, so nothing is executed to reach this verdict. + + This is a read-only test, NOT a statement-type allowlist: SQLite + considers ATTACH, BEGIN and several PRAGMAs read-only too, because none + of them change the contents of the database. Callers that need to + restrict which *kind* of statement is acceptable must say so separately + -- projectDataBase::newQuery() keeps isReadOnlySelect() in front of this + for exactly that reason. + + @param handle the connection the query would run on. A null handle is + refused rather than waved through: without it there is nothing to ask, + and guessing from the text is the weakness this exists to replace. + @param query the raw SQL text + @param error set to a human-readable reason when this returns false + @return true if @p query is a single, read-only statement +*/ +bool isSingleReadOnlyStatement(sqlite3 *handle, const QString &query, QString *error) +{ + if (error) { + error->clear(); + } + + if (!handle) { + if (error) { + *error = QCoreApplication::translate("QETSql", + "Impossible de vérifier la requête : " + "aucune connexion SQLite disponible."); + } + return false; + } + + const QByteArray utf8 = query.toUtf8(); + sqlite3_stmt *statement = nullptr; + const char *tail = nullptr; + + if (sqlite3_prepare_v2(handle, utf8.constData(), utf8.size(), + &statement, &tail) != SQLITE_OK) + { + if (error) { + *error = QCoreApplication::translate("QETSql", + "Requête SQL invalide : %1") + .arg(QString::fromUtf8(sqlite3_errmsg(handle))); + } + sqlite3_finalize(statement); + return false; + } + + // Whitespace or a bare comment compiles successfully to no statement + // at all, and sqlite3_stmt_readonly() must not be handed that. + if (!statement) { + if (error) { + *error = QCoreApplication::translate("QETSql", + "La requête ne contient aucune instruction."); + } + return false; + } + + const bool read_only = sqlite3_stmt_readonly(statement) != 0; + sqlite3_finalize(statement); + + if (!read_only) { + if (error) { + *error = QCoreApplication::translate("QETSql", + "Seules les requêtes en lecture seule sont autorisées : " + "cette requête modifierait la base de données."); + } + return false; + } + + // tail points just past the first statement, semicolon included. + // Anything left once semicolons and spacing are stripped is a second + // statement -- caught structurally here, where "SELECT ';'" is a + // perfectly ordinary query rather than a suspicious string. + if (tail) { + QString rest = QString::fromUtf8(tail); + rest.remove(QLatin1Char(';')); + if (!rest.trimmed().isEmpty()) { + if (error) { + *error = QCoreApplication::translate("QETSql", + "Une seule requête est autorisée."); + } + return false; + } + } + + return true; +} + +} // namespace QETSql diff --git a/sources/dataBase/sqlreadonly.h b/sources/dataBase/sqlreadonly.h new file mode 100644 index 000000000..6489e8bc9 --- /dev/null +++ b/sources/dataBase/sqlreadonly.h @@ -0,0 +1,43 @@ +/* + Copyright 2006-2026 The QElectroTech Team + This file is part of QElectroTech. + + QElectroTech is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 2 of the License, or + (at your option) any later version. + + QElectroTech is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with QElectroTech. If not, see . +*/ +#ifndef SQLREADONLY_H +#define SQLREADONLY_H + +#include + +struct sqlite3; + +/** + Deciding whether a piece of SQL only reads. + + Deliberately its own translation unit, depending on nothing but QString + and SQLite: it is the enforcement point for every query QElectroTech + runs against a project database, including queries that arrive from + outside the application (a .qet file's saved report/table query), so it + is worth being able to test it in isolation -- see + tests/qttest/tst_sqlreadonly.cpp, which links this file and nothing + else of QElectroTech. +*/ +namespace QETSql { + + bool isSingleReadOnlyStatement(sqlite3 *handle, + const QString &query, + QString *error = nullptr); +} + +#endif // SQLREADONLY_H diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index 0b140cedf..b3c86cd40 100644 --- a/tests/qttest/CMakeLists.txt +++ b/tests/qttest/CMakeLists.txt @@ -143,6 +143,22 @@ add_test(NAME tst_qetstrings COMMAND tst_qetstrings) target_include_directories(tst_qetstrings PRIVATE ${QET_DIR}/sources) target_link_libraries(tst_qetstrings PRIVATE Qt::Test Qt::Widgets Qt::Xml pugixml::pugixml) +# QETSql::isSingleReadOnlyStatement() -- read-only enforcement for every +# project-database query, including the ones a .qet file carries. Compiles +# sqlreadonly.cpp alone against its own in-memory SQLite, so the security +# property is checked without standing up a QETProject. +find_package(SQLite3 REQUIRED) +if(NOT TARGET SQLite3::SQLite3 AND TARGET SQLite::SQLite3) + add_library(SQLite3::SQLite3 ALIAS SQLite::SQLite3) +endif() +add_executable( + tst_sqlreadonly + tst_sqlreadonly.cpp + ${QET_DIR}/sources/dataBase/sqlreadonly.cpp) +add_test(NAME tst_sqlreadonly COMMAND tst_sqlreadonly) +target_include_directories(tst_sqlreadonly PRIVATE ${QET_DIR}/sources) +target_link_libraries(tst_sqlreadonly PRIVATE Qt::Test SQLite3::SQLite3) + # CrashHandler::formatInt() -- the async-signal-safe decimal formatter the # signal handler uses for the "Signal: N" line of a crash dump. Compiles # crashhandler.cpp and logring.cpp alongside; the handler deliberately diff --git a/tests/qttest/tst_sqlreadonly.cpp b/tests/qttest/tst_sqlreadonly.cpp new file mode 100644 index 000000000..d6c1f5882 --- /dev/null +++ b/tests/qttest/tst_sqlreadonly.cpp @@ -0,0 +1,198 @@ +/* + Copyright 2006-2026 The QElectroTech Team + This file is part of QElectroTech. + + QElectroTech is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 2 of the License, or + (at your option) any later version. + + QElectroTech is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with QElectroTech. If not, see . +*/ + +/* + QETSql::isSingleReadOnlyStatement() -- the read-only enforcement every + project-database query goes through. + + The case that matters most here is the CTE prefix. SQLite has allowed + WITH in front of DELETE/UPDATE/INSERT since 3.8.3, so a check on the + first keyword lets a write through while reading as though it refused + one. That is not hypothetical for QElectroTech: a 's + is stored in the .qet and executed on load, so the text can + arrive from a file rather than from the person at the keyboard. + + This test owns its own in-memory database and links nothing of + QElectroTech but sqlreadonly.cpp, so it stays a fast, hermetic check of + the security property itself. +*/ + +#include "dataBase/sqlreadonly.h" + +#include + +#include + +class TstSqlReadOnly : public QObject +{ + Q_OBJECT + + private slots: + void initTestCase(); + void cleanupTestCase(); + + void acceptsOrdinaryReads(); + void acceptsLegitimateCommonTableExpression(); + void refusesCtePrefixedWrites_data(); + void refusesCtePrefixedWrites(); + void refusesBareWrites_data(); + void refusesBareWrites(); + void refusesTrailingStatement(); + void refusesEmptyAndCommentOnly_data(); + void refusesEmptyAndCommentOnly(); + void refusesWithoutAConnection(); + void reportsAReason(); + void doesNotExecuteWhatItRefuses(); + + private: + sqlite3 *m_db = nullptr; + int rowCount(); +}; + +int TstSqlReadOnly::rowCount() +{ + sqlite3_stmt *st = nullptr; + sqlite3_prepare_v2(m_db, "SELECT COUNT(*) FROM element", -1, &st, nullptr); + sqlite3_step(st); + const int n = sqlite3_column_int(st, 0); + sqlite3_finalize(st); + return n; +} + +void TstSqlReadOnly::initTestCase() +{ + QCOMPARE(sqlite3_open(":memory:", &m_db), SQLITE_OK); + QCOMPARE(sqlite3_exec(m_db, + "CREATE TABLE element (uuid TEXT);" + "INSERT INTO element VALUES ('a'),('b');", nullptr, nullptr, nullptr), + SQLITE_OK); + QCOMPARE(rowCount(), 2); +} + +void TstSqlReadOnly::cleanupTestCase() +{ + sqlite3_close(m_db); + m_db = nullptr; +} + +void TstSqlReadOnly::acceptsOrdinaryReads() +{ + QVERIFY(QETSql::isSingleReadOnlyStatement(m_db, "SELECT * FROM element")); + QVERIFY(QETSql::isSingleReadOnlyStatement(m_db, "SELECT uuid FROM element WHERE uuid = 'a'")); + // A semicolon inside a string literal is not a second statement. The + // textual check this replaced rejected exactly this. + QVERIFY(QETSql::isSingleReadOnlyStatement(m_db, "SELECT ';' AS semicolon")); + // One trailing semicolon is ordinary punctuation, not a second statement. + QVERIFY(QETSql::isSingleReadOnlyStatement(m_db, "SELECT * FROM element;")); +} + +void TstSqlReadOnly::acceptsLegitimateCommonTableExpression() +{ + // WITH must keep working -- the fix is not "ban CTEs". + QVERIFY(QETSql::isSingleReadOnlyStatement(m_db, + "WITH x AS (SELECT 1 AS n) SELECT n FROM x")); + QVERIFY(QETSql::isSingleReadOnlyStatement(m_db, + "WITH RECURSIVE c(n) AS (SELECT 1 UNION ALL SELECT n+1 FROM c) " + "SELECT n FROM c LIMIT 3")); +} + +void TstSqlReadOnly::refusesCtePrefixedWrites_data() +{ + QTest::addColumn("query"); + QTest::newRow("delete") << "WITH x AS (SELECT 1) DELETE FROM element"; + QTest::newRow("update") << "WITH x AS (SELECT 1) UPDATE element SET uuid = 'pwned'"; + QTest::newRow("insert") << "WITH x AS (SELECT 1) INSERT INTO element VALUES ('injected')"; +} + +void TstSqlReadOnly::refusesCtePrefixedWrites() +{ + QFETCH(QString, query); + QVERIFY2(!QETSql::isSingleReadOnlyStatement(m_db, query), + qPrintable(QStringLiteral("accepted a write: %1").arg(query))); +} + +void TstSqlReadOnly::refusesBareWrites_data() +{ + QTest::addColumn("query"); + QTest::newRow("delete") << "DELETE FROM element"; + QTest::newRow("update") << "UPDATE element SET uuid = 'pwned'"; + QTest::newRow("insert") << "INSERT INTO element VALUES ('injected')"; + QTest::newRow("drop") << "DROP TABLE element"; +} + +void TstSqlReadOnly::refusesBareWrites() +{ + QFETCH(QString, query); + QVERIFY(!QETSql::isSingleReadOnlyStatement(m_db, query)); +} + +void TstSqlReadOnly::refusesTrailingStatement() +{ + QVERIFY(!QETSql::isSingleReadOnlyStatement(m_db, "SELECT 1; DROP TABLE element")); + QVERIFY(!QETSql::isSingleReadOnlyStatement(m_db, "SELECT 1; SELECT 2")); +} + +void TstSqlReadOnly::refusesEmptyAndCommentOnly_data() +{ + QTest::addColumn("query"); + QTest::newRow("empty") << ""; + QTest::newRow("whitespace") << " "; + QTest::newRow("comment") << "-- nothing to see here"; +} + +void TstSqlReadOnly::refusesEmptyAndCommentOnly() +{ + // sqlite3_prepare_v2() reports success and a null statement for these; + // sqlite3_stmt_readonly() must never be handed that. + QFETCH(QString, query); + QVERIFY(!QETSql::isSingleReadOnlyStatement(m_db, query)); +} + +void TstSqlReadOnly::refusesWithoutAConnection() +{ + // Fails closed: with no connection there is nothing to ask, and + // guessing from the text is the weakness this replaced. + QVERIFY(!QETSql::isSingleReadOnlyStatement(nullptr, "SELECT * FROM element")); +} + +void TstSqlReadOnly::reportsAReason() +{ + QString reason; + QVERIFY(!QETSql::isSingleReadOnlyStatement( + m_db, "WITH x AS (SELECT 1) DELETE FROM element", &reason)); + QVERIFY2(!reason.isEmpty(), "a refusal must say why"); + + reason = QStringLiteral("stale"); + QVERIFY(QETSql::isSingleReadOnlyStatement(m_db, "SELECT * FROM element", &reason)); + QVERIFY2(reason.isEmpty(), "an accepted query must not leave a reason behind"); +} + +void TstSqlReadOnly::doesNotExecuteWhatItRefuses() +{ + // The check compiles the statement to inspect it. Proving the table is + // untouched afterwards is what says it compiled without running it -- + // and this same assertion goes red if the refusals above ever stop + // refusing, since then the caller would run the DELETE for real. + QCOMPARE(rowCount(), 2); + QVERIFY(!QETSql::isSingleReadOnlyStatement(m_db, "WITH x AS (SELECT 1) DELETE FROM element")); + QVERIFY(!QETSql::isSingleReadOnlyStatement(m_db, "DELETE FROM element")); + QCOMPARE(rowCount(), 2); +} + +QTEST_APPLESS_MAIN(TstSqlReadOnly) +#include "tst_sqlreadonly.moc"