Merge pull request #376 from kyb3r/development

Security update

Author: Taaku18

CHANGELOG.md

@@ -6,6 +6,18 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 This project mostly adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html);
 however, insignificant breaking changes does not guarantee a major version bump, see the reasoning [here](https://github.com/kyb3r/modmail/issues/319).
 
+# v3.2.2
+
+Security update!
+
+### Important
+
+- Supporter permission users used to be able to "hack" snippets to reveal all your config vars, including your token and MongoURI.
+- Implemented some changes to address this bug:
+  - All customizable variables used in snippets, close messages, etc, using the `{}` syntax, now forbids chaining 2 or more attributes and attributes that starts with `_`.
+- It is advised to update to this version.
+- If you felt your credentials have been leaked, consider changing your bot token / mongo uri.
+
 # v3.2.1
 
 ### Fixed

bot.py

@@ -1,4 +1,4 @@
-__version__ = "3.2.1"
+__version__ = "3.2.2"
 
 import asyncio
 import logging
@@ -33,7 +33,7 @@
 from core.clients import ApiClient, PluginDatabaseClient
 from core.config import ConfigManager
 from core.utils import human_join, strtobool, parse_alias
-from core.models import PermissionLevel, ModmailLogger
+from core.models import PermissionLevel, ModmailLogger, SafeFormatter
 from core.thread import ThreadManager
 from core.time import human_timedelta
 
@@ -69,6 +69,7 @@ def __init__(self):
         self._session = None
         self._api = None
         self.metadata_loop = None
+        self.formatter = SafeFormatter()
 
         self._connected = asyncio.Event()
         self.start_time = datetime.utcnow()
@@ -120,7 +121,7 @@ def uptime(self) -> str:
         if days:
             fmt = "{d}d " + fmt
 
-        return fmt.format(d=days, h=hours, m=minutes, s=seconds)
+        return self.formatter.format(fmt, d=days, h=hours, m=minutes, s=seconds)
 
     def _configure_logging(self):
         level_text = self.config["log_level"].upper()
@@ -294,6 +295,8 @@ def guild_id(self) -> typing.Optional[int]:
             except ValueError:
                 self.config.remove("guild_id")
                 logger.critical("Invalid GUILD_ID set.")
+        else:
+            logger.debug("No GUILD_ID set.")
         return None
 
     @property
@@ -456,7 +459,7 @@ async def on_ready(self):
         await self.wait_for_connected()
 
         if self.guild is None:
-            logger.debug("Logging out due to invalid GUILD_ID.")
+            logger.error("Logging out due to invalid GUILD_ID.")
             return await self.logout()
 
         logger.line()
@@ -835,7 +838,7 @@ async def process_commands(self, message):
                 thread = await self.threads.find(channel=message.channel)
                 snippet = self.snippets[cmd]
                 if thread:
-                    snippet = snippet.format(recipient=thread.recipient)
+                    snippet = self.formatter.format(snippet, recipient=thread.recipient)
                 message.content = f"{self.prefix}reply {snippet}"
 
         ctxs = await self.get_contexts(message)

core/config.py

@@ -139,7 +139,7 @@ def populate_cache(self) -> dict:
         )
         if os.path.exists(config_json):
             logger.debug("Loading envs from config.json.")
-            with open(config_json, "r") as f:
+            with open(config_json, "r", encoding="utf-8") as f:
                 # Config json should override env vars
                 try:
                     data.update(

core/models.py

@@ -1,5 +1,7 @@
+import _string
 import logging
 from enum import IntEnum
+from string import Formatter
 
 from discord import Color, Embed
 from discord.ext import commands
@@ -85,3 +87,33 @@ class _Default:
 
 
 Default = _Default()
+
+
+class SafeFormatter(Formatter):
+
+    def get_field(self, field_name, args, kwargs):
+        first, rest = _string.formatter_field_name_split(field_name)
+
+        try:
+            obj = self.get_value(first, args, kwargs)
+        except (IndexError, KeyError):
+            return "<Invalid>", first
+
+        # loop through the rest of the field_name, doing
+        #  getattr or getitem as needed
+        # stops when reaches the depth of 2 or starts with _.
+        try:
+            for n, (is_attr, i) in enumerate(rest):
+                if n >= 2:
+                    break
+                if is_attr:
+                    if str(i).startswith('_'):
+                        break
+                    obj = getattr(obj, i)
+                else:
+                    obj = obj[i]
+            else:
+                return obj, first
+        except (IndexError, KeyError):
+            pass
+        return "<Invalid>", first

core/thread.py

@@ -393,8 +393,8 @@ async def _close(
             else:
                 message = self.bot.config["thread_close_response"]
 
-        message = message.format(
-            closer=closer, loglink=log_url, logkey=log_data["key"] if log_data else None
+        message = self.bot.formatter.format(
+            message, closer=closer, loglink=log_url, logkey=log_data["key"] if log_data else None
         )
 
         embed.description = message
@@ -492,7 +492,8 @@ async def _restart_close_timer(self):
             )
 
         # Grab message
-        close_message = self.bot.config["thread_auto_close_response"].format(
+        close_message = self.bot.formatter.format(
+            self.bot.config["thread_auto_close_response"],
             timeout=human_time
         )