Problem
Dozens of SQL queries are built with QString::arg() string concatenation instead of prepared statements with bound values. Several of these include data that crosses a trust boundary:
Data from imported ADIF files and online services (most important):
- Callsigns in WHERE clauses:
src/dataproxy_sqlite.cpp:919, :979, :3461, :3465
- Callsign from LoTW download responses:
src/dataproxy_sqlite.cpp:3718-3719
- Station callsign in UPDATE:
src/dataproxy_sqlite.cpp:3651
User-editable log metadata:
stationcall, operators, comment concatenated into UPDATE/INSERT/SELECT on the logs table: src/dataproxy_sqlite.cpp:6941-6942, :6965, :6990-6991
Other widespread instances throughout src/dataproxy_sqlite.cpp (log number, satellite names, DXCC/continent lookups, e.g. :457, :729, :5088, :7089...) and src/searchwindow.cpp:650.
Impact
A crafted callsign or comment field — e.g. X' OR '1'='1 — in a shared ADIF file (operators routinely exchange these) or in a manipulated online-service response can break queries or corrupt/delete log data. Realistic impact is data corruption / denial of service of the user's log rather than code execution, but the inputs are genuinely untrusted, and a corrupted logbook is a serious loss for the user.
There is also a correctness bug today: any legitimate value containing a single quote (e.g. a comment with an apostrophe) breaks these queries.
Proposed fix
Replace .arg() query construction with prepared statements:
QSqlQuery query;
query.prepare("SELECT id FROM log WHERE call = :call");
query.bindValue(":call", _call);
query.exec();
This can be done module-by-module, starting with the queries reachable from ADIF import and online-service responses. Side benefit: bound prepared statements avoid re-parsing query text on every call, which also helps performance in the hot paths identified in the performance analysis.
Problem
Dozens of SQL queries are built with
QString::arg()string concatenation instead of prepared statements with bound values. Several of these include data that crosses a trust boundary:Data from imported ADIF files and online services (most important):
src/dataproxy_sqlite.cpp:919,:979,:3461,:3465src/dataproxy_sqlite.cpp:3718-3719src/dataproxy_sqlite.cpp:3651User-editable log metadata:
stationcall,operators,commentconcatenated into UPDATE/INSERT/SELECT on thelogstable:src/dataproxy_sqlite.cpp:6941-6942,:6965,:6990-6991Other widespread instances throughout
src/dataproxy_sqlite.cpp(log number, satellite names, DXCC/continent lookups, e.g.:457,:729,:5088,:7089...) andsrc/searchwindow.cpp:650.Impact
A crafted callsign or comment field — e.g.
X' OR '1'='1— in a shared ADIF file (operators routinely exchange these) or in a manipulated online-service response can break queries or corrupt/delete log data. Realistic impact is data corruption / denial of service of the user's log rather than code execution, but the inputs are genuinely untrusted, and a corrupted logbook is a serious loss for the user.There is also a correctness bug today: any legitimate value containing a single quote (e.g. a comment with an apostrophe) breaks these queries.
Proposed fix
Replace
.arg()query construction with prepared statements:This can be done module-by-module, starting with the queries reachable from ADIF import and online-service responses. Side benefit: bound prepared statements avoid re-parsing query text on every call, which also helps performance in the hot paths identified in the performance analysis.