sanitize MIDI parser (#38806)

Co-authored-by: Pieter-Jan Briers <pieterjan.briers@gmail.com>

diff --git a/Content.Client/Instruments/MidiParser/MidiParser.cs b/Content.Client/Instruments/MidiParser/MidiParser.cs
index 937384e439b9f828d9e4676c6650e1d86d44ca8b..0b65e472fc94da55349693dab661815c7aecdfd1 100644 (file)
--- a/Content.Client/Instruments/MidiParser/MidiParser.cs
+++ b/Content.Client/Instruments/MidiParser/MidiParser.cs
@@ -102,6 +102,8 @@ public static class MidiParser
                         // 0x03 is TrackName,
                         // 0x04 is InstrumentName
 
+                        // This string can potentially contain control characters, including 0x00 which can cause problems if it ends up in database entries via admin logs
+                        // we sanitize TrackName and InstrumentName after they have been send to the server
                         var text = Encoding.ASCII.GetString(metaData, 0, (int)metaLength);
                         switch (metaType)
                         {

diff --git a/Content.Server/Instruments/InstrumentSystem.cs b/Content.Server/Instruments/InstrumentSystem.cs
index 420bd44737e9f2c96a03724e1a73a77a3d5685be..f6a216227155bc65f0b9bdd5660b0bbd6eb52627 100644 (file)
--- a/Content.Server/Instruments/InstrumentSystem.cs
+++ b/Content.Server/Instruments/InstrumentSystem.cs
@@ -156,6 +156,15 @@ public sealed partial class InstrumentSystem : SharedInstrumentSystem
             return;
         }
 
+
+        foreach (var t in msg.Tracks)
+        {
+            // Remove any control characters that may be part of the midi file so they don't end up in the admin logs.
+            t?.SanitizeFields();
+            // Truncate any track names too long.
+            t?.TruncateFields(_cfg.GetCVar(CCVars.MidiMaxChannelNameLength));
+        }
+
         var tracksString = string.Join("\n",
             msg.Tracks
             .Where(t => t != null)
@@ -166,12 +175,6 @@ public sealed partial class InstrumentSystem : SharedInstrumentSystem
             LogImpact.Low,
             $"{ToPrettyString(args.SenderSession.AttachedEntity)} set the midi channels for {ToPrettyString(uid)} to {tracksString}");
 
-        // Truncate any track names too long.
-        foreach (var t in msg.Tracks)
-        {
-            t?.TruncateFields(_cfg.GetCVar(CCVars.MidiMaxChannelNameLength));
-        }
-
         activeInstrument.Tracks = msg.Tracks;
 
         Dirty(uid, activeInstrument);

diff --git a/Content.Shared/Instruments/SharedInstrumentComponent.cs b/Content.Shared/Instruments/SharedInstrumentComponent.cs
index 97eef752ebb9e859967f173c14a17bc542da1e6f..41bef649023287df0b9ebc88c0e7300a720cd41e 100644 (file)
--- a/Content.Shared/Instruments/SharedInstrumentComponent.cs
+++ b/Content.Shared/Instruments/SharedInstrumentComponent.cs
@@ -1,4 +1,5 @@
 using System.Collections;
+using System.Text;
 using Robust.Shared.Audio.Midi;
 using Robust.Shared.GameStates;
 using Robust.Shared.Serialization;
@@ -207,6 +208,18 @@ public sealed class MidiTrack
             ProgramName = Truncate(ProgramName, limit);
     }
 
+    public void SanitizeFields()
+    {
+        if (InstrumentName != null)
+            InstrumentName = Sanitize(InstrumentName);
+
+        if (TrackName != null)
+            TrackName = Sanitize(TrackName);
+
+        if (ProgramName != null)
+            ProgramName = Sanitize(ProgramName);
+    }
+
     private const string Postfix = "…";
     // TODO: Make a general method to use in RT? idk if we have that.
     private string Truncate(string input, int limit)
@@ -218,4 +231,17 @@ public sealed class MidiTrack
 
         return input.Substring(0, truncatedLength) + Postfix;
     }
+
+    private static string Sanitize(string input)
+    {
+        var sanitized = new StringBuilder(input.Length);
+
+        foreach (char c in input)
+        {
+            if (!char.IsControl(c) && c <= 127) // no control characters, only ASCII
+                sanitized.Append(c);
+        }
+
+        return sanitized.ToString();
+    }
 }