ANSI escape sequences are not interpreted in the message body

[johslarsen]

Earlier versions of alot interpreted ANSI escape sequences in the messages bodies for e.g. HTML coloring. This feature was broken by 11ea16f, because it also strips ESC \x1b control characters. Leaving just the coloring commands like e.g. [1;30m. Returning true for \x1b along side \n in the unicode_printable function brings back the old behavior.

Whether that feature was a good idea to begin with is debatable, so if you want it like this I don't mind, but then at least this part of the documentation should be removed. And I am guessing a lot of alot/widgets/ansi.py might be superfluous now

Software Versions

• Python version: 3.12.5
• Alot version: 0.11

[pazz]

Thanks for reporting this.
To be honest, when I signed off on the merge I did not think of ANSI codes and I think retrospectively it was a mistake to tacitly remove this functionality.
The original idea for interpreting ANSI codes was to allow for interpreting of text/* attachments such as code snippets, via filters such as the one in ranger. I still believe there is some merrit in this although I personally make not much use of this.

I agree that, should we keep the current version instead of reverting that commit, we should also amend the docs as you suggest. I'm in favor of reverting though.

thoughts?

[johslarsen]

I would suggest something like #1680 instead. That is enough to bring back the coloring support, and there are a lot of other characters that could cause problems that the original commits rightfully filtered out

[lfos]

@pazz @johslarsen I personally think that, if brought back, this should be done in a more careful manner: Remove or replace escape sequences from original emails but allow them to be added back by locally run filters.

If we reintroduce parsing of escape sequences in incoming messages and email subjects, maliciously crafted emails can mess with alot in a variety of ways, e.g., have a subject that rewrites the subject of other emails in the buffer, clear the screen and mangle/manipulate all parts of the UI. Depending on the used terminal, even more sinister attacks might be possible [1].

[1] https://unix.stackexchange.com/questions/15101/how-to-avoid-escape-sequence-attacks-in-terminals

[johslarsen]

If we reintroduce parsing of escape sequences in incoming messages and email subjects, maliciously crafted emails can mess with alot

I agree. Something like 9a5a0a9 might help a bit. This seems to strip the incoming control characters, and leave the rendered ones. However, stripping stuff before rendering moves the charset conversion from mailcap entries to alot, and I am not sure if that handles corner cases in the same manner. Non-ASCII characters in both ISO-8859-1 and UTF-8 mails seems to render fine on my end at least.

We would also need to limit the string sanitizing to text parts, right? Doing this for binary parts would risk mangling them before they were passed onto mailcap.

[lfos]

(9a5a0a9) might help a bit. This seems to strip the incoming control characters, and leave the rendered ones. However, stripping stuff before rendering moves the charset conversion from mailcap entries to alot, and I am not sure if that handles corner cases in the same manner. Non-ASCII characters in both ISO-8859-1 and UTF-8 mails seems to render fine on my end at least.

We would also need to limit the string sanitizing to text parts, right? Doing this for binary parts would risk mangling them before they were passed onto mailcap.

You're right. Thinking about this again, maybe this is better solved by distinguishing between filters with "safe" outputs and filters with "insecure" outputs. By default, all filters (including no filter) are insecure. Specific filters are marked as safe; the details of how to do so would need to be figured out. Control characters are removed from all insecure filters when rendering (which is essentially the current behavior) and are kept from filters explicitly marked as safe.

That way we'd avoid dealing with all the subleties of incoming messages and their encodings etc., always pass original content to filters, and only allow escape sequences when we know they are produced by filters that are safe. WDYT?

[johslarsen]

You're right. Thinking about this again, maybe this is better solved by distinguishing between filters with "safe" outputs and filters with "insecure" outputs.

By filters you are essentially talking about user-specified mailcap entries, right? If so, then users are kind of free to do whatever they like in the filter, and I don't think we should assume that they will do any sort of filtering on their end. From alot's perspective that kind of leave sanitizing before and/or after a filter, and without sanitizing the input I am not so sure that we would have any means of detecting whether control characters were added by the filter or not.

And I don't see how we would do any proper input filtering without dealing with encodings. For instance stripping ESC bytes is safe in most (I guess) 8-bit charsets and UTF-8, but we certainly can't go stripping away any single such bytes in a UTF-16 stream without messing up the rest of the text.

All in all I think this comes down to a trade-off between safe and unmodified inputs. Control characters are tied to terminals displaying the text so safeguarding against those is pretty obvious to focus on at the end after rendering and such. The input to a filter on the other hand would more likely be susceptible to others things like e.g. malformed HTML, and I think we would be better of leaning towards unmodified input than making it safe. Paranoid users are still able to add as much sanitizing as they like, and in general I think this being a text-only mail client makes it a lot safer to begin with than most other clients on the market.

Maybe something like a per content-type (or per mailcap entry or something) opt-in setting for this would be the best approach. Something like strip ESC from rendered output by default, but users could also choose to strip them before filtering instead (like 9a5a0a9) or even not at all if they want to handle as raw input as possible themselves

[lfos]

Right, something like a per-mailcap entry opt-in setting is exactly what I had in mind. That way, we can distinguish safe from unsafe filters (via users informing us about properties of their filters; sorry for not making that more clear in my previous reply).

[lfos]

As another alternative, we could try to allowlist certain escape characters; e.g., those that change color and basic formatting. That said, we'd need to be quite careful with what we allow to ensure there's no security concern.

[lfos]

As another alternative, we could try to allowlist certain escape characters; e.g., those that change color and basic formatting. That said, we'd need to be quite careful with what we allow to ensure there's no security concern.

Proposal of what this could look like:

diff --git a/alot/helper.py b/alot/helper.py
index d5ac9efe..8e912c0f 100644
--- a/alot/helper.py
+++ b/alot/helper.py
@@ -68,10 +68,32 @@ def string_sanitize(string, tab_width=8):
     'foo             bar'
     """
 
-    string = ''.join([c for c in string if unicode_printable(c)])
+    preprocessed_string = ''
+    i = 0
+    while i < len(string):
+        # Check if this is the start of an allowed ANSI escape sequence. For
+        # security reasons, we only allow SGR (Select Graphic Rendition)
+        # parameters; i.e., sequences used for basic formatting such as
+        # coloring and font variants.
+        if string[i:i + 2] == '\x1b[':
+            j = i + 2
+            # Skip over all "parameter bytes" and "intermediate bytes"
+            SKIP_CHARS = '0123456789:;<=>? !"#$%&\'()*+,-./'
+            while j < len(string) and string[j] in SKIP_CHARS:
+                j += 1
+            # Keep only SGR sequences in the preprocessed string
+            if j < len(string) and string[j] == 'm':
+                preprocessed_string += string[i:j + 1]
+            i = j + 1
+            continue
+
+        # Not a valid ANSI escape sequence, only allow printable characters.
+        if unicode_printable(string[i]):
+            preprocessed_string += string[i]
+        i += 1
 
     lines = list()
-    for line in string.split('\n'):
+    for line in preprocessed_string.split('\n'):
         tab_count = line.count('\t')
 
         if tab_count > 0:
diff --git a/tests/test_helper.py b/tests/test_helper.py
index a6b7c40c..2a3cb733 100644
--- a/tests/test_helper.py
+++ b/tests/test_helper.py
@@ -173,6 +173,18 @@ class TestStringSanitize(unittest.TestCase):
         actual = helper.string_sanitize(base)
         self.assertEqual(actual, expected)
 
+    def test_valid_ansi_escape_seq(self):
+        base = 'foo\x1b[1;31mred\x1b[0mreset'
+        expected = 'foo\x1b[1;31mred\x1b[0mreset'
+        actual = helper.string_sanitize(base)
+        self.assertEqual(actual, expected)
+
+    def test_invalid_ansi_escape_seq(self):
+        base = 'cursor\x1b[2A manipulation'
+        expected = 'cursor manipulation'
+        actual = helper.string_sanitize(base)
+        self.assertEqual(actual, expected)
+
 
 class TestStringDecode(unittest.TestCase):