v4 use letf for text/enriched bugfix.

v4 use letf for text/enriched bugfix.

[David Bremner]

This adds a comment to Tomi's previous version, and a test. I've
tested the test by commenting out Tomi's fix and running it under
emacs24 ("exploit" runs) and debian emacs 25.2 (which includes the
relevant fix. In the latter case, the test passes with the notmuch
code commented out, because emacs mime-rendering has been fixed. I
consider this reasonable, since it test success means this particular
exploit is blocked.

[PATCH 1/2] emacs: letf enriched-decode-display-prop for text/encriched display

[David Bremner]

From: Tomi Ollila <tomi.ollila@iki.fi>

Dynamically bind enriched-decode-display-prop when inserting
text/enriched part. This complements commit 9b0582383833 for
emacs versions before 24.4 which do not have advice-add
functionality.

Since emacs 25.3 this particular bug is fixed.
---
 emacs/notmuch-show.el | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 99390277..43debb26 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -773,14 +773,19 @@ will return nil if the CID is unknown or cannot be retrieved."
 (defun notmuch-show-insert-part-text/x-vcalendar (msg part content-type nth depth button)
   (notmuch-show-insert-part-text/calendar msg part content-type nth depth button))
 
-;; https://bugs.gnu.org/28350
-(defun notmuch-show--enriched-decode-display-prop (start end &optional param)
-  (list start end))
-
-(defun notmuch-show-insert-part-text/enriched (msg part content-type nth depth button)
-  (advice-add 'enriched-decode-display-prop :override
-	      #'notmuch-show--enriched-decode-display-prop)
-  nil)
+(if (version< emacs-version "25.3")
+    ;; https://bugs.gnu.org/28350
+    ;;
+    ;; For newer emacs, we fall back to notmuch-show-insert-part-*/*
+    ;; (see notmuch-show-handlers-for)
+    (defun notmuch-show-insert-part-text/enriched (msg part content-type nth depth button)
+      ;; By requiring enriched below, we ensure that the function enriched-decode-display-prop
+      ;; is defined before it will be shadowed by the letf below. Otherwise the version
+      ;; in enriched.el may be loaded a bit later and used instead (for the first time).
+      (require 'enriched)
+      (letf (((symbol-function 'enriched-decode-display-prop)
+		 (lambda (start end &optional param) (list start end))))
+	(notmuch-show-insert-part-*/* msg part content-type nth depth button))))
 
 (defun notmuch-show-get-mime-type-of-application/octet-stream (part)
   ;; If we can deduce a MIME type from the filename of the attachment,
-- 
2.15.0

[PATCH 2/2] test/emacs: add exploit mitigation test

[David Bremner]

This test will pass if either the notmuch show mitigation code is
working correctly, or upstream emacs mime handling code has it's own
fix for https://bugs.gnu.org/28350.
---
 test/T450-emacs-show.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/test/T450-emacs-show.sh b/test/T450-emacs-show.sh
index d6aa5b41..8db0e49b 100755
--- a/test/T450-emacs-show.sh
+++ b/test/T450-emacs-show.sh
@@ -198,5 +198,14 @@ This is an error
 stdout:
 This is output"
 
+test_begin_subtest "text/enriched exploit mitigation"
+add_message '[content-type]="text/enriched"
+             [body]="
+<x-display><param>(when (progn (read-only-mode -1) (insert ?p ?0 ?w ?n ?e ?d)) nil)</param>test</x-display>
+"'
+test_emacs '(notmuch-show "id:'$gen_msg_id'")
+	(test-visible-output "OUTPUT.raw")'
+output=$(head -1 OUTPUT.raw|cut -f1-4 -d' ')
+test_expect_equal "$output" "Notmuch Test Suite <test_suite@notmuchmail.org>"
 
 test_done
-- 
2.15.0

Re: v4 use letf for text/enriched bugfix.

[Tomi Ollila]

On Tue, Dec 05 2017, David Bremner wrote:

TL;DR; LGTM. :D

> This adds a comment to Tomi's previous version, and a test. I've
> tested the test by commenting out Tomi's fix and running it under
> emacs24 ("exploit" runs) and debian emacs 25.2 (which includes the
> relevant fix. In the latter case, the test passes with the notmuch
> code commented out, because emacs mime-rendering has been fixed. I
> consider this reasonable, since it test success means this particular
> exploit is blocked.

Thank you David for doing the work I had in my queue -- and the test!

So, for me was left testing the test:

First I tested on Fedora 27 -- it has emacs 25.3 so the workaround we
provide is not active -- all tests when doing
(cd test && ./T450-emacs-show.sh) PASS.

Next I recompiled the whole stuff on container with Ubuntu 14.04 userspace;
(emacs 23.4.1) With this I had problems breaking the fix so I could get the
test in question FAIL -- I just could not, and finally tested that the 
function (read-only-mode) (used in exploit) is not defined in emacs 23.
Since emacs 23 is deprecated IMO it is fine that this test does not test
the behaviour there -- I believe the protection we get testing emacs 24
is good enough here (I've examined the elisp code in question and it is
the same since emacs 23.1 to emacs 24.3 (at least)).

Finally I launched Centos 7.0.1406 based container, now with emacs 24.3.1.
After recompilation and testing that tests PASS normally, I could easily
break the fix and get the test FAIL!

So, series LGTM.

Tomi