Fix improper sanitization of web app properties that lead to security…

… vulnerabilities This contains a critical security fix. More details will be published soon.
filips123 · Apr 30, 2024 · 9932d4b · 9932d4b
1 parent 2ef4e0d
commit 9932d4b
Show file tree

Hide file tree

Showing 18 changed files with 135 additions and 63 deletions.
diff --git a/extension/src/sites/install.js b/extension/src/sites/install.js
@@ -14,6 +14,7 @@ import {
   obtainSiteList,
   obtainUrls,
   PREF_DEFAULT_PROFILE_TEMPLATE,
+  sanitizeString,
   setPopupSize
 } from '../utils'
 import { getMessage } from '../utils/i18n'
@@ -136,10 +137,12 @@ async function initializeForm () {
   document.getElementById('web-app-start-url').setAttribute('placeholder', manifest?.start_url || documentUrl)
 
   const categoriesElement = document.getElementById('web-app-categories')
-  for (const category of manifest?.categories || []) categoriesElement.tagsInstance.addItem(category, category)
+  const categoriesList = manifest?.categories?.map(item => sanitizeString(item)).filter(item => item) || []
+  for (const category of categoriesList) categoriesElement.tagsInstance.addItem(category, category)
 
   const keywordsElement = document.getElementById('web-app-keywords')
-  for (const keyword of manifest?.keywords || []) keywordsElement.tagsInstance.addItem(keyword, keyword)
+  const keywordsList = manifest?.keywords?.map(item => sanitizeString(item)).filter(item => item) || []
+  for (const keyword of keywordsList) keywordsElement.tagsInstance.addItem(keyword, keyword)
 
   // Add available profiles to the select input
   const profilesElement = document.getElementById('web-app-profile')
@@ -350,11 +353,11 @@ async function initializeForm () {
     // Get categories and keywords based on user form input and site manifest
     // If the user list is identical to the manifest, ignore it, otherwise, set it as a user overwrite
     const userCategories = [...document.getElementById('web-app-categories').selectedOptions].map(option => option.value)
-    const manifestCategories = manifest?.categories || []
+    const manifestCategories = manifest?.categories?.map(item => sanitizeString(item)).filter(item => item) || []
     const categories = userCategories.toString() !== manifestCategories.toString() ? userCategories : null
 
     const userKeywords = [...document.getElementById('web-app-keywords').selectedOptions].map(option => option.value)
-    const manifestKeywords = manifest?.keywords || []
+    const manifestKeywords = manifest?.keywords?.map(item => sanitizeString(item)).filter(item => item) || []
     const keywords = userKeywords.toString() !== manifestKeywords.toString() ? userKeywords : null
 
     // If the manifest does not exist, generate a "fake" manifest data URL

diff --git a/extension/src/sites/launch.js b/extension/src/sites/launch.js
@@ -1,7 +1,7 @@
 import '../utils/errors'
 import '../utils/i18nHtml'
 
-import { launchSite, obtainSiteList, obtainUrls, PREF_LAUNCH_CURRENT_URL, setPopupSize } from '../utils'
+import { launchSite, obtainSiteList, obtainUrls, PREF_LAUNCH_CURRENT_URL, sanitizeString, setPopupSize } from '../utils'
 import { getMessage } from '../utils/i18n'
 
 async function createInstanceList () {
@@ -22,11 +22,12 @@ async function createInstanceList () {
 
   // Create a list element for every instance with handler that launches it
   for (const site of sites) {
+    const name = sanitizeString(site.config.name || site.manifest.name || site.manifest.short_name)
     const url = settingsLaunchCurrentUrl ? documentUrl : undefined
 
     const siteElement = document.createElement('button')
     siteElement.classList.add(...['list-group-item', 'list-group-item-action'])
-    siteElement.innerText = site.config.name || site.manifest.name || site.manifest.short_name || new URL(site.manifest.scope).host
+    siteElement.innerText = name || new URL(site.manifest.scope).host
     siteElement.addEventListener('click', () => { launchSite(site, url) })
 
     listElement.append(siteElement)

diff --git a/extension/src/sites/manage.js b/extension/src/sites/manage.js
@@ -22,8 +22,10 @@ import {
   PREF_DEFAULT_PROFILE_TEMPLATE,
   PREF_DISPLAY_PAGE_ACTION,
   PREF_ENABLE_AUTO_LAUNCH,
-  PREF_LAUNCH_CURRENT_URL, PREF_LOCALE,
+  PREF_LAUNCH_CURRENT_URL,
+  PREF_LOCALE,
   PREF_SHOW_UPDATE_POPUP,
+  sanitizeString,
   setConfig,
   setPopupSize
 } from '../utils'
@@ -83,7 +85,8 @@ async function createSiteList () {
   // Create a list element for every instance with handlers for launching and editing
   for (const site of sites) {
     const siteElement = templateElement.content.firstElementChild.cloneNode(true)
-    const siteName = site.config.name || site.manifest.name || site.manifest.short_name || new URL(site.manifest.scope).host
+    const siteName = sanitizeString(site.config.name || site.manifest.name || site.manifest.short_name) || new URL(site.manifest.scope).host
+    const siteDescription = sanitizeString(site.config.description || site.manifest.description) || ''
     const siteIcon = site.config.icon_url || getIcon(buildIconList(site.manifest.icons), 64)
 
     const letterElement = siteElement.querySelector('#sites-list-template-letter')
@@ -106,7 +109,7 @@ async function createSiteList () {
     titleElement.removeAttribute('id')
 
     const descriptionElement = siteElement.querySelector('#sites-list-template-description')
-    descriptionElement.innerText = site.config.description || site.manifest.description || ''
+    descriptionElement.innerText = siteDescription
     descriptionElement.removeAttribute('id')
 
     const launchElement = siteElement.querySelector('#sites-list-template-launch')
@@ -123,8 +126,8 @@ async function createSiteList () {
       const submit = document.getElementById('web-app-submit')
 
       // Set placeholders from manifest
-      document.getElementById('web-app-name').setAttribute('placeholder', site.manifest.name || site.manifest.short_name || new URL(site.manifest.scope).host)
-      document.getElementById('web-app-description').setAttribute('placeholder', site.manifest.description || '')
+      document.getElementById('web-app-name').setAttribute('placeholder', sanitizeString(site.manifest.name || site.manifest.short_name) || new URL(site.manifest.scope).host)
+      document.getElementById('web-app-description').setAttribute('placeholder', sanitizeString(site.manifest.description) || '')
       document.getElementById('web-app-start-url').setAttribute('placeholder', site.manifest.start_url)
 
       // Set values from config
@@ -140,29 +143,31 @@ async function createSiteList () {
       categoriesElement.tagsInstance.reset()
 
       // Set categories from config or manifest
-      const categoriesList = site.config.categories?.length ? site.config.categories : site.manifest.categories
-      for (const category of categoriesList || []) categoriesElement.tagsInstance.addItem(category, category)
+      let categoriesList = site.config.categories?.length ? site.config.categories : site.manifest.categories
+      categoriesList = categoriesList?.map(item => sanitizeString(item)).filter(item => item) || []
+      for (const category of categoriesList) categoriesElement.tagsInstance.addItem(category, category)
 
       // Clear previous keywords
       const keywordsElement = document.getElementById('web-app-keywords')
       keywordsElement.tagsInstance.resetSearchInput()
       keywordsElement.tagsInstance.reset()
 
       // Set keywords from config or manifest
-      const keywordsList = site.config.keywords?.length ? site.config.keywords : site.manifest.keywords
-      for (const keyword of keywordsList || []) keywordsElement.tagsInstance.addItem(keyword, keyword)
+      let keywordsList = site.config.keywords?.length ? site.config.keywords : site.manifest.keywords
+      keywordsList = keywordsList?.map(item => sanitizeString(item)).filter(item => item) || []
+      for (const keyword of keywordsList) keywordsElement.tagsInstance.addItem(keyword, keyword)
 
       // Set site's profile from config
       const profilesElement = document.getElementById('web-app-profile')
       profilesElement.replaceChildren()
-      profilesElement.add(new Option(profiles[site.profile].name || site.profile, site.profile))
+      profilesElement.add(new Option(sanitizeString(profiles[site.profile].name) || site.profile, site.profile))
 
       // Create protocol handlers list and set enabled handlers
       // Currently not supported on macOS
       const platform = await browser.runtime.getPlatformInfo()
       if (platform.os !== 'mac') {
-        const possibleHandlers = new Set([...site.config.custom_protocol_handlers, ...site.manifest.protocol_handlers].map(handler => handler.protocol).sort())
-        const enabledHandlers = site.config.enabled_protocol_handlers
+        const possibleHandlers = new Set([...site.config.custom_protocol_handlers, ...site.manifest.protocol_handlers].map(handler => sanitizeString(handler.protocol)).filter(handler => handler).sort())
+        const enabledHandlers = site.config.enabled_protocol_handlers.map(handler => sanitizeString(handler)).filter(handler => handler)
 
         const handlersBox = document.getElementById('web-app-protocol-handlers-box')
         const handlersList = document.getElementById('web-app-protocol-handlers-list')
@@ -314,11 +319,11 @@ async function createSiteList () {
         // Get categories and keywords based on user form input and site manifest
         // If the user list is identical to the manifest, ignore it, otherwise, set it as a user overwrite
         const userCategories = [...document.getElementById('web-app-categories').selectedOptions].map(option => option.value)
-        const manifestCategories = site.manifest.categories || []
+        const manifestCategories = site.manifest.categories?.map(item => sanitizeString(item)).filter(item => item) || []
         const categories = userCategories.toString() !== manifestCategories.toString() ? userCategories : null
 
         const userKeywords = [...document.getElementById('web-app-keywords').selectedOptions].map(option => option.value)
-        const manifestKeywords = site.manifest.keywords || []
+        const manifestKeywords = site.manifest.keywords?.map(item => sanitizeString(item)).filter(item => item) || []
         const keywords = userKeywords.toString() !== manifestKeywords.toString() ? userKeywords : null
 
         // Get list of enabled protocol handlers
@@ -526,7 +531,7 @@ async function createProfileList () {
     const profileElement = templateElement.content.firstElementChild.cloneNode(true)
 
     const nameElement = profileElement.querySelector('#profiles-list-template-name')
-    nameElement.innerText = profile.name || await getMessage('managePageProfileListUnnamed')
+    nameElement.innerText = sanitizeString(profile.name) || await getMessage('managePageProfileListUnnamed')
     nameElement.removeAttribute('id')
 
     const countElement = profileElement.querySelector('#profiles-list-template-count')
@@ -537,7 +542,7 @@ async function createProfileList () {
     countElement.removeAttribute('id')
 
     const descriptionElement = profileElement.querySelector('#profiles-list-template-description')
-    descriptionElement.innerText = profile.description || ''
+    descriptionElement.innerText = sanitizeString(profile.description) || ''
     descriptionElement.removeAttribute('id')
 
     const editElement = profileElement.querySelector('#profiles-list-template-edit')
@@ -684,8 +689,8 @@ async function handleSearch () {
 
     document.getElementById('search-input').oninput = function () {
       for (const item of document.getElementById(listElement).children) {
-        const itemName = item.querySelector('.list-group-item-name')?.innerText.toLowerCase()
-        const searchQuery = this.value.toLowerCase()
+        const itemName = sanitizeString(item.querySelector('.list-group-item-name')?.innerText.toLowerCase())
+        const searchQuery = sanitizeString(this.value.toLowerCase())
 
         if (!itemName) continue
         item.classList.toggle('d-none', itemName.indexOf(searchQuery) === -1)

diff --git a/extension/src/utils.js b/extension/src/utils.js
@@ -237,6 +237,18 @@ export function isProtocolSchemePermitted (scheme) {
   return true
 }
 
+/**
+ * Removes all control characters from the string.
+ *
+ * @param {string?} string
+ *
+ * @returns {string|undefined}
+ */
+export function sanitizeString (string) {
+  // eslint-disable-next-line no-control-regex
+  return string?.replace(/[\u0000-\u001F\u007F-\u009F]/g, '')
+}
+
 /**
  * Launches the site in the app browser.
  *

diff --git a/native/src/components/site.rs b/native/src/components/site.rs
@@ -15,6 +15,7 @@ pub use web_app_manifest::WebAppManifest as SiteManifest;
 use crate::components::runtime::Runtime;
 use crate::directories::ProjectDirs;
 use crate::storage::Config;
+use crate::utils::sanitize_string;
 
 const DOWNLOAD_ERROR: &str = "Failed to download web app manifest";
 const DATA_URL_ERROR: &str = "Failed to process web app manifest data URL";
@@ -245,30 +246,36 @@ impl Site {
         }
     }
 
-    /// First tries the user-specified name, then try manifest name
+    /// First tries the user-specified name, then tries manifest name
     /// and then short name. If no name is specified, uses the domain.
     pub fn name(&self) -> String {
-        self.config
-            .name
-            .as_ref()
-            .cloned()
-            .or_else(|| self.manifest.name.as_ref().cloned())
-            .or_else(|| self.manifest.short_name.as_ref().cloned())
-            .unwrap_or_else(|| self.domain())
+        sanitize_string(
+            &self
+                .config
+                .name
+                .as_ref()
+                .cloned()
+                .or_else(|| self.manifest.name.as_ref().cloned())
+                .or_else(|| self.manifest.short_name.as_ref().cloned())
+                .unwrap_or_else(|| self.domain()),
+        )
     }
 
-    /// First tries the user-specified description, then try manifest description.
+    /// First tries the user-specified description, then tries manifest description.
     /// If no description is specified, returns an empty string.
     pub fn description(&self) -> String {
-        self.config
-            .description
-            .as_ref()
-            .cloned()
-            .or_else(|| self.manifest.description.as_ref().cloned())
-            .unwrap_or_else(|| "".into())
+        sanitize_string(
+            &self
+                .config
+                .description
+                .as_ref()
+                .cloned()
+                .or_else(|| self.manifest.description.as_ref().cloned())
+                .unwrap_or_else(|| "".into()),
+        )
     }
 
-    /// First tries the user-specified icon, then try manifest icons.
+    /// First tries the user-specified icon, then tries manifest icons.
     pub fn icons(&self) -> Vec<IconResource> {
         match &self.config.icon_url {
             Some(icon) => vec![IconResource {
@@ -288,11 +295,14 @@ impl Site {
     /// to XDG menu categories on Linux and Apple App Store categories on macOS.
     ///
     /// First tries the user-specified categories, then try manifest categories.
-    pub fn categories(&self) -> &[String] {
+    pub fn categories(&self) -> Vec<String> {
         match &self.config.categories {
             Some(categories) => categories,
             None => &self.manifest.categories,
         }
+        .iter()
+        .map(|item| sanitize_string(item))
+        .collect()
     }
 
     /// Keywords can also be used for user organization and contain
@@ -301,10 +311,13 @@ impl Site {
     /// Keywords are used as additional search queries on Linux.
     ///
     /// First tries the user-specified keywords, then try manifest keywords.
-    pub fn keywords(&self) -> &[String] {
+    pub fn keywords(&self) -> Vec<String> {
         match &self.config.keywords {
             Some(keywords) => keywords,
             None => &self.manifest.keywords,
         }
+        .iter()
+        .map(|item| sanitize_string(item))
+        .collect()
     }
 }
diff --git a/native/src/connector/process.rs b/native/src/connector/process.rs
@@ -75,7 +75,7 @@ impl Process for GetConfig {
 impl Process for SetConfig {
     fn process(&self, connection: &Connection) -> Result<ConnectorResponse> {
         let mut storage = Storage::load(connection.dirs)?;
-        storage.config = self.0.to_owned();
+        self.0.clone_into(&mut storage.config);
         storage.write(connection.dirs)?;
         Ok(ConnectorResponse::ConfigSet)
     }

diff --git a/native/src/console/profile.rs b/native/src/console/profile.rs
@@ -20,6 +20,7 @@ use crate::directories::ProjectDirs;
 use crate::integrations;
 use crate::integrations::IntegrationUninstallArgs;
 use crate::storage::Storage;
+use crate::utils::sanitize_string;
 
 fn apply_profile_template(
     template: &Option<PathBuf>,
@@ -48,8 +49,11 @@ impl Run for ProfileListCommand {
         for (_, profile) in storage.profiles {
             println!(
                 "{:=^60}\nDescription: {}\nID: {}",
-                format!(" {} ", profile.name.unwrap_or_else(|| "* Unnamed *".into())),
-                profile.description.unwrap_or_else(|| "* Nothing *".into()),
+                format!(
+                    " {} ",
+                    sanitize_string(&profile.name.unwrap_or_else(|| "* Unnamed *".into()))
+                ),
+                sanitize_string(&profile.description.unwrap_or_else(|| "* Nothing *".into())),
                 profile.ulid
             );
 

diff --git a/native/src/console/site.rs b/native/src/console/site.rs
@@ -1,4 +1,3 @@
-use std::convert::TryInto;
 use std::fs::metadata;
 use std::io;
 use std::io::Write;

diff --git a/native/src/integrations/implementation/linux.rs b/native/src/integrations/implementation/linux.rs
@@ -18,6 +18,7 @@ use crate::components::site::Site;
 use crate::integrations::categories::XDG_CATEGORIES;
 use crate::integrations::utils::{download_icon, normalize_category_name, process_icons};
 use crate::integrations::{IntegrationInstallArgs, IntegrationUninstallArgs};
+use crate::utils::sanitize_string;
 
 const BASE_DIRECTORIES_ERROR: &str = "Failed to determine base system directories";
 const CONVERT_ICON_URL_ERROR: &str = "Failed to convert icon URL";
@@ -205,7 +206,7 @@ fn create_desktop_entry(
     let mut categories = vec![];
     for category in args.site.categories() {
         // Normalize category name for easier matching
-        let category = normalize_category_name(category);
+        let category = normalize_category_name(&category);
 
         // Get the mapped XDG category based on the site categories
         if let Some(category) = XDG_CATEGORIES.get(&category) {
@@ -248,7 +249,7 @@ StartupWMClass={wmclass}
         protocols = args.site.config.enabled_protocol_handlers.iter().fold(
             String::new(),
             |mut output, protocol| {
-                let _ = write!(output, "x-scheme-handler/{protocol};");
+                let _ = write!(output, "x-scheme-handler/{};", sanitize_string(protocol));
                 output
             }
         ),
@@ -259,11 +260,12 @@ StartupWMClass={wmclass}
 
     // Store all shortcuts
     for (i, shortcut) in args.site.manifest.shortcuts.iter().enumerate() {
+        let name = sanitize_string(&shortcut.name);
         let url: Url = shortcut.url.clone().try_into().context(CONVERT_SHORTCUT_URL_ERROR)?;
         let icon = format!("{}-{}", ids.classid, i);
 
         if args.update_icons {
-            store_icons(&icon, &shortcut.name, &shortcut.icons, data, args.client.unwrap())
+            store_icons(&icon, &name, &shortcut.icons, data, args.client.unwrap())
                 .context("Failed to store shortcut icons")?;
         }
 
@@ -276,7 +278,7 @@ Exec={exe} site launch {siteid} --url \"{url}\"
 ",
             actionid = i,
             siteid = &ids.ulid,
-            name = &shortcut.name,
+            name = &name,
             icon = &icon,
             url = &url,
             exe = &exe,

diff --git a/native/src/integrations/implementation/macos.rs b/native/src/integrations/implementation/macos.rs
@@ -28,6 +28,7 @@ use crate::integrations::utils::{
     sanitize_name,
 };
 use crate::integrations::{IntegrationInstallArgs, IntegrationUninstallArgs};
+use crate::utils::sanitize_string;
 
 const BASE_DIRECTORIES_ERROR: &str = "Failed to determine base system directories";
 const CONVERT_ICON_URL_ERROR: &str = "Failed to convert icon URL";
@@ -444,9 +445,10 @@ fn create_app_bundle(args: &IntegrationInstallArgs) -> Result<()> {
         .enabled_protocol_handlers
         .iter()
         .map(|protocol| {
+            let protocol = sanitize_string(protocol);
             let mut handler = plist::dictionary::Dictionary::new();
             handler.insert("CFBundleURLName".into(), format!("{protocol} URL").into());
-            handler.insert("CFBundleURLSchemes".into(), vec![protocol.clone().into()].into());
+            handler.insert("CFBundleURLSchemes".into(), vec![protocol.into()].into());
             handler.into()
         })
         .collect::<Vec<plist::Value>>();