From 31768a0e73f183cc29c1837f158d41b423d3ffef Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Tue, 14 Dec 2021 16:41:00 -0600 Subject: [PATCH 01/13] Add secure output encoding functions If we fail to use these when required, malicious web content could XSS Epiphany's internal pages. (As you might guess, the fact that these functions don't exist already indicates that is currently possible in various places.) Part-of: --- lib/ephy-output-encoding.c | 74 ++++++++++++++++++++++++++++++++++++++ lib/ephy-output-encoding.h | 38 ++++++++++++++++++++ lib/meson.build | 1 + 3 files changed, 113 insertions(+) create mode 100644 lib/ephy-output-encoding.c create mode 100644 lib/ephy-output-encoding.h diff --git a/lib/ephy-output-encoding.c b/lib/ephy-output-encoding.c new file mode 100644 index 000000000..7256059ed --- /dev/null +++ b/lib/ephy-output-encoding.c @@ -0,0 +1,74 @@ +/* -*- Mode: C; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +/* + * Copyright © Red Hat Inc. + * + * This file is part of Epiphany. + * + * Epiphany is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Epiphany is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Epiphany. If not, see . + */ + +#include "config.h" +#include "ephy-output-encoding.h" + +#include + +char * +ephy_encode_for_html_entity (const char *input) +{ + GString *str = g_string_new (input); + + g_string_replace (str, "&", "&", 0); + g_string_replace (str, "<", "<", 0); + g_string_replace (str, ">", ">", 0); + g_string_replace (str, "\"", """, 0); + g_string_replace (str, "'", "'", 0); + g_string_replace (str, "/", "/", 0); + + return g_string_free (str, FALSE); +} + +static char * +encode_all_except_alnum (const char *input, + const char *format) +{ + GString *str; + const char *c = input; + + if (!g_utf8_validate (input, -1, NULL)) + return g_strdup (""); + + str = g_string_new (NULL); + do { + gunichar u = g_utf8_get_char (c); + if (g_unichar_isalnum (u)) + g_string_append_unichar (str, u); + else + g_string_append_printf (str, format, u); + c = g_utf8_next_char (c); + } while (*c); + + return g_string_free (str, FALSE); +} + +char * +ephy_encode_for_html_attribute (const char *input) +{ + return encode_all_except_alnum (input, "&#x%02x;"); +} + +char * +ephy_encode_for_javascript (const char *input) +{ + return encode_all_except_alnum (input, "\\u%04u;"); +} diff --git a/lib/ephy-output-encoding.h b/lib/ephy-output-encoding.h new file mode 100644 index 000000000..7ff6a33bd --- /dev/null +++ b/lib/ephy-output-encoding.h @@ -0,0 +1,38 @@ +/* -*- Mode: C; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +/* + * Copyright © 2021 Red Hat Inc. + * + * This file is part of Epiphany. + * + * Epiphany is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Epiphany is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Epiphany. If not, see . + */ + +#pragma once + +#include + +G_BEGIN_DECLS + +/* These functions implement the OWASP XSS prevention output encoding rules: + * https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#output-encoding-rules-summary + * + * You must *carefully* read that document to safely inject untrusted data into + * web content. Here be dragons. + */ + +char *ephy_encode_for_html_entity (const char *input); +char *ephy_encode_for_html_attribute (const char *input); +char *ephy_encode_for_javascript (const char *input); + +G_END_DECLS diff --git a/lib/meson.build b/lib/meson.build index 894589a1a..264f9c5fb 100644 --- a/lib/meson.build +++ b/lib/meson.build @@ -20,6 +20,7 @@ libephymisc_sources = [ 'ephy-langs.c', 'ephy-notification.c', 'ephy-notification-container.c', + 'ephy-output-encoding.c', 'ephy-permissions-manager.c', 'ephy-profile-utils.c', 'ephy-search-engine-manager.c', -- GitLab From 2f0fcea640b6219e55ceb02aba796158e49650a4 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Tue, 14 Dec 2021 16:42:30 -0600 Subject: [PATCH 02/13] Add anti-XSS rules to HACKING file Part-of: --- HACKING.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/HACKING.md b/HACKING.md index 545d7c960..8a050769f 100644 --- a/HACKING.md +++ b/HACKING.md @@ -167,6 +167,19 @@ more `WebKitWebExtension`s (web process extensions). Meanwhile, each `WebKitWebView` will have one or more `WebKitWebPage`s. Only one page will be active in a view at a given time: the other pages are for process swaps. +# Security + +When injecting untrusted data into web content, you need to properly encode the +data for the relevant context in order to prevent XSS vulnerabilities. For +example: page titles could be malicious, URLs could be malicious, web app IDs +could be malicious, etc. You must carefully read and understand the [OWASP +XSS Prevention rules](https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html) +or you will mess up. `lib/ephy-output-encoding.h` contains functions to help +with this. + +When working with JavaScript, pay particular attention to Rule #8 "Prevent DOM- +based XSS" as it is tricky and requires care throughout your JavaScript. + # Debugging To enable debugging use the configure option `-Ddeveloper_mode=true`. -- GitLab From 359e465148254278337537b4f0ef4fd16df72bb4 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Tue, 14 Dec 2021 16:43:05 -0600 Subject: [PATCH 03/13] about-handler: properly encode page title/URL in about:overview Otherwise, web pages can execute code in about:overview via a malicious page title. It might be possible to do the same via the URL, so better encode that too. Fixes #1612 Part-of: --- embed/ephy-about-handler.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/embed/ephy-about-handler.c b/embed/ephy-about-handler.c index 4c794f526..aecb50731 100644 --- a/embed/ephy-about-handler.c +++ b/embed/ephy-about-handler.c @@ -27,6 +27,7 @@ #include "ephy-file-helpers.h" #include "ephy-flatpak-utils.h" #include "ephy-history-service.h" +#include "ephy-output-encoding.h" #include "ephy-prefs.h" #include "ephy-settings.h" #include "ephy-smaps.h" @@ -410,7 +411,8 @@ history_service_query_urls_cb (EphyHistoryService *history, EphyHistoryURL *url = (EphyHistoryURL *)l->data; const char *snapshot; g_autofree char *thumbnail_style = NULL; - g_autofree char *markup = NULL; + g_autofree char *encoded_title = NULL; + g_autofree char *encoded_url = NULL; snapshot = ephy_snapshot_service_lookup_cached_snapshot_path (snapshot_service, url->url); if (snapshot) @@ -418,15 +420,17 @@ history_service_query_urls_cb (EphyHistoryService *history, else ephy_embed_shell_schedule_thumbnail_update (shell, url); - markup = g_markup_escape_text (url->title, -1); + /* Title and URL are controlled by web content and could be malicious. */ + encoded_title = ephy_encode_for_html_attribute (url->title); + encoded_url = ephy_encode_for_html_attribute (url->url); g_string_append_printf (data_str, "" "
" " " " %s" "", - markup, url->url, _("Remove from overview"), - thumbnail_style ? thumbnail_style : "", url->title); + encoded_title, encoded_url, _("Remove from overview"), + thumbnail_style ? thumbnail_style : "", encoded_title); } data_str = g_string_append (data_str, -- GitLab From 23385ab95fa6771e8e6145682854c960f03af554 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Tue, 14 Dec 2021 16:44:36 -0600 Subject: [PATCH 04/13] about-handler: properly encode web app info in about:applications The web app has some partial control over its title, and full control over its URL. Let's be careful here to ensure the web app info cannot be used to execute code. Part-of: --- embed/ephy-about-handler.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/embed/ephy-about-handler.c b/embed/ephy-about-handler.c index aecb50731..ee169e40a 100644 --- a/embed/ephy-about-handler.c +++ b/embed/ephy-about-handler.c @@ -264,19 +264,37 @@ handle_applications_finished_cb (EphyAboutHandler *handler, for (p = applications; p; p = p->next) { EphyWebApplication *app = (EphyWebApplication *)p->data; + g_autofree char *html_encoded_id = NULL; + g_autofree char *encoded_icon_url = NULL; + g_autofree char *encoded_name = NULL; + g_autofree char *encoded_url = NULL; + g_autofree char *js_encoded_id = NULL; + g_autofree char *encoded_install_date = NULL; if (ephy_web_application_is_system (app)) continue; + /* Most of these fields are untrusted. The web app suggests its own title, + * which gets used in the app ID and icon URL. The main URL could contain + * anything. Install date is the only trusted field here in that it's + * constructed by Epiphany, but it's a freeform string and we're encoding + * everything else here anyway, so might as well encode this too. + */ + html_encoded_id = ephy_encode_for_html_attribute (app->id); + encoded_icon_url = ephy_encode_for_html_attribute (app->icon_url); + encoded_name = ephy_encode_for_html_entity (app->name); + encoded_url = ephy_encode_for_html_entity (app->url); + js_encoded_id = ephy_encode_for_javascript (app->id); + encoded_install_date = ephy_encode_for_html_entity (app->install_date); g_string_append_printf (data_str, "" "" "
%s
%s
" "" "%s
%s", - app->id, app->icon_url, app->name, app->url, _("Delete"), app->id, + html_encoded_id, encoded_icon_url, encoded_name, encoded_url, _("Delete"), js_encoded_id, /* Note for translators: this refers to the installation date. */ - _("Installed on:"), app->install_date); + _("Installed on:"), encoded_install_date); } g_string_append (data_str, ""); -- GitLab From 943d4be1ea059e5a6e091928b2b7cc928e80922e Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Tue, 14 Dec 2021 16:45:53 -0600 Subject: [PATCH 05/13] pdf-handler: fix a leak Part-of: --- embed/ephy-pdf-handler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/embed/ephy-pdf-handler.c b/embed/ephy-pdf-handler.c index 871d2487a..da10b7e02 100644 --- a/embed/ephy-pdf-handler.c +++ b/embed/ephy-pdf-handler.c @@ -121,7 +121,7 @@ pdf_file_loaded (GObject *source, gpointer user_data) { EphyPdfRequest *self = user_data; - GBytes *html_file; + g_autoptr (GBytes) html_file = NULL; g_autoptr (GError) error = NULL; g_autoptr (GString) html = NULL; g_autofree gchar *b64 = NULL; -- GitLab From 6a6799b7e6c690d14dc5a669f79be58ce52a952d Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Tue, 14 Dec 2021 16:46:07 -0600 Subject: [PATCH 06/13] pdf-handler: properly encode filename before inserting to HTML The file's name is suggested by the server, and could be malicious. We don't want it to be able to escape the HTML attribute context. The file data should already be safe because it is base-64 encoded. Here I'm just adjusting the code style to match what I've done for the filename. Part-of: --- embed/ephy-pdf-handler.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/embed/ephy-pdf-handler.c b/embed/ephy-pdf-handler.c index da10b7e02..bcc226c5f 100644 --- a/embed/ephy-pdf-handler.c +++ b/embed/ephy-pdf-handler.c @@ -23,6 +23,7 @@ #include "ephy-embed-container.h" #include "ephy-embed-shell.h" +#include "ephy-output-encoding.h" #include "ephy-web-view.h" #include @@ -124,8 +125,9 @@ pdf_file_loaded (GObject *source, g_autoptr (GBytes) html_file = NULL; g_autoptr (GError) error = NULL; g_autoptr (GString) html = NULL; - g_autofree gchar *b64 = NULL; g_autofree char *file_data = NULL; + g_autofree char *encoded_file_data = NULL; + g_autofree char *encoded_filename = NULL; gsize len = 0; if (!g_file_load_contents_finish (G_FILE (source), res, &file_data, &len, NULL, &error)) { @@ -134,13 +136,13 @@ pdf_file_loaded (GObject *source, return; } - html_file = g_resources_lookup_data ("/org/gnome/epiphany/pdfjs/web/viewer.html", 0, NULL); - - b64 = g_base64_encode ((const guchar *)file_data, len); g_file_delete_async (G_FILE (source), G_PRIORITY_DEFAULT, NULL, pdf_file_deleted, NULL); - html = g_string_new (""); - g_string_printf (html, g_bytes_get_data (html_file, NULL), b64, self->file_name ? self->file_name : ""); + html = g_string_new (NULL); + html_file = g_resources_lookup_data ("/org/gnome/epiphany/pdfjs/web/viewer.html", 0, NULL); + encoded_file_data = g_base64_encode ((const guchar *)file_data, len); + encoded_filename = self->file_name ? ephy_encode_for_html_attribute (self->file_name) : g_strdup (""); + g_string_printf (html, g_bytes_get_data (html_file, NULL), encoded_file_data, encoded_filename); finish_uri_scheme_request (self, g_strdup (html->str), NULL); } -- GitLab From d02518606ef365e42d60c2154759f8005fecbbe3 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Tue, 14 Dec 2021 16:47:00 -0600 Subject: [PATCH 07/13] view-source-handler: encode data passed to highlight.js The actual data here should be good already because it gets escaped by GLib, but this function is really designed for use in XML, so let's switch to the simpler Epiphany function designed for anti-XSS to make it more clear what's going on here. The URL is probably vulnerable, though, since a malicious URL could conceivably try to escape the HTML entity context. Encode that. Part-of: --- embed/ephy-view-source-handler.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/embed/ephy-view-source-handler.c b/embed/ephy-view-source-handler.c index 115f4b718..10e4ddc44 100644 --- a/embed/ephy-view-source-handler.c +++ b/embed/ephy-view-source-handler.c @@ -23,6 +23,7 @@ #include "ephy-embed-container.h" #include "ephy-embed-shell.h" +#include "ephy-output-encoding.h" #include "ephy-web-view.h" #include @@ -109,7 +110,9 @@ web_resource_data_cb (WebKitWebResource *resource, EphyViewSourceRequest *request) { g_autofree guchar *data = NULL; - g_autofree char *escaped_str = NULL; + g_autofree char *data_str = NULL; + g_autofree char *encoded_str = NULL; + g_autofree char *encoded_uri = NULL; g_autoptr (GError) error = NULL; g_autofree char *html = NULL; gsize length; @@ -120,8 +123,13 @@ web_resource_data_cb (WebKitWebResource *resource, return; } - /* Warning: data is not a string, so we pass length here because it's not NUL-terminated. */ - escaped_str = g_markup_escape_text ((const char *)data, length); + /* Convert data to a string */ + data_str = g_malloc (length + 1); + memcpy (data_str, data, length); + data_str[length] = '\0'; + + encoded_str = ephy_encode_for_html_entity (data_str); + encoded_uri = ephy_encode_for_html_entity (webkit_web_resource_get_uri (resource)); html = g_strdup_printf ("" " " @@ -136,8 +144,8 @@ web_resource_data_cb (WebKitWebResource *resource, " hljs.initLineNumbersOnLoad();" " 
%s
" "", - webkit_web_resource_get_uri (resource), - escaped_str); + encoded_uri, + encoded_str); finish_uri_scheme_request (request, g_steal_pointer (&html), NULL); } -- GitLab From 7562e5782f459cf1dcb81d7bc55d9136d8aed73d Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Tue, 14 Dec 2021 16:59:32 -0600 Subject: [PATCH 08/13] reader-handler: fix style error Part-of: --- embed/ephy-reader-handler.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/embed/ephy-reader-handler.c b/embed/ephy-reader-handler.c index 25adf90cb..0fa9750b2 100644 --- a/embed/ephy-reader-handler.c +++ b/embed/ephy-reader-handler.c @@ -123,8 +123,7 @@ enum_nick (GType enum_type, return nick; } -static -char * +static char * readability_get_property_string (WebKitJavascriptResult *js_result, char *property) { -- GitLab From 1ea654eec26fae671334e626646f9e89eb36224a Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Tue, 14 Dec 2021 17:07:30 -0600 Subject: [PATCH 09/13] reader-handler: encode data when constructing reader mode content This is necessary to prevent web content from escaping its intended context. Part-of: --- embed/ephy-reader-handler.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/embed/ephy-reader-handler.c b/embed/ephy-reader-handler.c index 0fa9750b2..0b6daa29f 100644 --- a/embed/ephy-reader-handler.c +++ b/embed/ephy-reader-handler.c @@ -24,6 +24,7 @@ #include "ephy-embed-container.h" #include "ephy-embed-shell.h" #include "ephy-lib-type-builtins.h" +#include "ephy-output-encoding.h" #include "ephy-settings.h" #include "ephy-web-view.h" @@ -157,7 +158,10 @@ readability_js_finish_cb (GObject *object, g_autoptr (WebKitJavascriptResult) js_result = NULL; g_autoptr (GError) error = NULL; g_autofree gchar *byline = NULL; + g_autofree gchar *encoded_byline = NULL; g_autofree gchar *content = NULL; + g_autofree gchar *encoded_content = NULL; + g_autofree gchar *encoded_title = NULL; g_autoptr (GString) html = NULL; g_autoptr (GBytes) style_css = NULL; const gchar *title; @@ -175,10 +179,15 @@ readability_js_finish_cb (GObject *object, byline = readability_get_property_string (js_result, "byline"); content = readability_get_property_string (js_result, "content"); + title = webkit_web_view_get_title (web_view); + + encoded_byline = byline ? ephy_encode_for_html_entity (byline) : g_strdup (""); + encoded_content = ephy_encode_for_html_entity (content); + encoded_title = ephy_encode_for_html_entity (title); - html = g_string_new (""); + html = g_string_new (NULL); style_css = g_resources_lookup_data ("/org/gnome/epiphany/readability/reader.css", G_RESOURCE_LOOKUP_FLAGS_NONE, NULL); - title = webkit_web_view_get_title (web_view); + font_style = enum_nick (EPHY_TYPE_PREFS_READER_FONT_STYLE, g_settings_get_enum (EPHY_SETTINGS_READER, EPHY_PREFS_READER_FONT_STYLE)); @@ -205,12 +214,12 @@ readability_js_finish_cb (GObject *object, "" "
", (gchar *)g_bytes_get_data (style_css, NULL), - title, + encoded_title, font_style, color_scheme, - title, - byline != NULL ? byline : ""); - g_string_append (html, content); + encoded_title, + encoded_byline); + g_string_append (html, encoded_content); g_string_append (html, ""); finish_uri_scheme_request (request, g_strdup (html->str), NULL); -- GitLab From fd86b51103c5af68d6d64254b3ac41d24ddc8449 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Tue, 14 Dec 2021 17:15:00 -0600 Subject: [PATCH 10/13] web-view: convert error pages to use autofree/autoptr Part-of: --- embed/ephy-web-view.c | 77 ++++++++++++++----------------------------- 1 file changed, 24 insertions(+), 53 deletions(-) diff --git a/embed/ephy-web-view.c b/embed/ephy-web-view.c index 12cd2455d..55d969020 100644 --- a/embed/ephy-web-view.c +++ b/embed/ephy-web-view.c @@ -1858,9 +1858,9 @@ format_network_error_page (const char *uri, const char **icon_name, const char **style) { - char *formatted_origin; - char *formatted_reason; - char *first_paragraph; + g_autofree char *formatted_origin = NULL; + g_autofree char *formatted_reason = NULL; + g_autofree char *first_paragraph = NULL; const char *second_paragraph; /* Page title when a site cannot be loaded due to a network error. */ @@ -1897,10 +1897,6 @@ format_network_error_page (const char *uri, *icon_name = "network-error-symbolic.svg"; *style = "default"; - - g_free (formatted_origin); - g_free (formatted_reason); - g_free (first_paragraph); } static void @@ -1914,10 +1910,10 @@ format_crash_error_page (const char *uri, const char **icon_name, const char **style) { - char *formatted_uri; - char *formatted_distributor; - char *first_paragraph; - char *second_paragraph; + g_autofree char *formatted_uri = NULL; + g_autofree char *formatted_distributor = NULL; + g_autofree char *first_paragraph = NULL; + g_autofree char *second_paragraph = NULL; /* Page title when a site cannot be loaded due to a page crash error. */ *page_title = g_strdup_printf (_("Problem Loading Page")); @@ -1950,11 +1946,6 @@ format_crash_error_page (const char *uri, *icon_name = "computer-fail-symbolic.svg"; *style = "default"; - - g_free (formatted_uri); - g_free (formatted_distributor); - g_free (first_paragraph); - g_free (second_paragraph); } static void @@ -2041,8 +2032,8 @@ format_tls_error_page (EphyWebView *view, const char **icon_name, const char **style) { - char *formatted_origin; - char *first_paragraph; + g_autofree char *formatted_origin = NULL; + g_autofree char *first_paragraph = NULL; /* Page title when a site is not loaded due to an invalid TLS certificate. */ *page_title = g_strdup_printf (_("Security Violation")); @@ -2076,9 +2067,6 @@ format_tls_error_page (EphyWebView *view, *icon_name = "channel-insecure-symbolic.svg"; *style = "danger"; - - g_free (formatted_origin); - g_free (first_paragraph); } static void @@ -2098,8 +2086,8 @@ format_unsafe_browsing_error_page (EphyWebView *view, const char **icon_name, const char **style) { - char *formatted_origin; - char *first_paragraph; + g_autofree char *formatted_origin = NULL; + g_autofree char *first_paragraph = NULL; /* Page title when a site is flagged by Google Safe Browsing verification. */ *page_title = g_strdup_printf (_("Security Warning")); @@ -2165,9 +2153,6 @@ format_unsafe_browsing_error_page (EphyWebView *view, *icon_name = "security-high-symbolic.svg"; *style = "danger"; - - g_free (formatted_origin); - g_free (first_paragraph); } static void @@ -2229,19 +2214,19 @@ ephy_web_view_load_error_page (EphyWebView *view, GError *error, gpointer user_data) { - GBytes *html_file; - GString *html = g_string_new (""); - char *origin = NULL; - char *lang = NULL; - char *page_title = NULL; - char *msg_title = NULL; - char *msg_body = NULL; - char *msg_details = NULL; - char *button_label = NULL; - char *hidden_button_label = NULL; - char *button_action = NULL; - char *hidden_button_action = NULL; - char *style_sheet = NULL; + g_autoptr (GBytes) html_file = NULL; + g_autoptr (GString) html = g_string_new (NULL); + g_autofree char *origin = NULL; + g_autofree char *lang = NULL; + g_autofree char *page_title = NULL; + g_autofree char *msg_title = NULL; + g_autofree char *msg_body = NULL; + g_autofree char *msg_details = NULL; + g_autofree char *button_label = NULL; + g_autofree char *hidden_button_label = NULL; + g_autofree char *button_action = NULL; + g_autofree char *hidden_button_action = NULL; + g_autofree char *style_sheet = NULL; const char *button_accesskey = NULL; const char *hidden_button_accesskey = NULL; const char *icon_name = NULL; @@ -2392,23 +2377,9 @@ ephy_web_view_load_error_page (EphyWebView *view, button_accesskey, button_label); #pragma GCC diagnostic pop - g_bytes_unref (html_file); - g_free (origin); - g_free (lang); - g_free (page_title); - g_free (msg_title); - g_free (msg_body); - g_free (msg_details); - g_free (button_label); - g_free (button_action); - g_free (hidden_button_label); - g_free (hidden_button_action); - g_free (style_sheet); - /* Make our history backend ignore the next page load, since it will be an error page. */ ephy_web_view_freeze_history (view); webkit_web_view_load_alternate_html (WEBKIT_WEB_VIEW (view), html->str, uri, 0); - g_string_free (html, TRUE); } static gboolean -- GitLab From abd54b7cde38f3328e063731d94d61fed87bad5e Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Tue, 14 Dec 2021 17:37:05 -0600 Subject: [PATCH 11/13] web-view: encode data in error pages Page titles and URLs are untrusted and could be nasty, so we need to encode them appropriately when injecting them into HTML. Part-of: --- embed/ephy-web-view.c | 41 ++++++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/embed/ephy-web-view.c b/embed/ephy-web-view.c index 55d969020..ae8413e94 100644 --- a/embed/ephy-web-view.c +++ b/embed/ephy-web-view.c @@ -38,6 +38,7 @@ #include "ephy-gsb-utils.h" #include "ephy-history-service.h" #include "ephy-lib-type-builtins.h" +#include "ephy-output-encoding.h" #include "ephy-permissions-manager.h" #include "ephy-prefs.h" #include "ephy-reader-handler.h" @@ -1858,6 +1859,8 @@ format_network_error_page (const char *uri, const char **icon_name, const char **style) { + g_autofree char *encoded_uri = NULL; + g_autofree char *encoded_origin = NULL; g_autofree char *formatted_origin = NULL; g_autofree char *formatted_reason = NULL; g_autofree char *first_paragraph = NULL; @@ -1869,7 +1872,8 @@ format_network_error_page (const char *uri, /* Message title when a site cannot be loaded due to a network error. */ *message_title = g_strdup (_("Unable to display this website")); - formatted_origin = g_strdup_printf ("%s", origin); + encoded_origin = ephy_encode_for_html_entity (origin); + formatted_origin = g_strdup_printf ("%s", encoded_origin); /* Error details when a site cannot be loaded due to a network error. */ first_paragraph = g_strdup_printf (_("The site at %s seems to be " "unavailable."), @@ -1891,7 +1895,8 @@ format_network_error_page (const char *uri, /* The button on the network error page. DO NOT ADD MNEMONICS HERE. */ *button_label = g_strdup (_("Reload")); - *button_action = g_strdup_printf ("window.location = '%s';", uri); + encoded_uri = ephy_encode_for_javascript (uri); + *button_action = g_strdup_printf ("window.location = '%s';", encoded_uri); /* Mnemonic for the Reload button on browser error pages. */ *button_accesskey = C_("reload-access-key", "R"); @@ -1910,6 +1915,8 @@ format_crash_error_page (const char *uri, const char **icon_name, const char **style) { + g_autofree char *html_encoded_uri = NULL; + g_autofree char *js_encoded_uri = NULL; g_autofree char *formatted_uri = NULL; g_autofree char *formatted_distributor = NULL; g_autofree char *first_paragraph = NULL; @@ -1921,7 +1928,8 @@ format_crash_error_page (const char *uri, /* Message title when a site cannot be loaded due to a page crash error. */ *message_title = g_strdup (_("Oops! There may be a problem")); - formatted_uri = g_strdup_printf ("%s", uri); + html_encoded_uri = ephy_encode_for_html_entity (uri); + formatted_uri = g_strdup_printf ("%s", html_encoded_uri); /* Error details when a site cannot be loaded due to a page crash error. */ first_paragraph = g_strdup_printf (_("The page %s may have caused Web to " "close unexpectedly."), @@ -1940,7 +1948,8 @@ format_crash_error_page (const char *uri, /* The button on the page crash error page. DO NOT ADD MNEMONICS HERE. */ *button_label = g_strdup (_("Reload")); - *button_action = g_strdup_printf ("window.location = '%s';", uri); + js_encoded_uri = ephy_encode_for_javascript (uri); + *button_action = g_strdup_printf ("window.location = '%s';", js_encoded_uri); /* Mnemonic for the Reload button on browser error pages. */ *button_accesskey = C_("reload-access-key", "R"); @@ -1959,6 +1968,7 @@ format_process_crash_error_page (const char *uri, const char **icon_name, const char **style) { + g_autofree char *encoded_uri = NULL; const char *first_paragraph; /* Page title when a site cannot be loaded due to a process crash error. */ @@ -1974,7 +1984,8 @@ format_process_crash_error_page (const char *uri, /* The button on the process crash error page. DO NOT ADD MNEMONICS HERE. */ *button_label = g_strdup (_("Reload")); - *button_action = g_strdup_printf ("window.location = '%s';", uri); + encoded_uri = ephy_encode_for_javascript (uri); + *button_action = g_strdup_printf ("window.location = '%s';", encoded_uri); /* Mnemonic for the Reload button on browser error pages. */ *button_accesskey = C_("reload-access-key", "R"); @@ -1993,6 +2004,7 @@ format_unresponsive_process_error_page (const char *uri, const char **icon_name, const char **style) { + g_autofree char *encoded_uri = NULL; const char *first_paragraph; /* Page title when web content has become unresponsive. */ @@ -2008,7 +2020,8 @@ format_unresponsive_process_error_page (const char *uri, /* The button on the unresponsive process error page. DO NOT ADD MNEMONICS HERE. */ *button_label = g_strdup (_("Reload")); - *button_action = g_strdup_printf ("window.location = '%s';", uri); + encoded_uri = ephy_encode_for_javascript (uri); + *button_action = g_strdup_printf ("window.location = '%s';", encoded_uri); /* Mnemonic for the Reload button on browser error pages. */ *button_accesskey = C_("reload-access-key", "R"); @@ -2032,6 +2045,7 @@ format_tls_error_page (EphyWebView *view, const char **icon_name, const char **style) { + g_autofree char *encoded_origin = NULL; g_autofree char *formatted_origin = NULL; g_autofree char *first_paragraph = NULL; @@ -2041,7 +2055,8 @@ format_tls_error_page (EphyWebView *view, /* Message title when a site is not loaded due to an invalid TLS certificate. */ *message_title = g_strdup (_("This Connection is Not Secure")); - formatted_origin = g_strdup_printf ("%s", origin); + encoded_origin = ephy_encode_for_html_entity (origin); + formatted_origin = g_strdup_printf ("%s", encoded_origin); /* Error details when a site is not loaded due to an invalid TLS certificate. */ first_paragraph = g_strdup_printf (_("This does not look like the real %s. " "Attackers might be trying to steal or " @@ -2086,6 +2101,7 @@ format_unsafe_browsing_error_page (EphyWebView *view, const char **icon_name, const char **style) { + g_autofree char *encoded_origin = NULL; g_autofree char *formatted_origin = NULL; g_autofree char *first_paragraph = NULL; @@ -2095,7 +2111,8 @@ format_unsafe_browsing_error_page (EphyWebView *view, /* Message title on the unsafe browsing error page. */ *message_title = g_strdup (_("Unsafe website detected!")); - formatted_origin = g_strdup_printf ("%s", origin); + encoded_origin = ephy_encode_for_html_entity (origin); + formatted_origin = g_strdup_printf ("%s", encoded_origin); /* Error details on the unsafe browsing error page. * https://developers.google.com/safe-browsing/v4/usage-limits#UserWarnings */ @@ -2166,7 +2183,8 @@ format_no_such_file_error_page (EphyWebView *view, const char **icon_name, const char **style) { - g_autofree gchar *formatted_origin = NULL; + g_autofree gchar *encoded_address = NULL; + g_autofree gchar *formatted_address = NULL; g_autofree gchar *first_paragraph = NULL; g_autofree gchar *second_paragraph = NULL; @@ -2176,10 +2194,11 @@ format_no_such_file_error_page (EphyWebView *view, /* Message title on the no such file error page. */ *message_title = g_strdup (_("File not found")); - formatted_origin = g_strdup_printf ("%s", view->address); + encoded_address = ephy_encode_for_html_entity (view->address); + formatted_address = g_strdup_printf ("%s", encoded_address); first_paragraph = g_strdup_printf (_("%s could not be found."), - formatted_origin); + formatted_address); second_paragraph = g_strdup_printf (_("Please check the file name for " "capitalization or other typing errors. Also check if " "it has been moved, renamed, or deleted.")); -- GitLab From 87fc964c741237726a8f7e2ba8b60316707a6f77 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Wed, 15 Dec 2021 13:09:30 -0600 Subject: [PATCH 12/13] ci: add script to run scan-build This lets us pass our own args to scan-build. In particular, we can make scan-build fail when it detects a problem, instead of manually inspecting whether it created an HTML report. Part-of: --- .gitlab-ci.yml | 3 +-- .run-scan-build | 5 +++++ 2 files changed, 6 insertions(+), 2 deletions(-) create mode 100755 .run-scan-build diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 71a55d6d5..fe603bb35 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -57,8 +57,7 @@ scanbuild: CONFIG_OPTS: '-Dprofile=Devel -Dunit_tests=enabled' script: - flatpak-builder --user --disable-rofiles-fuse --stop-at=${FLATPAK_MODULE} flatpak_app ${MANIFEST_PATH} - - flatpak build flatpak_app bash -c "source /usr/lib/sdk/llvm12/enable.sh; meson --prefix=/app ${CONFIG_OPTS} _build; ninja -C _build scan-build" - - if [[ -n "$(ls -A _build/meson-logs/scanbuild/)" ]]; then echo "Scan build log found, assuming defects exist"; exit 1; fi + - flatpak build flatpak_app bash -c "source /usr/lib/sdk/llvm12/enable.sh; meson --prefix=/app ${CONFIG_OPTS} _build; SCANBUILD=$(pwd)/.run-scan-build ninja -C _build scan-build" artifacts: when: on_failure paths: diff --git a/.run-scan-build b/.run-scan-build new file mode 100755 index 000000000..88fe82454 --- /dev/null +++ b/.run-scan-build @@ -0,0 +1,5 @@ +#!/bin/sh + +set -e + +scan-build -v --status-bugs "$@" -- GitLab From 5c9f4bf2544c9ed447096a5aba54cbf52e4ba51e Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Wed, 15 Dec 2021 13:21:44 -0600 Subject: [PATCH 13/13] ci: don't check for memory leaks, use-after-free, double free This is unfortunate, but the checker is too dumb. It also has no way to suppress false positives, so we either have to make undesirable changes to the code, or else disable the checker entirely. We only really want to disable the memory leak checker, but there's no way to do that without also disabling the check for use-after-free or double free, so here we are. My opinion of scan-build has declined drastically today. This is sad. But it seems like the best we can do. Part-of: --- .run-scan-build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.run-scan-build b/.run-scan-build index 88fe82454..ea778b932 100755 --- a/.run-scan-build +++ b/.run-scan-build @@ -2,4 +2,4 @@ set -e -scan-build -v --status-bugs "$@" +scan-build -v --status-bugs -disable-checker unix.Malloc "$@" -- GitLab