From 2f742a809a4d8915a4e692b8eb920f18fd4f0258 Mon Sep 17 00:00:00 2001 From: Richard Hughes Date: Fri, 17 Jul 2009 11:09:37 +0100 Subject: [PATCH] Make saving the proxy data secure, and add some notes to the security document --- docs/security.txt | 19 +++++++++++++ docs/setting-the-proxy.txt | 26 ------------------ src/pk-dbus.c | 2 +- src/pk-transaction-db.c | 55 +++++++++++++++++++++++++++++--------- 4 files changed, 62 insertions(+), 40 deletions(-) diff --git a/docs/security.txt b/docs/security.txt index f5a3ef824..4da7c73de 100644 --- a/docs/security.txt +++ b/docs/security.txt @@ -91,3 +91,22 @@ Possible attack vectors: (MaximumPackagesToProcess) for each method, and also limiting the total length of the data. + * The HTTP and FTP proxies that are sent by the session may contain embedded + usernames and passwords. Whilst these are sent in plain text over the system + D-Bus (FIXME?), the database is not be readable by users (mode 0640). + + * The matching of UID and session to proxies assumes that the user cannot + modify the login UID, and cannot spoof a session in ConsoleKit. If a user were + able to change the apparent UID, then this would allow them to use proxy + settings set from another user. + This is manifested when a graphical application is run using sudo (which you + should never do, but I digress) and different proxy settings may be used from + the user settings. It also allows applications run using sudo to use the proxy + specified by root, which may be different. + + * We save the UID and session to a database to get the proxy state for each + transaction. If the user is able to create a large number of different sessions + then this will add many entries to the database. Some rate-limiting could be + added to ConsoleKit or PackageKit to solve this, but has not been done at this + time. + diff --git a/docs/setting-the-proxy.txt b/docs/setting-the-proxy.txt index 4dece3587..74065456e 100644 --- a/docs/setting-the-proxy.txt +++ b/docs/setting-the-proxy.txt @@ -27,29 +27,3 @@ proxy settings and this is passed to the backend. This also allows the daemon to have to correct proxy settings if the daemon times out (as it is designed to do) and is restarted by a client application using DBus system activation. -Implementation considerations; - -The PID and session should be be cached in PkDbus as these will be used on each transaction. - -Security considerations: -The HTTP and FTP proxies that are sent by the session may contain embedded -usernames and passwords. Whilst these are sent in plain text over the system -DBus (TODO), the database should not be readable by a user as this could be used -to discover proxy server passwords. - -The matching of UID and session to proxies assumes that the user cannot modify -the login UID, and cannot spoof a session in ConsoleKit. If a user were able to -change the apparent UID, then this would allow them to use proxy settings set -from another user. This is manifested when a graphical application is run using -sudo (which you should never do, but I digress) and different proxy settings may -be used from the user settings. It also allows applications run using sudo to -use the proxy specified by root, which may be different. - -We save the UID and session to a database to get the proxy state for each -transaction. If the user is able to create a large number of different sessions -then this will add many entries to the database. Some rate-limiting could be -added to ConsoleKit or PackageKit to solve this, but has not been done at this -time. - -TODO: need to check for injected SQL in the proxy data before we add freeform text. - diff --git a/src/pk-dbus.c b/src/pk-dbus.c index 0605b90d7..28ba20c77 100644 --- a/src/pk-dbus.c +++ b/src/pk-dbus.c @@ -95,7 +95,7 @@ pk_dbus_get_pid (PkDbus *dbus, const gchar *sender) g_return_val_if_fail (dbus->priv->proxy_pid != NULL, G_MAXUINT); g_return_val_if_fail (sender != NULL, G_MAXUINT); - /* get pid from DBus (quite slow) */ + /* get pid from DBus (quite slow) - TODO: cache this */ ret = dbus_g_proxy_call (dbus->priv->proxy_pid, "GetConnectionUnixProcessID", &error, G_TYPE_STRING, sender, diff --git a/src/pk-transaction-db.c b/src/pk-transaction-db.c index 1f999976a..3d5cbe728 100644 --- a/src/pk-transaction-db.c +++ b/src/pk-transaction-db.c @@ -683,13 +683,12 @@ gboolean pk_transaction_db_set_proxy (PkTransactionDb *tdb, guint uid, const gchar *session, const gchar *proxy_http, const gchar *proxy_ftp) { - gchar *error_msg = NULL; - gchar *statement = NULL; gchar *timespec = NULL; gchar *proxy_http_tmp = NULL; gchar *proxy_ftp_tmp = NULL; gboolean ret = FALSE; gint rc; + sqlite3_stmt *statement = NULL; g_return_val_if_fail (PK_IS_TRANSACTION_DB (tdb), FALSE); g_return_val_if_fail (uid != G_MAXUINT, FALSE); @@ -703,13 +702,27 @@ pk_transaction_db_set_proxy (PkTransactionDb *tdb, guint uid, const gchar *sessi /* any data */ if (proxy_http_tmp != NULL || proxy_ftp_tmp != NULL) { - statement = g_strdup_printf ("UPDATE proxy SET proxy_http = '%s', proxy_ftp = '%s' WHERE uid = '%i' AND session = '%s'", - proxy_http, proxy_ftp, uid, session); egg_debug ("updated proxy for uid:%i and session:%s", uid, session); - rc = sqlite3_exec (tdb->priv->db, statement, NULL, NULL, &error_msg); + + /* prepare statement */ + rc = sqlite3_prepare_v2 (tdb->priv->db, + "UPDATE proxy SET proxy_http = ?, proxy_ftp = ? WHERE uid = ? AND session = ?", + -1, &statement, NULL); if (rc != SQLITE_OK) { - egg_warning ("SQL error: %s", error_msg); - sqlite3_free (error_msg); + egg_warning ("failed to prepare statement: %s", sqlite3_errmsg (tdb->priv->db)); + goto out; + } + + /* bind data, so that the freeform proxy text cannot be used to inject SQL */ + sqlite3_bind_text (statement, 1, proxy_http, -1, SQLITE_STATIC); + sqlite3_bind_text (statement, 2, proxy_ftp, -1, SQLITE_STATIC); + sqlite3_bind_int (statement, 3, uid); + sqlite3_bind_text (statement, 4, session, -1, SQLITE_STATIC); + + /* execute statement */ + rc = sqlite3_step (statement); + if (rc != SQLITE_DONE) { + egg_warning ("failed to execute statement: %s", sqlite3_errmsg (tdb->priv->db)); goto out; } goto out; @@ -717,20 +730,36 @@ pk_transaction_db_set_proxy (PkTransactionDb *tdb, guint uid, const gchar *sessi /* insert new entry */ timespec = pk_iso8601_present (); - statement = g_strdup_printf ("INSERT INTO proxy (created, uid, session, proxy_http, proxy_ftp) VALUES ('%s', '%i', '%s', '%s', '%s')", - timespec, uid, session, proxy_http, proxy_ftp); egg_debug ("set proxy for uid:%i and session:%s", uid, session); - rc = sqlite3_exec (tdb->priv->db, statement, NULL, NULL, &error_msg); + + /* prepare statement */ + rc = sqlite3_prepare_v2 (tdb->priv->db, + "INSERT INTO proxy (created, uid, session, proxy_http, proxy_ftp) VALUES (?, ?, ?, ?, ?)", + -1, &statement, NULL); if (rc != SQLITE_OK) { - egg_warning ("SQL error: %s", error_msg); - sqlite3_free (error_msg); + egg_warning ("failed to prepare statement: %s", sqlite3_errmsg (tdb->priv->db)); + goto out; + } + + /* bind data, so that the freeform proxy text cannot be used to inject SQL */ + sqlite3_bind_text (statement, 1, timespec, -1, SQLITE_STATIC); + sqlite3_bind_int (statement, 2, uid); + sqlite3_bind_text (statement, 3, session, -1, SQLITE_STATIC); + sqlite3_bind_text (statement, 4, proxy_http, -1, SQLITE_STATIC); + sqlite3_bind_text (statement, 5, proxy_ftp, -1, SQLITE_STATIC); + + /* execute statement */ + rc = sqlite3_step (statement); + if (rc != SQLITE_DONE) { + egg_warning ("failed to execute statement: %s", sqlite3_errmsg (tdb->priv->db)); goto out; } ret = TRUE; out: + if (statement != NULL) + sqlite3_finalize (statement); g_free (timespec); - g_free (statement); g_free (proxy_http_tmp); g_free (proxy_ftp_tmp); return ret;