From 50d1d9c7e53692e06c72eab518e671846068acd1 Mon Sep 17 00:00:00 2001 From: Josh Date: Thu, 30 Mar 2023 18:03:08 -0500 Subject: [PATCH] Fix doubly sanitized strings (#727) * Add migration to de-sanitize database strings * Remove SanitizationHelper functions related to XML sanitization * Remove sanitization usage from website * Implement suggested changes --- .../Controllers/FriendsController.cs | 2 - .../Controllers/ReportController.cs | 2 - .../Controllers/Resources/PhotosController.cs | 2 - .../Controllers/Slots/ScoreController.cs | 2 - .../Controllers/UserController.cs | 2 - .../Controllers/SlotPageController.cs | 2 - .../Controllers/UserPageController.cs | 2 - .../Pages/SlotSettingsPage.cshtml.cs | 4 +- .../Pages/UserSettingsPage.cshtml.cs | 1 - ...igration.cs => CleanupSanitizedStrings.cs} | 32 +++++++++++---- .../Extensions/ControllerExtensions.cs | 1 - .../Helpers/SanitizationHelper.cs | 40 ------------------- 12 files changed, 26 insertions(+), 66 deletions(-) rename ProjectLighthouse/Administration/Maintenance/MigrationTasks/{CleanupXMLInjectionMigration.cs => CleanupSanitizedStrings.cs} (55%) diff --git a/ProjectLighthouse.Servers.GameServer/Controllers/FriendsController.cs b/ProjectLighthouse.Servers.GameServer/Controllers/FriendsController.cs index 3229e940..92098c38 100644 --- a/ProjectLighthouse.Servers.GameServer/Controllers/FriendsController.cs +++ b/ProjectLighthouse.Servers.GameServer/Controllers/FriendsController.cs @@ -34,8 +34,6 @@ public class FriendsController : ControllerBase NPData? npData = await this.DeserializeBody(); if (npData == null) return this.BadRequest(); - SanitizationHelper.SanitizeStringsInClass(npData); - List friends = new(); foreach (string friendName in npData.Friends ?? new List()) { diff --git a/ProjectLighthouse.Servers.GameServer/Controllers/ReportController.cs b/ProjectLighthouse.Servers.GameServer/Controllers/ReportController.cs index eb26129f..74bb62ee 100644 --- a/ProjectLighthouse.Servers.GameServer/Controllers/ReportController.cs +++ b/ProjectLighthouse.Servers.GameServer/Controllers/ReportController.cs @@ -37,8 +37,6 @@ public class ReportController : ControllerBase GameGriefReport? report = await this.DeserializeBody(); if (report == null) return this.BadRequest(); - SanitizationHelper.SanitizeStringsInClass(report); - if (string.IsNullOrWhiteSpace(report.JpegHash)) return this.BadRequest(); if (!FileHelper.ResourceExists(report.JpegHash)) return this.BadRequest(); diff --git a/ProjectLighthouse.Servers.GameServer/Controllers/Resources/PhotosController.cs b/ProjectLighthouse.Servers.GameServer/Controllers/Resources/PhotosController.cs index e9bad05c..401a3fe6 100644 --- a/ProjectLighthouse.Servers.GameServer/Controllers/Resources/PhotosController.cs +++ b/ProjectLighthouse.Servers.GameServer/Controllers/Resources/PhotosController.cs @@ -42,8 +42,6 @@ public class PhotosController : ControllerBase GamePhoto? photo = await this.DeserializeBody(); if (photo == null) return this.BadRequest(); - SanitizationHelper.SanitizeStringsInClass(photo); - foreach (PhotoEntity p in this.database.Photos.Where(p => p.CreatorId == user.UserId)) { if (p.LargeHash == photo.LargeHash) return this.Ok(); // photo already uplaoded diff --git a/ProjectLighthouse.Servers.GameServer/Controllers/Slots/ScoreController.cs b/ProjectLighthouse.Servers.GameServer/Controllers/Slots/ScoreController.cs index 80c725ea..c6c794c4 100644 --- a/ProjectLighthouse.Servers.GameServer/Controllers/Slots/ScoreController.cs +++ b/ProjectLighthouse.Servers.GameServer/Controllers/Slots/ScoreController.cs @@ -95,8 +95,6 @@ public class ScoreController : ControllerBase return this.BadRequest(); } - SanitizationHelper.SanitizeStringsInClass(score); - int slotId = id; if (slotType == "developer") slotId = await SlotHelper.GetPlaceholderSlotId(this.database, slotId, SlotType.Developer); diff --git a/ProjectLighthouse.Servers.GameServer/Controllers/UserController.cs b/ProjectLighthouse.Servers.GameServer/Controllers/UserController.cs index 53d495e0..4ff1aa46 100644 --- a/ProjectLighthouse.Servers.GameServer/Controllers/UserController.cs +++ b/ProjectLighthouse.Servers.GameServer/Controllers/UserController.cs @@ -72,8 +72,6 @@ public class UserController : ControllerBase if (update == null) return this.BadRequest(); - SanitizationHelper.SanitizeStringsInClass(update); - if (update.Biography != null) { if (update.Biography.Length > 512) return this.BadRequest(); diff --git a/ProjectLighthouse.Servers.Website/Controllers/SlotPageController.cs b/ProjectLighthouse.Servers.Website/Controllers/SlotPageController.cs index c92ad074..eee60619 100644 --- a/ProjectLighthouse.Servers.Website/Controllers/SlotPageController.cs +++ b/ProjectLighthouse.Servers.Website/Controllers/SlotPageController.cs @@ -68,8 +68,6 @@ public class SlotPageController : ControllerBase return this.Redirect("~/slot/" + id); } - // Prevent potential xml injection and censor content - msg = SanitizationHelper.SanitizeString(msg); msg = CensorHelper.FilterMessage(msg); bool success = await this.database.PostComment(token.UserId, id, CommentType.Level, msg); diff --git a/ProjectLighthouse.Servers.Website/Controllers/UserPageController.cs b/ProjectLighthouse.Servers.Website/Controllers/UserPageController.cs index ad73199b..71f8b409 100644 --- a/ProjectLighthouse.Servers.Website/Controllers/UserPageController.cs +++ b/ProjectLighthouse.Servers.Website/Controllers/UserPageController.cs @@ -44,8 +44,6 @@ public class UserPageController : ControllerBase return this.Redirect("~/user/" + id); } - // Prevent potential xml injection and censor content - msg = SanitizationHelper.SanitizeString(msg); msg = CensorHelper.FilterMessage(msg); bool success = await this.database.PostComment(token.UserId, id, CommentType.Profile, msg); diff --git a/ProjectLighthouse.Servers.Website/Pages/SlotSettingsPage.cshtml.cs b/ProjectLighthouse.Servers.Website/Pages/SlotSettingsPage.cshtml.cs index d218e7c0..6c1fbd6a 100644 --- a/ProjectLighthouse.Servers.Website/Pages/SlotSettingsPage.cshtml.cs +++ b/ProjectLighthouse.Servers.Website/Pages/SlotSettingsPage.cshtml.cs @@ -29,15 +29,13 @@ public class SlotSettingsPage : BaseLayout if (avatarHash != null) this.Slot.IconHash = avatarHash; - name = SanitizationHelper.SanitizeString(name); name = CensorHelper.FilterMessage(name); if (this.Slot.Name != name && name.Length <= 64) this.Slot.Name = name; - description = SanitizationHelper.SanitizeString(description); description = CensorHelper.FilterMessage(description); if (this.Slot.Description != description && description.Length <= 512) this.Slot.Description = description; - labels = LabelHelper.RemoveInvalidLabels(SanitizationHelper.SanitizeString(labels)); + labels = LabelHelper.RemoveInvalidLabels(labels); if (this.Slot.AuthorLabels != labels) this.Slot.AuthorLabels = labels; // ReSharper disable once InvertIf diff --git a/ProjectLighthouse.Servers.Website/Pages/UserSettingsPage.cshtml.cs b/ProjectLighthouse.Servers.Website/Pages/UserSettingsPage.cshtml.cs index 757100e0..a6b8596d 100644 --- a/ProjectLighthouse.Servers.Website/Pages/UserSettingsPage.cshtml.cs +++ b/ProjectLighthouse.Servers.Website/Pages/UserSettingsPage.cshtml.cs @@ -33,7 +33,6 @@ public class UserSettingsPage : BaseLayout if (avatarHash != null) this.ProfileUser.IconHash = avatarHash; - biography = SanitizationHelper.SanitizeString(biography); biography = CensorHelper.FilterMessage(biography); if (this.ProfileUser.Biography != biography && biography.Length <= 512) this.ProfileUser.Biography = biography; diff --git a/ProjectLighthouse/Administration/Maintenance/MigrationTasks/CleanupXMLInjectionMigration.cs b/ProjectLighthouse/Administration/Maintenance/MigrationTasks/CleanupSanitizedStrings.cs similarity index 55% rename from ProjectLighthouse/Administration/Maintenance/MigrationTasks/CleanupXMLInjectionMigration.cs rename to ProjectLighthouse/Administration/Maintenance/MigrationTasks/CleanupSanitizedStrings.cs index e7fd205e..c039b05d 100644 --- a/ProjectLighthouse/Administration/Maintenance/MigrationTasks/CleanupXMLInjectionMigration.cs +++ b/ProjectLighthouse/Administration/Maintenance/MigrationTasks/CleanupSanitizedStrings.cs @@ -1,20 +1,21 @@ +#nullable enable using System.Collections.Generic; +using System.Reflection; using System.Threading.Tasks; +using System.Web; using LBPUnion.ProjectLighthouse.Database; -using LBPUnion.ProjectLighthouse.Helpers; using LBPUnion.ProjectLighthouse.Types.Maintenance; namespace LBPUnion.ProjectLighthouse.Administration.Maintenance.MigrationTasks; -public class CleanupXmlInjectionMigration : IMigrationTask +public class CleanupSanitizedStrings : IMigrationTask { - public string Name() => "Cleanup XML injections"; + public string Name() => "Cleanup Sanitized strings"; - // Weird, but required. Thanks, hejlsberg. async Task IMigrationTask.Run(DatabaseContext database) { List objsToBeSanitized = new(); - + // Store all the objects we need to sanitize in a list. // The alternative here is to loop through every table, but thats a ton of code... objsToBeSanitized.AddRange(database.Slots); @@ -24,8 +25,25 @@ public class CleanupXmlInjectionMigration : IMigrationTask objsToBeSanitized.AddRange(database.Users); objsToBeSanitized.AddRange(database.Photos); objsToBeSanitized.AddRange(database.Reports); - - foreach (object obj in objsToBeSanitized) SanitizationHelper.SanitizeStringsInClass(obj); + + foreach (object obj in objsToBeSanitized) + { + PropertyInfo[] properties = obj.GetType().GetProperties(); + foreach (PropertyInfo property in properties) + { + if (property.PropertyType != typeof(string)) continue; + + string? before = (string?)property.GetValue(obj); + + if (before == null) continue; + + string after = HttpUtility.HtmlDecode(before); + if (before != after) + { + property.SetValue(obj, after); + } + } + } await database.SaveChangesAsync(); return true; diff --git a/ProjectLighthouse/Extensions/ControllerExtensions.cs b/ProjectLighthouse/Extensions/ControllerExtensions.cs index dbcda725..5f511527 100644 --- a/ProjectLighthouse/Extensions/ControllerExtensions.cs +++ b/ProjectLighthouse/Extensions/ControllerExtensions.cs @@ -120,7 +120,6 @@ public static partial class ControllerExtensions } XmlSerializer serializer = new(typeof(T), root); T? obj = (T?)serializer.Deserialize(new StringReader(bodyString)); - SanitizationHelper.SanitizeStringsInClass(obj); return obj; } catch (Exception e) diff --git a/ProjectLighthouse/Helpers/SanitizationHelper.cs b/ProjectLighthouse/Helpers/SanitizationHelper.cs index 50366631..0065bc9f 100644 --- a/ProjectLighthouse/Helpers/SanitizationHelper.cs +++ b/ProjectLighthouse/Helpers/SanitizationHelper.cs @@ -1,49 +1,9 @@ #nullable enable -using System.Collections.Generic; using System.ComponentModel.DataAnnotations; -using System.Linq; -using System.Reflection; namespace LBPUnion.ProjectLighthouse.Helpers; public static class SanitizationHelper { - - private static readonly Dictionary charsToReplace = new() { - {"<", "<"}, - {">", ">"}, - {"\"", """}, - {"'", "'"}, - }; - public static bool IsValidEmail(string? email) => !string.IsNullOrWhiteSpace(email) && new EmailAddressAttribute().IsValid(email); - - public static void SanitizeStringsInClass(object? instance) - { - if (instance == null) return; - PropertyInfo[] properties = instance.GetType().GetProperties(); - foreach (PropertyInfo property in properties) - { - if (property.PropertyType != typeof(string)) continue; - - string? before = (string?) property.GetValue(instance); - - if (before == null) continue; - if (!charsToReplace.Keys.Any(k => before.Contains(k))) continue; - - property.SetValue(instance, SanitizeString(before)); - } - } - - public static string SanitizeString(string? input) - { - if (input == null) return ""; - - foreach ((string? key, string? value) in charsToReplace) - { - input = input.Replace(key, value); - } - return input; - } - } \ No newline at end of file