bug#40397: 28.0.50; epg decrypt does not verify signed content in smime

[Sebastian Fieber <sebastian.fieber@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3973 bytes --]

On Mo, Apr 06 2020, Noam Postavsky <npostavs@gmail.com> wrote:

> This hunk looks a bit suspicious to me as well, I don't think you can
> apply operators like "?" to anchors.
>
> @@ -759,7 +782,7 @@ MIME-Version header before proceeding."
>          (mb enable-multibyte-characters)
>          beg)
>      (goto-char (point-min))
> -    (search-forward-regexp "^\n" nil 'move) ;; There might be no body.
> +    (search-forward-regexp "^?\n" nil 'move) ;; There might be no body.
>      (setq beg (point))
>      (with-current-buffer
>            (generate-new-buffer " *mm*")
>

Yes, this sections is also wrong.

>> Since you have looked over the patch: What do you think about the
>> approach to internally structure application/pkcs7-mime parts like
>> multipart parts containing the mime type with text properties until the
>> decrypted, maybe verified singlepart in the car of the handle?
> Sorry, I'm not familiar enough with how this code is currently
> structured to say anything intelligent about that.

No problem :)

I have attached a new patch which fixes the problem and also does
implement support for the security buttons for application/pkcs7-mime
parts.  This is quite nice as application/pkcs7-mime parts are not
handled automatically by default in gnus.  ATM you have to set
mm-decrypt-option and mm-verify-option at least to 'ask.  So with this
supported it should now work out of the box even without setting
mm-decrypt-option and mm-verify-option because now gnus shows the
buttons properly and one can click on them and decrypt/verify the part
"manually".

This time the patch should be clean and was tested properly at least
with mml-smime-use 'epg.  I'm not quite sure if the patch breaks using
openssl as I didn't get this running.  Maybe someone can test this? If
this does break using openssl modifying mm-views decrypt and verify
function should suffice to fix any problems.

The gist of the patch is: treat application/pkcs7-mime like multipart
mails and especially multipart/encrypted with protocol
application/pgp-encrypted and change not more stuff than necessary.

Here is the commit message which is a bit more detailed (also found in
the patch):

"This fixes S/MIME encrypted AND signed mails where in the encrypted
pkcs7 envelope is a signed pkcs7 structure.

Also this patch enables proper security-buttons for pkcs7-mime
encrypted and/or signed mails.

Changes:
- don't force Content-type header to text/plain in front of decrypted
  content for smime decryption using mm-view-pkcs7.  This fixes the
  initial bug where the signed part was not verified due to the wrong
  content type header.

- structure the result of mm-dissect-buffer of application/pkcs7-mime
  like a multipart mail so there is no loosing of information of
  verification and decryption results which can now be displayed by
  gnus-mime-display-security

- adjust gnus-mime-display-part to handle application/pkcs7-mime like
  multipart/encrypted or multipart/signed

- add dummy entries to mm-verify-function-alist and
  mm-decrypt-function-alist so gnus-mime-display-security correctly
  displays "S/MIME" and not "unknown protocol"

- don't just check for multipart/signed in
  gnus-insert-mime-security-button but also for the pkcs7-mime mimetypes
  to print "Encrypted" or "Signed" accordingly in the security button

- adjust mm-possibly-verify-or-decrypt to check for smime-type to ask
  wether to verify or decrypt the part and not to always ask to decrypt

- adjust mm-view-pkcs7-decrypt and verify to call mm-sec-status so
  success information can be displayed by gnus-mime-display-security

- in mm-view-pkcs7-verify also remove carriage returns like in
  mm-view-pkcs7-decrypt

- adjust gnus-mime-security-verify-or-decrypt to handle pkcs7-mime
  right with the done changes

TODO: mm-view-pkcs7-decrypt and verify error handling and
reporting. ATM there is only the good case implemented - at least for
reporting with gnus-mime-display-security."


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-fix-bug-40397.patch --]
[-- Type: text/x-patch, Size: 13674 bytes --]

From 3f85a1a72953f0877d2edcf56e872e7fe760b9f9 Mon Sep 17 00:00:00 2001
From: Sebastian Fieber <sebastian.fieber@web.de>
Date: Mon, 6 Apr 2020 20:45:05 +0200
Subject: [PATCH] fix bug #40397

This fixes S/MIME encrypted AND signed mails where in the encrypted
pkcs7 envelope is a signed pkcs7 structure.

Also this patch enables proper security-buttons for pkcs7-mime
encrypted and/or signed mails.

Changes:
- don't force Content-type header to text/plain in front of decrypted
  content for smime decryption using mm-view-pkcs7. This fixes the
  initial bug where the signed part was not verified due to the wrong
  content type header.

- structure the result of mm-dissect-buffer of application/pkcs7-mime
  like a multipart mail so there is no loosing of information of
  verification and decryption results which can now be displayed by
  gnus-mime-display-security

- adjust gnus-mime-display-part to handle application/pkcs7-mime like
  multipart/encrypted or multipart/signed

- add dummy entries to mm-verify-function-alist and
  mm-decrypt-function-alist so gnus-mime-display-security correctly
  displays "S/MIME" and not "unknown protocol"

- don't just check for multipart/signed in
  gnus-insert-mime-security-button but also for the pkcs7-mime mimetypes
  to print "Encrypted" or "Signed" accordingly in the security button

- adjust mm-possibly-verify-or-decrypt to check for smime-type to ask
  wether to verify or decrypt the part and not to always ask to decrypt

- adjust mm-view-pkcs7-decrypt and verify to call mm-sec-status so
  success information can be displayed by gnus-mime-display-security

- in mm-view-pkcs7-verify also remove carriage returns like in
  mm-view-pkcs7-decrypt

- adjust gnus-mime-security-verify-or-decrypt to handle pkcs7-mime
  right with the done changes

TODO: mm-view-pkcs7-decrypt and verify error handling and
reporting. ATM there is only the good case implemented - at least for
reporting with gnus-mime-display-security.
---
 lisp/gnus/gnus-art.el  | 60 ++++++++++++++++++++++++++++---
 lisp/gnus/mm-decode.el | 81 +++++++++++++++++++++++++++++++-----------
 lisp/gnus/mm-view.el   | 25 +++++++++++--
 3 files changed, 138 insertions(+), 28 deletions(-)

diff --git a/lisp/gnus/gnus-art.el b/lisp/gnus/gnus-art.el
index 6b9610d312..b130650df6 100644
--- a/lisp/gnus/gnus-art.el
+++ b/lisp/gnus/gnus-art.el
@@ -5986,6 +5986,34 @@ gnus-mime-display-part
    ((equal (car handle) "multipart/encrypted")
     (gnus-add-wash-type 'encrypted)
     (gnus-mime-display-security handle))
+   ;; pkcs7-mime handling:
+   ;;
+   ;; although not really multipart these are structured internally by
+   ;; mm-dissect-buffer like multipart to not discard the decryption
+   ;; and verification results
+   ;;
+   ;; application/pkcs7-mime
+   ((and (equal (car handle) "application/pkcs7-mime")
+         (equal (mm-handle-multipart-ctl-parameter handle 'protocol)
+                "application/pkcs7-mime_signed-data"))
+    (gnus-add-wash-type 'signed)
+    (gnus-mime-display-security handle))
+   ((and (equal (car handle) "application/pkcs7-mime")
+         (equal (mm-handle-multipart-ctl-parameter handle 'protocol)
+                "application/pkcs7-mime_enveloped-data"))
+    (gnus-add-wash-type 'encrypted)
+    (gnus-mime-display-security handle))
+   ;; application/x-pkcs7-mime
+   ((and (equal (car handle) "application/x-pkcs7-mime")
+         (equal (mm-handle-multipart-ctl-parameter handle 'protocol)
+                "application/x-pkcs7-mime_signed-data"))
+    (gnus-add-wash-type 'signed)
+    (gnus-mime-display-security handle))
+   ((and (equal (car handle) "application/x-pkcs7-mime")
+         (equal (mm-handle-multipart-ctl-parameter handle 'protocol)
+                "application/x-pkcs7-mime_enveloped-data"))
+    (gnus-add-wash-type 'encrypted)
+    (gnus-mime-display-security handle))
    ;; Other multiparts are handled like multipart/mixed.
    (t
     (gnus-mime-display-mixed (cdr handle)))))
@@ -8733,9 +8761,16 @@ gnus-mime-security-verify-or-decrypt
     (with-current-buffer (mm-handle-multipart-original-buffer handle)
       (let* ((mm-verify-option 'known)
 	     (mm-decrypt-option 'known)
-	     (nparts (mm-possibly-verify-or-decrypt (cdr handle) handle)))
+             (pkcs7-mime-p (or (equal (car handle) "application/pkcs7-mime")
+                               (equal (car handle) "application/x-pkcs7-mime")))
+	     (nparts (if pkcs7-mime-p
+                         (list (mm-possibly-verify-or-decrypt (cadr handle) (cadadr handle)))
+                       (mm-possibly-verify-or-decrypt (cdr handle) handle))))
 	(unless (eq nparts (cdr handle))
-	  (mm-destroy-parts (cdr handle))
+          ;; if pkcs7-mime don't destroy the parts as the buffer in
+          ;; the cdr still needs to be accessible
+          (when (not pkcs7-mime-p)
+	    (mm-destroy-parts (cdr handle)))
 	  (setcdr handle nparts))))
     (gnus-mime-display-security handle)
     (when region
@@ -8793,8 +8828,25 @@ gnus-insert-mime-security-button
 	   (or (nth 2 (assoc protocol mm-verify-function-alist))
 	       (nth 2 (assoc protocol mm-decrypt-function-alist))
 	       "Unknown")
-	   (if (equal (car handle) "multipart/signed")
-	       " Signed" " Encrypted")
+	   (cond ((equal (car handle) "multipart/signed") " Signed")
+	         ((equal (car handle) "multipart/encrypted") " Encrypted")
+                 ((and (equal (car handle) "application/pkcs7-mime")
+                       (equal (mm-handle-multipart-ctl-parameter handle 'protocol)
+                              "application/pkcs7-mime_signed-data"))
+                  " Signed")
+                 ((and (equal (car handle) "application/pkcs7-mime")
+                       (equal (mm-handle-multipart-ctl-parameter handle 'protocol)
+                              "application/pkcs7-mime_enveloped-data"))
+                  " Encrypted")
+                 ;; application/x-pkcs7-mime
+                 ((and (equal (car handle) "application/x-pkcs7-mime")
+                       (equal (mm-handle-multipart-ctl-parameter handle 'protocol)
+                              "application/x-pkcs7-mime_signed-data"))
+                  " Signed")
+                 ((and (equal (car handle) "application/x-pkcs7-mime")
+                       (equal (mm-handle-multipart-ctl-parameter handle 'protocol)
+                              "application/x-pkcs7-mime_enveloped-data"))
+                  " Encrypted"))
 	   " Part"))
 	 (gnus-tmp-info
 	  (or (mm-handle-multipart-ctl-parameter handle 'gnus-info)
diff --git a/lisp/gnus/mm-decode.el b/lisp/gnus/mm-decode.el
index 96695aabfd..da1a7c36a5 100644
--- a/lisp/gnus/mm-decode.el
+++ b/lisp/gnus/mm-decode.el
@@ -473,6 +473,7 @@ mm-dissect-default-type
 (autoload 'mml2015-verify-test "mml2015")
 (autoload 'mml-smime-verify "mml-smime")
 (autoload 'mml-smime-verify-test "mml-smime")
+(autoload 'mm-view-pkcs7-verify "mm-view")

 (defvar mm-verify-function-alist
   '(("application/pgp-signature" mml2015-verify "PGP" mml2015-verify-test)
@@ -481,7 +482,15 @@ mm-verify-function-alist
     ("application/pkcs7-signature" mml-smime-verify "S/MIME"
      mml-smime-verify-test)
     ("application/x-pkcs7-signature" mml-smime-verify "S/MIME"
-     mml-smime-verify-test)))
+     mml-smime-verify-test)
+    ("application/x-pkcs7-signature" mml-smime-verify "S/MIME"
+     mml-smime-verify-test)
+    ;; these are only used for security-buttons and contain the
+    ;; smime-type after the underscore
+    ("application/pkcs7-mime_signed-data" mm-view-pkcs7-verify "S/MIME"
+     nil)
+    ("application/x-pkcs7-mime_signed-data" mml-view-pkcs7-verify "S/MIME"
+     nil)))

 (defcustom mm-verify-option 'never
   "Option of verifying signed parts.
@@ -500,11 +509,16 @@ mm-verify-option

 (autoload 'mml2015-decrypt "mml2015")
 (autoload 'mml2015-decrypt-test "mml2015")
+(autoload 'mm-view-pkcs7-decrypt "mm-view")

 (defvar mm-decrypt-function-alist
   '(("application/pgp-encrypted" mml2015-decrypt "PGP" mml2015-decrypt-test)
     ("application/x-gnus-pgp-encrypted" mm-uu-pgp-encrypted-extract-1 "PGP"
-     mm-uu-pgp-encrypted-test)))
+     mm-uu-pgp-encrypted-test)
+    ;; these are only used for security-buttons and contain the
+    ;; smime-type after the underscore
+    ("application/pkcs7-mime_enveloped-data" mm-view-pkcs7-decrypt "S/MIME" nil)
+    ("application/x-pkcs7-mime_enveloped-data" mm-view-pkcs7-decrypt "S/MIME" nil)))

 (defcustom mm-decrypt-option nil
   "Option of decrypting encrypted parts.
@@ -682,14 +696,29 @@ mm-dissect-buffer
 				  (car ctl))
 	     (cons (car ctl) (mm-dissect-multipart ctl from))))
 	  (t
-	   (mm-possibly-verify-or-decrypt
-	    (mm-dissect-singlepart
-	     ctl
-	     (and cte (intern (downcase (mail-header-strip-cte cte))))
-	     no-strict-mime
-	     (and cd (mail-header-parse-content-disposition cd))
-	     description id)
-	    ctl from))))
+	   (let* ((handle
+                   (mm-dissect-singlepart
+	            ctl
+	            (and cte (intern (downcase (mail-header-strip-cte cte))))
+	            no-strict-mime
+	            (and cd (mail-header-parse-content-disposition cd))
+	            description id))
+                  (intermediate-result (mm-possibly-verify-or-decrypt handle ctl from)))
+             (when (and (equal type "application")
+                        (or (equal subtype "pkcs7-mime")
+                            (equal subtype "x-pkcs7-mime")))
+               (add-text-properties 0
+                                    (length (car ctl))
+                                    (list 'protocol
+                                          (concat (substring-no-properties (car ctl))
+                                                  "_"
+                                                  (cdr (assoc 'smime-type ctl))))
+                                    (car ctl))
+               ;; if this is a pkcs7-mime lets treat this special and
+               ;; more like multipart so the pkcs7-mime part does not
+               ;; get ignored
+               (setq intermediate-result (cons (car ctl) (list intermediate-result))))
+             intermediate-result))))
 	(when id
 	  (when (string-match " *<\\(.*\\)> *" id)
 	    (setq id (match-string 1 id)))
@@ -1672,17 +1701,27 @@ mm-possibly-verify-or-decrypt
     (cond
      ((or (equal type "application/x-pkcs7-mime")
 	  (equal type "application/pkcs7-mime"))
-      (with-temp-buffer
-	(when (and (cond
-		    ((eq mm-decrypt-option 'never) nil)
-		    ((eq mm-decrypt-option 'always) t)
-		    ((eq mm-decrypt-option 'known) t)
-		    (t (y-or-n-p
-			(format "Decrypt (S/MIME) part? "))))
-		   (mm-view-pkcs7 parts from))
-	  (goto-char (point-min))
-	  (insert "Content-type: text/plain\n\n")
-	  (setq parts (mm-dissect-buffer t)))))
+      (add-text-properties 0 (length (car ctl))
+			   (list 'buffer (car parts))
+			   (car ctl))
+      (let* ((smime-type (cdr (assoc 'smime-type ctl)))
+             (envelope-p (string= smime-type "enveloped-data"))
+             (decrypt-or-sign-option (if envelope-p
+                                         mm-decrypt-option
+                                       mm-verify-option))
+             (question (if envelope-p
+                           "Decrypt (S/MIME) part? "
+                         "Verify signed (S/MIME) part? ")))
+        (with-temp-buffer
+	  (when (and (cond
+		      ((eq decrypt-or-sign-option 'never) nil)
+		      ((eq decrypt-or-sign-option 'always) t)
+		      ((eq decrypt-or-sign-option 'known) t)
+		      (t (y-or-n-p
+			  (format question)))))
+	    (mm-view-pkcs7 parts from)
+	    (goto-char (point-min))
+	    (setq parts (mm-dissect-buffer t))))))
      ((equal subtype "signed")
       (unless (and (setq protocol
 			 (mm-handle-multipart-ctl-parameter ctl 'protocol))
diff --git a/lisp/gnus/mm-view.el b/lisp/gnus/mm-view.el
index 828ac633dc..34da9464ce 100644
--- a/lisp/gnus/mm-view.el
+++ b/lisp/gnus/mm-view.el
@@ -591,8 +591,15 @@ mm-view-pkcs7-verify
 	 (with-temp-buffer
 	   (insert-buffer-substring (mm-handle-buffer handle))
 	   (goto-char (point-min))
-	   (let ((part (base64-decode-string (buffer-string))))
-	     (epg-verify-string (epg-make-context 'CMS) part))))
+           (let* ((part (base64-decode-string (buffer-string)))
+                  (context (epg-make-context 'CMS))
+                  (plain (epg-verify-string context part)))
+             (mm-sec-status
+              'gnus-info
+              (epg-verify-result-to-string (epg-context-result-for context 'verify))
+              'gnus-details
+              nil)
+             plain)))
       (with-temp-buffer
 	(insert "MIME-Version: 1.0\n")
 	(mm-insert-headers "application/pkcs7-mime" "base64" "smime.p7m")
@@ -601,6 +608,10 @@ mm-view-pkcs7-verify
       (if verified
 	  (insert verified)
 	(insert-buffer-substring smime-details-buffer)))
+    (goto-char (point-min))
+    (while (search-forward "\r\n" nil t)
+      (replace-match "\n"))
+    (goto-char (point-min))
     t))

 (autoload 'epg-decrypt-string "epg")
@@ -612,7 +623,15 @@ mm-view-pkcs7-decrypt
       ;; Use EPG/gpgsm
       (let ((part (base64-decode-string (buffer-string))))
 	(erase-buffer)
-	(insert (epg-decrypt-string (epg-make-context 'CMS) part)))
+	(insert
+         (let* ((context (epg-make-context 'CMS))
+                (plain (epg-decrypt-string context part)))
+           (mm-sec-status
+            'gnus-info
+            "OK"
+            'gnus-details
+            nil)
+           plain)))
     ;; Use openssl
     (insert "MIME-Version: 1.0\n")
     (mm-insert-headers "application/pkcs7-mime" "base64" "smime.p7m")
--
2.25.2