From 1006007cc88d6727a993c1e9bbe6eb58dafa88ab Mon Sep 17 00:00:00 2001
From: baldurk <baldurk@baldurk.org>
Date: Fri, 19 May 2023 10:28:58 +0100
Subject: Sanitise strings printed when received from target control/remote
 server

* Given socket corruption or network errors these strings could contain
  unprintable characters so we sanitise them reasonably. This also ameliorates a
  potential security concern with arbitrary strings being written to a log, but
  these connections are still considered trusted and users should not be
  exposing RenderDoc ports to the internet.
---
 renderdoc/common/common.cpp        | 11 +++++++++++
 renderdoc/core/remote_server.cpp   |  2 +-
 renderdoc/core/target_control.cpp  | 29 ++++++++++++++++++-----------
 renderdoc/strings/string_utils.cpp | 12 ++++++++++++
 renderdoc/strings/string_utils.h   |  5 +++++
 5 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/renderdoc/common/common.cpp b/renderdoc/common/common.cpp
index f026eea16..670b3fbd0 100644
--- a/renderdoc/common/common.cpp
+++ b/renderdoc/common/common.cpp
@@ -473,6 +473,17 @@ void rdclog_direct(time_t utcTime, uint32_t pid, LogType type, const char *proje
     va_end(args2);
   }
 
+  // normalise newlines
+  {
+    char *nl = base;
+    while(*nl)
+    {
+      if(*nl == '\r')
+        *nl = '\n';
+      nl++;
+    }
+  }
+
   // likely path - string contains no newlines
   char *nl = strchr(base, '\n');
   if(nl == NULL)
diff --git a/renderdoc/core/remote_server.cpp b/renderdoc/core/remote_server.cpp
index 4944ab394..5153562f2 100644
--- a/renderdoc/core/remote_server.cpp
+++ b/renderdoc/core/remote_server.cpp
@@ -464,7 +464,7 @@ static void ActiveRemoteClientThread(ClientThread *threadData,
 
       reader.EndChunk();
 
-      RDCLOG("Taking ownership of '%s'.", path.c_str());
+      RDCLOG("Taking ownership of capture.");
 
       tempFiles.push_back(path);
     }
diff --git a/renderdoc/core/target_control.cpp b/renderdoc/core/target_control.cpp
index a63a4a2e6..bfc6a3ddd 100644
--- a/renderdoc/core/target_control.cpp
+++ b/renderdoc/core/target_control.cpp
@@ -31,6 +31,7 @@
 #include "os/os_specific.h"
 #include "replay/replay_driver.h"
 #include "serialise/serialiser.h"
+#include "strings/string_utils.h"
 
 static const uint32_t TargetControlProtocolVersion = 9;
 
@@ -484,6 +485,8 @@ void RenderDoc::TargetControlServerThread(Network::Socket *sock)
 
       ser.EndChunk();
 
+      strip_nonbasic(newClient);
+
       if(newClient.empty() || !IsProtocolVersionSupported(version))
       {
         RDCLOG("Invalid/Unsupported handshake '%s' / %d", newClient.c_str(), version);
@@ -605,12 +608,23 @@ public:
 
     m_Version = 0;
 
+    if(type == ePacket_Handshake)
     {
       READ_DATA_SCOPE();
       SERIALISE_ELEMENT(m_Version);
       SERIALISE_ELEMENT(m_Target);
       SERIALISE_ELEMENT(m_PID);
     }
+    else if(type == ePacket_Busy)
+    {
+      READ_DATA_SCOPE();
+      SERIALISE_ELEMENT(m_Version);
+      SERIALISE_ELEMENT(m_Target);
+      SERIALISE_ELEMENT(m_BusyClient);
+    }
+
+    strip_nonbasic(m_Target);
+    strip_nonbasic(m_BusyClient);
 
     reader.EndChunk();
 
@@ -745,17 +759,6 @@ public:
       reader.EndChunk();
       return msg;
     }
-    else if(type == ePacket_Busy)
-    {
-      READ_DATA_SCOPE();
-      SERIALISE_ELEMENT(msg.busy.clientName).Named("Client Name"_lit);
-
-      SAFE_DELETE(m_Socket);
-
-      RDCLOG("Got busy signal: '%s", msg.busy.clientName.c_str());
-      msg.type = TargetControlMessageType::Busy;
-      return msg;
-    }
     else if(type == ePacket_NewChild)
     {
       msg.type = TargetControlMessageType::NewChild;
@@ -884,8 +887,12 @@ public:
       RDCLOG("Used API: %s (%s & %s)", msg.apiUse.name.c_str(),
              presenting ? "Presenting" : "Not presenting",
              supported ? "supported" : "not supported");
+
       if(!supportMessage.empty())
+      {
+        strip_nonbasic(supportMessage);
         RDCLOG("Support: %s", supportMessage.c_str());
+      }
 
       reader.EndChunk();
       return msg;
diff --git a/renderdoc/strings/string_utils.cpp b/renderdoc/strings/string_utils.cpp
index 100ec9773..b2d02c8b4 100644
--- a/renderdoc/strings/string_utils.cpp
+++ b/renderdoc/strings/string_utils.cpp
@@ -141,6 +141,18 @@ rdcstr strip_extension(const rdcstr &path)
   return path.substr(0, offs);
 }
 
+rdcstr strip_nonbasic(rdcstr &str)
+{
+  for(char &c : str)
+  {
+    if((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '.' ||
+       c == ' ')
+      continue;
+
+    c = '_';
+  }
+}
+
 void split(const rdcstr &in, rdcarray<rdcstr> &out, const char sep)
 {
   if(in.empty())
diff --git a/renderdoc/strings/string_utils.h b/renderdoc/strings/string_utils.h
index e833b7263..bb6c45a2f 100644
--- a/renderdoc/strings/string_utils.h
+++ b/renderdoc/strings/string_utils.h
@@ -37,5 +37,10 @@ rdcstr get_basename(const rdcstr &path);
 rdcstr get_dirname(const rdcstr &path);
 rdcstr strip_extension(const rdcstr &path);
 
+// remove everything but alphanumeric ' ' and '.'
+// It replaces everything else with _
+// for logging strings where they might contain garbage characters
+rdcstr strip_nonbasic(rdcstr &str);
+
 void split(const rdcstr &in, rdcarray<rdcstr> &out, const char sep);
 void merge(const rdcarray<rdcstr> &in, rdcstr &out, const char sep);
-- 
2.30.2