Permalink
Browse files

Fixing security vulnerability with Qt 4.8.5+ and PostgreSQL.

Properly detects whether Qt performs slash escaping in SQL queries or
not, and then configures PostgreSQL accordingly. This bug was a
introduced due to a bugfix in Qt 4.8.5 disables slash escaping when
binding queries: https://bugreports.qt-project.org/browse/QTBUG-30076
Thanks to brot and Tucos.

[Fixes #1244]
1 parent 61bf666 commit aa1008be162cb27da938cce93ba533f54d228869 @egs-me egs-me committed Oct 10, 2013
Showing with 49 additions and 10 deletions.
  1. +6 −3 src/core/abstractsqlstorage.cpp
  2. +1 −1 src/core/abstractsqlstorage.h
  3. +41 −5 src/core/postgresqlstorage.cpp
  4. +1 −1 src/core/postgresqlstorage.h
@@ -91,11 +91,14 @@ void AbstractSqlStorage::addConnectionToPool()
}
if (!db.open()) {
- qWarning() << "Unable to open database" << displayName() << "for thread" << QThread::currentThread();
- qWarning() << "-" << db.lastError().text();
+ quWarning() << "Unable to open database" << displayName() << "for thread" << QThread::currentThread();
+ quWarning() << "-" << db.lastError().text();
}
else {
- initDbSession(db);
+ if (!initDbSession(db)) {
+ quWarning() << "Unable to initialize database" << displayName() << "for thread" << QThread::currentThread();
+ db.close();
+ }
}
}
@@ -80,7 +80,7 @@ public slots:
* When reimplementing this method, don't use logDB() inside this function as
* this would cause as we're just about to initialize that DB connection.
*/
- inline virtual void initDbSession(QSqlDatabase & /* db */) {}
+ inline virtual bool initDbSession(QSqlDatabase & /* db */) { return true; }
private slots:
void connectionDestroyed();
@@ -96,11 +96,47 @@ QVariantMap PostgreSqlStorage::setupDefaults() const
}
-void PostgreSqlStorage::initDbSession(QSqlDatabase &db)
-{
- // this blows... but unfortunately Qt's PG driver forces us to this...
- db.exec("set standard_conforming_strings = off");
- db.exec("set escape_string_warning = off");
+bool PostgreSqlStorage::initDbSession(QSqlDatabase &db)
+{
+ // check whether the Qt driver performs string escaping or not.
+ // i.e. test if it doubles slashes.
+ QSqlField testField;
+ testField.setType(QVariant::String);
+ testField.setValue("\\");
+ QString formattedString = db.driver()->formatValue(testField);
+ switch(formattedString.count('\\')) {
+ case 2:
+ // yes it does... and we cannot do anything to change the behavior of Qt.
+ // If this is a legacy DB (Postgres < 8.2), then everything is already ok,
+ // as this is the expected behavior.
+ // If it is a newer version, switch to legacy mode.
+
+ quWarning() << "Switching Postgres to legacy mode. (set standard conforming strings to off)";
+ // If the following calls fail, it is a legacy DB anyways, so it doesn't matter
+ // and no need to check the outcome.
+ db.exec("set standard_conforming_strings = off");
+ db.exec("set escape_string_warning = off");
+ break;
+ case 1:
+ // ok, so Qt does not escape...
+ // That means we have to ensure that postgres uses standard conforming strings...
+ {
+ QSqlQuery query = db.exec("set standard_conforming_strings = on");
+ if (query.lastError().isValid()) {
+ // We cannot enable standard conforming strings...
+ // since Quassel does no escaping by itself, this would yield a major vulnerability.
+ quError() << "Failed to enable standard_conforming_strings for the Postgres db!";
+ return false;
+ }
+ }
+ break;
+ default:
+ // The slash got replaced with 0 or more than 2 slashes! o_O
+ quError() << "Your version of Qt does something _VERY_ strange to slashes in QSqlQueries! You should consult your trusted doctor!";
+ return false;
+ break;
+ }
+ return true;
}
@@ -103,7 +103,7 @@ public slots:
virtual QList<Message> requestAllMsgs(UserId user, MsgId first = -1, MsgId last = -1, int limit = -1);
protected:
- virtual void initDbSession(QSqlDatabase &db);
+ virtual bool initDbSession(QSqlDatabase &db);
virtual void setConnectionProperties(const QVariantMap &properties);
inline virtual QString driverName() { return "QPSQL"; }
inline virtual QString hostName() { return _hostName; }

0 comments on commit aa1008b

Please sign in to comment.