From 7ae8ae989e2cba1727ca1aae9e24a542c1e8f0fd Mon Sep 17 00:00:00 2001 From: Timo Pollmeier Date: Thu, 22 Aug 2019 16:01:08 +0200 Subject: [PATCH 1/4] Do not abort on user name constraint violation If the UNIQUE constraint for user names is violated, gvmd will send a GMP response instead of aborting and closing the connection. --- src/manage_sql.c | 176 ++++++++++++++++++++++++++++------------------- src/sql.c | 23 +++++-- src/sql_pg.c | 15 +++- 3 files changed, 135 insertions(+), 79 deletions(-) diff --git a/src/manage_sql.c b/src/manage_sql.c index 2d07cb208..86f5b8fde 100644 --- a/src/manage_sql.c +++ b/src/manage_sql.c @@ -4751,6 +4751,7 @@ copy_resource_lock (const char *type, const char *name, const char *comment, user_t owner; resource_t resource; resource_t new; + int ret = -1; if (resource_id == NULL) return -1; @@ -4818,60 +4819,79 @@ copy_resource_lock (const char *type, const char *name, const char *comment, { gchar *quoted_comment; quoted_comment = sql_nquote (comment, strlen (comment)); - sql ("INSERT INTO %ss" - " (uuid, owner, name, comment, creation_time, modification_time%s%s)" - " SELECT make_uuid ()," - " (SELECT id FROM users where users.uuid = '%s')," - " %s%s%s, '%s', m_now (), m_now ()%s%s" - " FROM %ss WHERE uuid = '%s';", - type, - columns ? ", " : "", - columns ? columns : "", - current_credentials.uuid, - quoted_name ? "'" : "", - quoted_name ? quoted_name : uniquify, - quoted_name ? "'" : "", - quoted_comment, - columns ? ", " : "", - columns ? columns : "", - type, - quoted_uuid); + ret = sql_error ("INSERT INTO %ss" + " (uuid, owner, name, comment," + " creation_time, modification_time%s%s)" + " SELECT make_uuid ()," + " (SELECT id FROM users" + " where users.uuid = '%s')," + " %s%s%s, '%s', m_now (), m_now ()%s%s" + " FROM %ss WHERE uuid = '%s';", + type, + columns ? ", " : "", + columns ? columns : "", + current_credentials.uuid, + quoted_name ? "'" : "", + quoted_name ? quoted_name : uniquify, + quoted_name ? "'" : "", + quoted_comment, + columns ? ", " : "", + columns ? columns : "", + type, + quoted_uuid); g_free (quoted_comment); } else if (named) - sql ("INSERT INTO %ss" - " (uuid, owner, name%s, creation_time, modification_time%s%s)" - " SELECT make_uuid ()," - " (SELECT id FROM users where users.uuid = '%s')," - " %s%s%s%s, m_now (), m_now ()%s%s" - " FROM %ss WHERE uuid = '%s';", - type, - type_has_comment (type) ? ", comment" : "", - columns ? ", " : "", - columns ? columns : "", - current_credentials.uuid, - quoted_name ? "'" : "", - quoted_name ? quoted_name : uniquify, - quoted_name ? "'" : "", - type_has_comment (type) ? ", comment" : "", - columns ? ", " : "", - columns ? columns : "", - type, - quoted_uuid); - else - sql ("INSERT INTO %ss" - " (uuid, owner, creation_time, modification_time%s%s)" - " SELECT make_uuid (), (SELECT id FROM users where users.uuid = '%s')," - " m_now (), m_now ()%s%s" - " FROM %ss WHERE uuid = '%s';", - type, - columns ? ", " : "", - columns ? columns : "", - current_credentials.uuid, - columns ? ", " : "", - columns ? columns : "", - type, - quoted_uuid); + ret = sql_error ("INSERT INTO %ss" + " (uuid, owner, name%s," + " creation_time, modification_time%s%s)" + " SELECT make_uuid ()," + " (SELECT id FROM users where users.uuid = '%s')," + " %s%s%s%s, m_now (), m_now ()%s%s" + " FROM %ss WHERE uuid = '%s';", + type, + type_has_comment (type) ? ", comment" : "", + columns ? ", " : "", + columns ? columns : "", + current_credentials.uuid, + quoted_name ? "'" : "", + quoted_name ? quoted_name : uniquify, + quoted_name ? "'" : "", + type_has_comment (type) ? ", comment" : "", + columns ? ", " : "", + columns ? columns : "", + type, + quoted_uuid); + else + ret = sql_error ("INSERT INTO %ss" + " (uuid, owner, creation_time, modification_time%s%s)" + " SELECT make_uuid ()," + " (SELECT id FROM users where users.uuid = '%s')," + " m_now (), m_now ()%s%s" + " FROM %ss WHERE uuid = '%s';", + type, + columns ? ", " : "", + columns ? columns : "", + current_credentials.uuid, + columns ? ", " : "", + columns ? columns : "", + type, + quoted_uuid); + + if (ret == 3) + { + g_free (quoted_uuid); + g_free (quoted_name); + g_free (uniquify); + return 1; + } + else if (ret) + { + g_free (quoted_uuid); + g_free (quoted_name); + g_free (uniquify); + return -1; + } new = sql_last_insert_id (); @@ -59638,7 +59658,7 @@ create_user (const gchar * name, const gchar * password, const gchar *comment, char *errstr, *uuid; gchar *quoted_hosts, *quoted_ifaces, *quoted_method, *quoted_name, *hash; gchar *quoted_comment, *clean, *generated; - int index, max; + int index, max, ret; user_t user; GArray *cache_users; @@ -59735,24 +59755,27 @@ create_user (const gchar * name, const gchar * password, const gchar *comment, quoted_method = sql_quote (allowed_methods ? g_ptr_array_index (allowed_methods, 0) : "file"); - sql ("INSERT INTO users" - " (uuid, owner, name, password, comment, hosts, hosts_allow," - " ifaces, ifaces_allow, method, creation_time, modification_time)" - " VALUES" - " (make_uuid ()," - " (SELECT id FROM users WHERE uuid = '%s')," - " '%s', '%s', '%s', '%s', %i," - " '%s', %i, '%s', m_now (), m_now ());", - current_credentials.uuid, - quoted_name, - hash, - quoted_comment, - quoted_hosts, - hosts_allow, - quoted_ifaces, - ifaces_allow, - quoted_method); - user = sql_last_insert_id (); + + ret + = sql_error ("INSERT INTO users" + " (uuid, owner, name, password, comment, hosts, hosts_allow," + " ifaces, ifaces_allow, method, creation_time," + " modification_time)" + " VALUES" + " (make_uuid ()," + " (SELECT id FROM users WHERE uuid = '%s')," + " '%s', '%s', '%s', '%s', %i," + " '%s', %i, '%s', m_now ()," + " m_now ());", + current_credentials.uuid, + quoted_name, + hash, + quoted_comment, + quoted_hosts, + hosts_allow, + quoted_ifaces, + ifaces_allow, + quoted_method); g_free (generated); g_free (hash); g_free (quoted_comment); @@ -59761,6 +59784,19 @@ create_user (const gchar * name, const gchar * password, const gchar *comment, g_free (quoted_method); g_free (quoted_name); + if (ret == 3) + { + sql_rollback (); + return -2; + } + else if (ret) + { + sql_rollback (); + return -1; + } + + user = sql_last_insert_id (); + /* Add the user to any given groups. */ index = 0; diff --git a/src/sql.c b/src/sql.c index 5c6845ded..a6e8fd5b5 100644 --- a/src/sql.c +++ b/src/sql.c @@ -154,7 +154,9 @@ sql_insert (const char *string) * @param[in] sql Format string for SQL statement. * @param[in] args Arguments for format string. * - * @return 0 success, 1 gave up (even when retry given), -1 error. + * @return 0 success, 1 gave up (even when retry given), + * 2 reserved (lock unavailable), 3 unique constraint violation, + * -1 error. */ int sqlv (int retry, char* sql, va_list args) @@ -188,6 +190,8 @@ sqlv (int retry, char* sql, va_list args) return 1; if (ret == -3) return -1; + if (ret == -4) + return 3; assert (ret == -1 || ret == 0); return ret; } @@ -210,11 +214,11 @@ sql (char* sql, ...) va_start (args, sql); ret = sqlv (1, sql, args); va_end (args); - if (ret == -1) - abort (); if (ret == 1) /* Gave up with statement reset. */ continue; + else if (ret) + abort(); break; } } @@ -227,7 +231,8 @@ sql (char* sql, ...) * @param[in] sql Format string for SQL statement. * @param[in] ... Arguments for format string. * - * @return 0 success, -1 error. + * @return 0 success, 2 reserved (lock unavailable), + * 3 unique constraint violation, -1 error. */ int sql_error (char* sql, ...) @@ -243,6 +248,8 @@ sql_error (char* sql, ...) if (ret == 1) /* Gave up with statement reset. */ continue; + if (ret == -4) + return 3; break; } @@ -255,7 +262,9 @@ sql_error (char* sql, ...) * @param[in] sql Format string for SQL statement. * @param[in] ... Arguments for format string. * - * @return 0 success, 1 gave up, -1 error. + * @return 0 success, 1 gave up, + * 2 reserved (lock unavailable), 3 unique constraint violation, + * -1 error. */ int sql_giveup (char* sql, ...) @@ -305,7 +314,7 @@ sql_x_internal (int log, char* sql, va_list args, sql_stmt_t** stmt_return) /* Run statement. */ ret = sql_exec_internal (1, *stmt_return); - if (ret == -1) + if (ret == -1 || ret == -4) { if (log_errors) g_warning ("%s: sql_exec_internal failed", __FUNCTION__); @@ -691,7 +700,7 @@ next (iterator_t* iterator) iterator->done = TRUE; return FALSE; } - if (ret == -1) + if (ret == -1 || ret == -4) { if (log_errors) g_warning ("%s: sql_exec_internal failed", __FUNCTION__); diff --git a/src/sql_pg.c b/src/sql_pg.c index f4a60518e..06b2e1d61 100644 --- a/src/sql_pg.c +++ b/src/sql_pg.c @@ -485,7 +485,7 @@ sql_prepare_internal (int retry, int log, const char* sql, va_list args, * @param[in] stmt Statement. * * @return 0 complete, 1 row available in results, -1 error, -2 gave up, - * -3 lock unavailable. + * -3 lock unavailable, -4 unique constraint violation. */ int sql_exec_internal (int retry, sql_stmt_t *stmt) @@ -513,18 +513,29 @@ sql_exec_internal (int retry, sql_stmt_t *stmt) sqlstate = PQresultErrorField (result, PG_DIAG_SQLSTATE); g_debug ("%s: sqlstate: %s", __FUNCTION__, sqlstate); - if (sqlstate && (strcmp (sqlstate, "57014") == 0)) /* query_canceled */ + if (sqlstate && (strcmp (sqlstate, "57014") == 0)) { + /* query_canceled */ log_errors = 0; g_debug ("%s: canceled SQL: %s", __FUNCTION__, stmt->sql); } else if (sqlstate && (strcmp (sqlstate, "55P03") == 0)) { + /* lock_not_available */ g_debug ("%s: lock unavailable: %s", __FUNCTION__, PQresultErrorMessage(result)); return -3; } + else if (sqlstate && (strcmp (sqlstate, "23505") == 0)) + { + /* unique_violation */ + g_warning ("%s: constraint violation: %s", + __FUNCTION__, + PQresultErrorMessage (result)); + g_warning ("%s: SQL: %s", __FUNCTION__, stmt->sql); + return -4; + } if (log_errors) { From 8ccaf3f20dec8ebfe6e3df057634b16078ad23b4 Mon Sep 17 00:00:00 2001 From: Timo Pollmeier Date: Tue, 27 Aug 2019 09:18:27 +0200 Subject: [PATCH 2/4] Uniquify user names before adding constraint Without this the migration would not work if there are already duplicate user names. --- src/manage_migrators.c | 7 +++++++ src/manage_pg.c | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/manage_migrators.c b/src/manage_migrators.c index 69e7d2c35..4f82f55fc 100644 --- a/src/manage_migrators.c +++ b/src/manage_migrators.c @@ -1304,6 +1304,13 @@ migrate_217_to_218 () /* Update the database. */ + /* Ensure all user names are unique */ + + sql ("UPDATE users" + " SET name = uniquify('user', name, NULL, '')" + " WHERE id != (SELECT min(id) FROM users AS inner_users" + " WHERE users.name = inner_users.name);"); + /* Add an UNIQUE constraint to the name column of users */ sql ("ALTER TABLE users ADD UNIQUE (name);"); diff --git a/src/manage_pg.c b/src/manage_pg.c index 0294cc5f8..467567139 100644 --- a/src/manage_pg.c +++ b/src/manage_pg.c @@ -1039,7 +1039,7 @@ manage_create_sql_functions () " LOOP" " EXECUTE 'SELECT count (*) = 0 FROM ' || type || 's" " WHERE name = $1" - " AND ((owner IS NULL) OR (owner = $2))'" + " AND (($2 IS NULL) OR (owner IS NULL) OR (owner = $2))'" " INTO unique_candidate" " USING candidate, owner;" " EXIT WHEN unique_candidate;" From d23048abb24cc6a7a75c2438bd5f3d78297431d0 Mon Sep 17 00:00:00 2001 From: Timo Pollmeier Date: Tue, 27 Aug 2019 09:43:04 +0200 Subject: [PATCH 3/4] Make uniquify when copying users global --- src/manage_sql.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/manage_sql.c b/src/manage_sql.c index e22fd3981..cb6df738a 100644 --- a/src/manage_sql.c +++ b/src/manage_sql.c @@ -4461,6 +4461,22 @@ type_named (const char *type) && strcasecmp (type, "override"); } +/** + * @brief Check whether a type must have globally unique names. + * + * @param[in] type Type of resource. + * + * @return 1 yes, 0 no. + */ +static int +type_globally_unique (const char *type) +{ + if (strcasecmp (type, "user") == 0) + return 1; + else + return 0; +} + /** * @brief Check whether a type has a comment. * @@ -4761,7 +4777,7 @@ copy_resource_lock (const char *type, const char *name, const char *comment, resource_t *old_resource) { gchar *quoted_name, *quoted_uuid, *uniquify, *command; - int named; + int named, globally_unique; user_t owner; resource_t resource; resource_t new; @@ -4806,6 +4822,7 @@ copy_resource_lock (const char *type, const char *name, const char *comment, } named = type_named (type); + globally_unique = type_globally_unique (type); if (named && name && *name && resource_with_name_exists (name, type, 0)) return 1; @@ -4822,11 +4839,14 @@ copy_resource_lock (const char *type, const char *name, const char *comment, /* Copy the existing resource. */ - if (make_name_unique) - uniquify = g_strdup_printf ("uniquify ('%s', name, %llu, '%cClone')", + if (globally_unique && make_name_unique) + uniquify = g_strdup_printf ("uniquify ('%s', name, NULL, '%cClone')", type, - owner, strcmp (type, "user") ? ' ' : '_'); + else if (make_name_unique) + uniquify = g_strdup_printf ("uniquify ('%s', name, %llu, ' Clone')", + type, + owner); else uniquify = g_strdup ("name"); if (named && comment && strlen (comment)) From e5d6cddebf62d14477faa007667a13cf184f0fac Mon Sep 17 00:00:00 2001 From: Timo Pollmeier Date: Tue, 27 Aug 2019 09:52:00 +0200 Subject: [PATCH 4/4] Amend CHANGELOG for changes to unique user names --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 624b0b195..45d6c25d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Check and create default permissions individually [#671](https://github.com/greenbone/gvmd/pull/671) - Add -f arg to sendmail call in email alert [#676](https://github.com/greenbone/gvmd/pull/676) [#678](https://github.com/greenbone/gvmd/pull/678) - Change get_tickets to use the status text for filtering. [#697](https://github.com/greenbone/gvmd/pull/697) -- Made checks to prevent duplicate user names stricter. [#708](https://github.com/greenbone/gvmd/pull/708) +- Made checks to prevent duplicate user names stricter. [#708](https://github.com/greenbone/gvmd/pull/708) [#722](https://github.com/greenbone/gvmd/pull/722) - Send delete command to ospd after stopping the task. [#710](https://github.com/greenbone/gvmd/pull/710) - Check whether hosts are alive and have results when adding them in slave scans. [#717](https://github.com/greenbone/gvmd/pull/717)