From 2e6fee186e3fe4b54f123c304def6882ba375078 Mon Sep 17 00:00:00 2001 From: Richard Hughes Date: Tue, 11 May 2021 21:10:14 +0100 Subject: [PATCH] trivial: Fix several potential leaks when parsing remotes This fixes several issues when calling fwupd_remote_load_from_filename() multiple times on the same FwupdRemote. --- data/remotes.d/lvfs-testing.conf | 4 +- data/tests/disabled.conf | 4 + libfwupd/fwupd-remote.c | 257 +++++++++++++++++-------------- libfwupd/fwupd-self-test.c | 25 +++ 4 files changed, 170 insertions(+), 120 deletions(-) create mode 100644 data/tests/disabled.conf diff --git a/data/remotes.d/lvfs-testing.conf b/data/remotes.d/lvfs-testing.conf index 740a793e8..42575498f 100644 --- a/data/remotes.d/lvfs-testing.conf +++ b/data/remotes.d/lvfs-testing.conf @@ -5,8 +5,8 @@ Enabled=false Title=Linux Vendor Firmware Service (testing) MetadataURI=https://cdn.fwupd.org/downloads/firmware-testing.xml.gz ReportURI=https://fwupd.org/lvfs/firmware/report -Username= -Password= +#Username= +#Password= OrderBefore=lvfs,fwupd AutomaticReports=false ApprovalRequired=false diff --git a/data/tests/disabled.conf b/data/tests/disabled.conf new file mode 100644 index 000000000..7fd7d46d0 --- /dev/null +++ b/data/tests/disabled.conf @@ -0,0 +1,4 @@ +[fwupd Remote] +Enabled=false +Keyring=none +Password= diff --git a/libfwupd/fwupd-remote.c b/libfwupd/fwupd-remote.c index 76aa03f4e..ae7a572c3 100644 --- a/libfwupd/fwupd-remote.c +++ b/libfwupd/fwupd-remote.c @@ -79,8 +79,12 @@ static void fwupd_remote_set_username (FwupdRemote *self, const gchar *username) { FwupdRemotePrivate *priv = GET_PRIVATE (self); - if (username != NULL && username[0] == '\0') - username = NULL; + + /* not changed */ + if (g_strcmp0 (priv->username, username) == 0) + return; + + g_free (priv->username); priv->username = g_strdup (username); } @@ -141,8 +145,6 @@ fwupd_remote_set_password (FwupdRemote *self, const gchar *password) if (g_strcmp0 (priv->password, password) == 0) return; - if (password != NULL && password[0] == '\0') - password = NULL; g_free (priv->password); priv->password = g_strdup (password); } @@ -312,6 +314,12 @@ static void fwupd_remote_set_firmware_base_uri (FwupdRemote *self, const gchar *firmware_base_uri) { FwupdRemotePrivate *priv = GET_PRIVATE (self); + + /* not changed */ + if (g_strcmp0 (priv->firmware_base_uri, firmware_base_uri) == 0) + return; + + g_free (priv->firmware_base_uri); priv->firmware_base_uri = g_strdup (firmware_base_uri); } @@ -319,6 +327,12 @@ static void fwupd_remote_set_report_uri (FwupdRemote *self, const gchar *report_uri) { FwupdRemotePrivate *priv = GET_PRIVATE (self); + + /* not changed */ + if (g_strcmp0 (priv->report_uri, report_uri) == 0) + return; + + g_free (priv->report_uri); priv->report_uri = g_strdup (report_uri); } @@ -326,6 +340,12 @@ static void fwupd_remote_set_security_report_uri (FwupdRemote *self, const gchar *security_report_uri) { FwupdRemotePrivate *priv = GET_PRIVATE (self); + + /* not changed */ + if (g_strcmp0 (priv->security_report_uri, security_report_uri) == 0) + return; + + g_free (priv->security_report_uri); priv->security_report_uri = g_strdup (security_report_uri); } @@ -396,6 +416,24 @@ fwupd_remote_set_filename_cache (FwupdRemote *self, const gchar *filename) } } +static void +fwupd_remote_set_order_before (FwupdRemote *self, const gchar *order_before) +{ + FwupdRemotePrivate *priv = GET_PRIVATE (self); + g_clear_pointer (&priv->order_before, g_strfreev); + if (order_before != NULL) + priv->order_before = g_strsplit_set (order_before, ",:;", -1); +} + +static void +fwupd_remote_set_order_after (FwupdRemote *self, const gchar *order_after) +{ + FwupdRemotePrivate *priv = GET_PRIVATE (self); + g_clear_pointer (&priv->order_after, g_strfreev); + if (order_after != NULL) + priv->order_after = g_strsplit_set (order_after, ",:;", -1); +} + /** * fwupd_remote_load_from_filename: * @self: a #FwupdRemote @@ -419,14 +457,7 @@ fwupd_remote_load_from_filename (FwupdRemote *self, { FwupdRemotePrivate *priv = GET_PRIVATE (self); const gchar *group = "fwupd Remote"; - g_autofree gchar *firmware_base_uri = NULL; g_autofree gchar *id = NULL; - g_autofree gchar *keyring_kind = NULL; - g_autofree gchar *metadata_uri = NULL; - g_autofree gchar *order_after = NULL; - g_autofree gchar *order_before = NULL; - g_autofree gchar *report_uri = NULL; - g_autofree gchar *security_report_uri = NULL; g_autoptr(GKeyFile) kf = NULL; g_return_val_if_fail (FWUPD_IS_REMOTE (self), FALSE); @@ -443,100 +474,101 @@ fwupd_remote_load_from_filename (FwupdRemote *self, if (!g_key_file_load_from_file (kf, filename, G_KEY_FILE_NONE, error)) return FALSE; - /* get verification type, falling back to GPG */ - keyring_kind = g_key_file_get_string (kf, group, "Keyring", NULL); - if (keyring_kind == NULL) { - priv->keyring_kind = FWUPD_KEYRING_KIND_JCAT; - } else { - priv->keyring_kind = fwupd_keyring_kind_from_string (keyring_kind); + /* optional verification type */ + if (g_key_file_has_key (kf, group, "Keyring", NULL)) { + g_autofree gchar *tmp = g_key_file_get_string (kf, group, "Keyring", NULL); + priv->keyring_kind = fwupd_keyring_kind_from_string (tmp); if (priv->keyring_kind == FWUPD_KEYRING_KIND_UNKNOWN) { g_set_error (error, FWUPD_ERROR, FWUPD_ERROR_INVALID_FILE, - "Failed to parse type '%s'", - keyring_kind); + "keyring kind '%s' unknown", + tmp); return FALSE; } } - /* all remotes need a URI, even if it's file:// to the cache */ - metadata_uri = g_key_file_get_string (kf, group, "MetadataURI", error); - if (metadata_uri == NULL) - return FALSE; - if (g_str_has_prefix (metadata_uri, "file://")) { - const gchar *filename_cache = metadata_uri; - if (g_str_has_prefix (filename_cache, "file://")) - filename_cache += 7; - fwupd_remote_set_filename_cache (self, filename_cache); - if (g_file_test (filename_cache, G_FILE_TEST_IS_DIR)) - priv->kind = FWUPD_REMOTE_KIND_DIRECTORY; - else - priv->kind = FWUPD_REMOTE_KIND_LOCAL; - } else if (g_str_has_prefix (metadata_uri, "http://") || - g_str_has_prefix (metadata_uri, "https://") || - g_str_has_prefix (metadata_uri, "ipfs://") || - g_str_has_prefix (metadata_uri, "ipns://")) { - priv->kind = FWUPD_REMOTE_KIND_DOWNLOAD; - } else { - g_set_error (error, - FWUPD_ERROR, - FWUPD_ERROR_INVALID_FILE, - "Failed to parse MetadataURI type '%s'", - metadata_uri); - return FALSE; + /* the first remote sets the URI, even if it's file:// to the cache */ + if (g_key_file_has_key (kf, group, "MetadataURI", NULL)) { + g_autofree gchar *tmp = g_key_file_get_string (kf, group, "MetadataURI", NULL); + if (g_str_has_prefix (tmp, "file://")) { + const gchar *filename_cache = tmp; + if (g_str_has_prefix (filename_cache, "file://")) + filename_cache += 7; + fwupd_remote_set_filename_cache (self, filename_cache); + if (g_file_test (filename_cache, G_FILE_TEST_IS_DIR)) + priv->kind = FWUPD_REMOTE_KIND_DIRECTORY; + else + priv->kind = FWUPD_REMOTE_KIND_LOCAL; + } else if (g_str_has_prefix (tmp, "http://") || + g_str_has_prefix (tmp, "https://") || + g_str_has_prefix (tmp, "ipfs://") || + g_str_has_prefix (tmp, "ipns://")) { + priv->kind = FWUPD_REMOTE_KIND_DOWNLOAD; + fwupd_remote_set_metadata_uri (self, tmp); + } } - /* extract data */ - priv->enabled = g_key_file_get_boolean (kf, group, "Enabled", NULL); - priv->approval_required = g_key_file_get_boolean (kf, group, "ApprovalRequired", NULL); - priv->title = g_key_file_get_string (kf, group, "Title", NULL); + /* all keys are optional */ + if (g_key_file_has_key (kf, group, "Enabled", NULL)) + priv->enabled = g_key_file_get_boolean (kf, group, "Enabled", NULL); + if (g_key_file_has_key (kf, group, "ApprovalRequired", NULL)) + priv->approval_required = g_key_file_get_boolean (kf, group, "ApprovalRequired", NULL); + if (g_key_file_has_key (kf, group, "Title", NULL)) { + g_autofree gchar *tmp = g_key_file_get_string (kf, group, "Title", NULL); + fwupd_remote_set_title (self, tmp); + } + if (g_key_file_has_key (kf, group, "ReportURI", NULL)) { + g_autofree gchar *tmp = g_key_file_get_string (kf, group, "ReportURI", NULL); + fwupd_remote_set_report_uri (self, tmp); + } + if (g_key_file_has_key (kf, group, "SecurityReportURI", NULL)) { + g_autofree gchar *tmp = g_key_file_get_string (kf, group, "SecurityReportURI", NULL); + fwupd_remote_set_security_report_uri (self, tmp); + } + if (g_key_file_has_key (kf, group, "Username", NULL)) { + g_autofree gchar *tmp = g_key_file_get_string (kf, group, "Username", NULL); + fwupd_remote_set_username (self, tmp); + } + if (g_key_file_has_key (kf, group, "Password", NULL)) { + g_autofree gchar *tmp = g_key_file_get_string (kf, group, "Password", NULL); + fwupd_remote_set_password (self, tmp); + } + if (g_key_file_has_key (kf, group, "FirmwareBaseURI", NULL)) { + g_autofree gchar *tmp = g_key_file_get_string (kf, group, "FirmwareBaseURI", NULL); + fwupd_remote_set_firmware_base_uri (self, tmp); + } + if (g_key_file_has_key (kf, group, "OrderBefore", NULL)) { + g_autofree gchar *tmp = g_key_file_get_string (kf, group, "OrderBefore", NULL); + fwupd_remote_set_order_before (self, tmp); + } + if (g_key_file_has_key (kf, group, "OrderAfter", NULL)) { + g_autofree gchar *tmp = g_key_file_get_string (kf, group, "OrderAfter", NULL); + fwupd_remote_set_order_after (self, tmp); + } + if (g_key_file_has_key (kf, group, "AutomaticReports", NULL)) + priv->automatic_reports = g_key_file_get_boolean (kf, group, "AutomaticReports", NULL); + if (g_key_file_has_key (kf, group, "AutomaticSecurityReports", NULL)) + priv->automatic_security_reports = g_key_file_get_boolean (kf, group, "AutomaticSecurityReports", NULL); - /* reporting is optional */ - report_uri = g_key_file_get_string (kf, group, "ReportURI", NULL); - if (report_uri != NULL && report_uri[0] != '\0') - fwupd_remote_set_report_uri (self, report_uri); - - /* security reporting is optional */ - security_report_uri = g_key_file_get_string (kf, group, "SecurityReportURI", NULL); - if (security_report_uri != NULL && security_report_uri[0] != '\0') - fwupd_remote_set_security_report_uri (self, security_report_uri); - - /* automatic report uploading */ - priv->automatic_reports = g_key_file_get_boolean (kf, group, "AutomaticReports", NULL); - priv->automatic_security_reports = g_key_file_get_boolean (kf, group, "AutomaticSecurityReports", NULL); - - /* DOWNLOAD-type remotes */ - if (priv->kind == FWUPD_REMOTE_KIND_DOWNLOAD) { - g_autofree gchar *filename_cache = NULL; - g_autofree gchar *username = NULL; - g_autofree gchar *password = NULL; - - /* the client has to download this and the signature */ - fwupd_remote_set_metadata_uri (self, metadata_uri); - - /* check the URI was valid */ - if (priv->metadata_uri == NULL) { - g_set_error (error, + /* we can override, hence the extra section */ + if (priv->kind == FWUPD_REMOTE_KIND_UNKNOWN) { + g_set_error_literal (error, FWUPD_ERROR, FWUPD_ERROR_INVALID_FILE, - "Failed to parse URI '%s' in %s", - metadata_uri, filename); - return FALSE; - } + "metadata kind invalid"); + return FALSE; + } - /* username and password are optional */ - username = g_key_file_get_string (kf, group, "Username", NULL); - if (username != NULL) - fwupd_remote_set_username (self, username); - password = g_key_file_get_string (kf, group, "Password", NULL); - if (password != NULL) - fwupd_remote_set_password (self, password); + /* some validation for DOWNLOAD types */ + if (priv->kind == FWUPD_REMOTE_KIND_DOWNLOAD) { + g_autofree gchar *filename_cache = NULL; if (priv->remotes_dir == NULL) { g_set_error_literal (error, FWUPD_ERROR, FWUPD_ERROR_INTERNAL, - "Remotes directory not set"); + "remotes directory not set"); return FALSE; } /* set cache to /var/lib... */ @@ -547,6 +579,25 @@ fwupd_remote_load_from_filename (FwupdRemote *self, fwupd_remote_set_filename_cache (self, filename_cache); } + /* some validation for DIRECTORY types */ + if (priv->kind == FWUPD_REMOTE_KIND_DIRECTORY) { + if (priv->keyring_kind != FWUPD_KEYRING_KIND_NONE) { + g_set_error (error, + FWUPD_ERROR, + FWUPD_ERROR_INVALID_FILE, + "keyring kind %s is not supported with directory remote", + fwupd_keyring_kind_to_string (priv->keyring_kind)); + return FALSE; + } + if (priv->firmware_base_uri != NULL) { + g_set_error_literal (error, + FWUPD_ERROR, + FWUPD_ERROR_INVALID_FILE, + "Directory remotes don't support firmware base URI"); + return FALSE; + } + } + /* load the checksum */ if (priv->filename_cache_sig != NULL && g_file_test (priv->filename_cache_sig, G_FILE_TEST_EXISTS)) { @@ -563,38 +614,6 @@ fwupd_remote_load_from_filename (FwupdRemote *self, fwupd_remote_set_checksum (self, NULL); } - /* the base URI is optional */ - firmware_base_uri = g_key_file_get_string (kf, group, "FirmwareBaseURI", NULL); - if (firmware_base_uri != NULL) - fwupd_remote_set_firmware_base_uri (self, firmware_base_uri); - - /* some validation around DIRECTORY types */ - if (priv->kind == FWUPD_REMOTE_KIND_DIRECTORY) { - if (priv->keyring_kind != FWUPD_KEYRING_KIND_NONE) { - g_set_error (error, - FWUPD_ERROR, - FWUPD_ERROR_INVALID_FILE, - "Keyring kind %s is not supported with directory remote", - fwupd_keyring_kind_to_string (priv->keyring_kind)); - return FALSE; - } - if (firmware_base_uri != NULL) { - g_set_error_literal (error, - FWUPD_ERROR, - FWUPD_ERROR_INVALID_FILE, - "Directory remotes don't support firmware base URI"); - return FALSE; - } - } - - /* dep logic */ - order_before = g_key_file_get_string (kf, group, "OrderBefore", NULL); - if (order_before != NULL) - priv->order_before = g_strsplit_set (order_before, ",:;", -1); - order_after = g_key_file_get_string (kf, group, "OrderAfter", NULL); - if (order_after != NULL) - priv->order_after = g_strsplit_set (order_after, ",:;", -1); - /* success */ fwupd_remote_set_filename_source (self, filename); return TRUE; @@ -1533,6 +1552,8 @@ fwupd_remote_class_init (FwupdRemoteClass *klass) static void fwupd_remote_init (FwupdRemote *self) { + FwupdRemotePrivate *priv = GET_PRIVATE (self); + priv->keyring_kind = FWUPD_KEYRING_KIND_JCAT; } static void diff --git a/libfwupd/fwupd-self-test.c b/libfwupd/fwupd-self-test.c index 089bfafe2..b762951d4 100644 --- a/libfwupd/fwupd-self-test.c +++ b/libfwupd/fwupd-self-test.c @@ -235,6 +235,30 @@ fwupd_remote_baseuri_func (void) g_assert_cmpstr (firmware_uri, ==, "https://my.fancy.cdn/firmware.cab"); } +static void +fwupd_remote_duplicate_func (void) +{ + gboolean ret; + g_autofree gchar *fn2 = NULL; + g_autofree gchar *fn = NULL; + g_autoptr(FwupdRemote) remote = fwupd_remote_new (); + g_autoptr(GError) error = NULL; + + fn = g_build_filename (TESTDATADIR, "tests", "remotes.d", "stable.conf", NULL); + ret = fwupd_remote_load_from_filename (remote, fn, NULL, &error); + g_assert_no_error (error); + g_assert_true (ret); + fn2 = g_build_filename (TESTDATADIR, "tests", "disabled.conf", NULL); + ret = fwupd_remote_load_from_filename (remote, fn2, NULL, &error); + g_assert_no_error (error); + g_assert_true (ret); + g_assert_false (fwupd_remote_get_enabled (remote)); + g_assert_cmpint (fwupd_remote_get_keyring_kind (remote), ==, FWUPD_KEYRING_KIND_NONE); + g_assert_cmpstr (fwupd_remote_get_username (remote), ==, NULL); + g_assert_cmpstr (fwupd_remote_get_password (remote), ==, ""); + g_assert_cmpstr (fwupd_remote_get_filename_cache (remote), ==, "/tmp/fwupd-self-test/stable.xml"); +} + /* verify we used the metadata path for firmware */ static void fwupd_remote_nopath_func (void) @@ -686,6 +710,7 @@ main (int argc, char **argv) g_test_add_func ("/fwupd/remote{base-uri}", fwupd_remote_baseuri_func); g_test_add_func ("/fwupd/remote{no-path}", fwupd_remote_nopath_func); g_test_add_func ("/fwupd/remote{local}", fwupd_remote_local_func); + g_test_add_func ("/fwupd/remote{duplicate}", fwupd_remote_duplicate_func); if (fwupd_has_system_bus ()) { g_test_add_func ("/fwupd/client{remotes}", fwupd_client_remotes_func); g_test_add_func ("/fwupd/client{devices}", fwupd_client_devices_func);