bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Illia Ostapyshyn]

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

* Bug

mml-smime-openssl-sign always takes the cdar of smime-keys, resulting in
keyfile parameter of the #secure tag being ignored.  Hence, only the
first entry of smime-keys is used, regardless of the mail contents or
sender address.

* Fix

The relevant information (returned from mml-smime-openssl-sign-query) is
already in the cont alist passed to mml-smime-openssl-sign, just use
that instead.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch --]
[-- Type: text/x-patch, Size: 1053 bytes --]

From 477badfc705c5dd59cfd8a577eab9eaf4a510e0f Mon Sep 17 00:00:00 2001
From: Illia Ostapyshyn <illia@yshyn.com>
Date: Wed, 20 Dec 2023 13:57:28 +0100
Subject: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

* lisp/gnus/mml-smime.el (mml-smime-openssl-sign): Use the key
passed in the cont argument instead of the first smime-keys entry.
---
 lisp/gnus/mml-smime.el | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/lisp/gnus/mml-smime.el b/lisp/gnus/mml-smime.el
index 896c95f8d3e..713b7fe5b68 100644
--- a/lisp/gnus/mml-smime.el
+++ b/lisp/gnus/mml-smime.el
@@ -130,10 +130,7 @@ mml-smime-verify-test
 	(funcall func handle ctl))))
 
 (defun mml-smime-openssl-sign (_cont)
-  (when (null smime-keys)
-    (customize-variable 'smime-keys)
-    (error "No S/MIME keys configured, use customize to add your key"))
-  (smime-sign-buffer (cdar smime-keys))
+  (smime-sign-buffer (cdr (assq 'keyfile cont)))
   (goto-char (point-min))
   (while (search-forward "\r\n" nil t)
     (replace-match "\n" t t))
-- 
2.43.0

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Stefan Kangas]

Illia Ostapyshyn <illia@yshyn.com> writes:

> * Bug
>
> mml-smime-openssl-sign always takes the cdar of smime-keys, resulting in
> keyfile parameter of the #secure tag being ignored.  Hence, only the
> first entry of smime-keys is used, regardless of the mail contents or
> sender address.
>
> * Fix
>
> The relevant information (returned from mml-smime-openssl-sign-query) is
> already in the cont alist passed to mml-smime-openssl-sign, just use
> that instead.

Thanks for the patch.

Could you please provide a way to reproduce the issue that you're
seeing?  We don't have anyone onboard that is deeply familiar with this
code, I think, and it is security-sensitive.  Therefore, I'd like to be
careful when making changes here.

If we could have unit tests for this, it would be even better, of course.

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Illia Ostapyshyn]

Hi Stefan,

I've been investigating this issue a bit more and discovered bug#17780.  My
original patch basically reverts its "fix" ac1507a8b6 (which wasn't a proper
fix), and there is another issue present.  I'm sending a new patch that fixes
both issues for good.  To recap:

- When composing a message signed with S/MIME, the workflow is to insert a
  "sign tag" using `mml-secure-sign-smime'.  When using openssl (as per
  mml-smime-use), this will search `smime-keys' for the keyfile and certs
  corresponding to the message sender (From header) and generate a sign MML
  tag [1].  Then, just before the message is sent, `mml-generate-mime' parses
  the tag and converts it into an alist passed to `mml-smime-openssl-sign',
  which executes openssl with the respective arguments from the alist/mml tag.

- Prior to bug#17780 patch this process would use the right keyfile from
  smime-keys, but would ignore additional certificates to be included in the
  message (third member of `smime-keys' entry).  The generated MML tag did not
  include certfiles and `mml-smime-openssl-sign' did not have the logic to
  process these, even if they were included in the tag/received alist.

- The applied patch ac1507a8b6 just uses (cdar smime-keys), which now includes
  the certfiles, but always takes the first entry of `smime-keys'.  If the
  user has setup several entries, i.e., different keys for subsequent mail
  addresses, this results in wrong keyfile/certs being used.  This is
  bug#67931.

The new patch complements `mml-secure-sign-smime' to include certfiles in the
generated tag.  With this, certfiles appear in the alist for
`mml-smime-openssl-sign', which is modified to process these entries and
forward them to `smime-sign-buffer'.

It also fixes a typo in documentation of `smime-sign-region': caar is meant to
be cadr.

> Could you please provide a way to reproduce the issue that you're
> seeing?

Here's a way to reproduce this in emacs -Q:

1. Start composing a message from bar@localhost with

(progn
  (setq mml-smime-use 'openssl
     	smime-keys '(("foo@localhost" "foo.pem" ("chain1foo.pem" "chain2foo.pem"))
                     ("bar@localhost" "bar.pem" ("chain1bar.pem" "chain2bar.pem"))
                     ("baz@localhost" "baz.pem" ("chain1baz.pem" "chain2baz.pem"))))
  (debug-on-entry #'smime-sign-buffer)
  (compose-mail "test@example.org" "#67931 reproducer" '((from . "bar@localhost"))))

2. Use `mml-secure-sign-smime' (C-c RET S s) to insert a tag on top of the
   message with the proper path for message sender bar@localhost:
   <#part sign=smime keyfile=bar.pem>

3. Use `message-send-and-exit` (C-c C-c) to trigger the breakpoint. This
   yields the following backtrace:

Debugger entered--entering a function:
* smime-sign-buffer(("foo.pem" ("chain1foo.pem" "chain2foo.pem")))
  mml-smime-openssl-sign((part (sign . "smime") (keyfile . "bar.pem") (tag-location . 202) (contents . "")))
  mml-smime-sign((part (sign . "smime") (keyfile . "bar.pem") (tag-location . 202) (contents . "")))
  mml-smime-sign-buffer((part (sign . "smime") (keyfile . "bar.pem") (tag-location . 202) (contents . "")))
  mml-generate-mime-1((part (sign . "smime") (keyfile . "bar.pem") (tag-location . 202) (contents . "")))
  mml-generate-mime(nil nil)
  message-encode-message-body()
  message-send-mail(nil)
  message-send-via-mail(nil)
  message-send(nil)
  message-send-and-exit(nil)
  funcall-interactively(message-send-and-exit nil)
  command-execute(message-send-and-exit)

Here, `smime-sign-buffer' signs the buffer with foo.pem, which corresponds to
smime-keys entry for foo@localhost, not bar@localhost.  As I described, (cdar
smime-keys) on line 136 in mml-smime.el always uses the first entry of
`smime-keys' regardless of the tag parameters.

In theory, `mml-smime-openssl-sign' should not access `smime-keys' at all, as
the keyfile/certfiles selection is handled (including the removed error
message and customize call) during sign tag generation in
`mml-secure-sign-smime'.  Instead, `mml-smime-openssl-sign' should use the
information from the tag passed in the cont argument (seen in the backtrace).

This is the case with this patch.  With it applied, the behavior changes:

- In step 2, the inserted tag now includes all the certfiles:
  <#part sign=smime keyfile=bar.pem certfile=chain1bar.pem certfile=chain2bar.pem>

- In step 3, `smime-sign-buffer' receives proper keyfile and all certfiles.

* smime-sign-buffer(("bar.pem" ("chain1bar.pem" "chain2bar.pem")))
  mml-smime-openssl-sign((part (sign . "smime") (keyfile . "bar.pem") (certfile . "chain1bar.pem") (certfile . "chain2bar.pem") (tag-location . 202) (contents . "")))

I've also updated the MML definition in documentation, since certfile
parameter is now common to both sign and encrypt tags.  Regarding the remark
about multiple entries: this is not new and already the case when encrypting
for multiple recipients (try `mml-secure-encrypt-smime'), but IMHO worth
clarifying, in case users desire write MML tags manually.

[1] https://www.gnu.org/software/emacs/manual/html_node/emacs-mime/MML-Definition.html

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Illia Ostapyshyn]

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

Sorry, forgot to attach the patch, sending it with this email.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: PATCH --]
[-- Type: text/x-patch, Size: 5550 bytes --]

From b228ee97f41911f2aba7b98ae1b5d1226e95e099 Mon Sep 17 00:00:00 2001
From: Illia Ostapyshyn <illia@yshyn.com>
Date: Mon, 6 May 2024 20:24:22 +0200
Subject: [PATCH] Use proper smime-keys entry for S/MIME signatures using
 OpenSSL

* lisp/gnus/mml-smime.el (mml-smime-openssl-sign-query): Include the
additional certificates from smime-keys in plist for MML tag generation.
(mml-smime-openssl-sign): Forward certfile entries from the MML tag to
smime-sign-buffer.
* doc/misc/emacs-mime.texi (MML Definition): certfile parameter is now
common to both sign and encrypt.  Clarify that certfile entries can be
repeated.
; * lisp/gnus/smime.el (smime-sign-region): Fix typo in documentation.
; (smime-sign-buffer): Improve documentation to match smime-sign-region.
---
 doc/misc/emacs-mime.texi | 11 +++-------
 lisp/gnus/mml-smime.el   | 46 +++++++++++++++++++++++-----------------
 lisp/gnus/smime.el       |  7 ++++--
 3 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/doc/misc/emacs-mime.texi b/doc/misc/emacs-mime.texi
index 96a6328cd47..e3e33bad8b4 100644
--- a/doc/misc/emacs-mime.texi
+++ b/doc/misc/emacs-mime.texi
@@ -780,21 +780,16 @@ MML Definition
 
 @end table
 
-Parameters for @samp{sign=smime}:
+Parameters for @samp{sign=smime} and @samp{encrypt=smime}:
 
 @table @samp
 
 @item keyfile
 File containing key and certificate for signer.
 
-@end table
-
-Parameters for @samp{encrypt=smime}:
-
-@table @samp
-
 @item certfile
-File containing certificate for recipient.
+File containing certificate for recipient.  May appear multiple times
+for multiple certificates.
 
 @end table
 
diff --git a/lisp/gnus/mml-smime.el b/lisp/gnus/mml-smime.el
index 3064c46d2a3..17b338755e3 100644
--- a/lisp/gnus/mml-smime.el
+++ b/lisp/gnus/mml-smime.el
@@ -129,11 +129,15 @@ mml-smime-verify-test
     (if func
 	(funcall func handle ctl))))
 
-(defun mml-smime-openssl-sign (_cont)
-  (when (null smime-keys)
-    (customize-variable 'smime-keys)
-    (error "No S/MIME keys configured, use customize to add your key"))
-  (smime-sign-buffer (cdar smime-keys))
+(defun mml-smime-openssl-sign (cont)
+  (smime-sign-buffer
+   ;; List with key and certificate as its car, and a list of additional
+   ;; certificates to include in its cadr for smime-sign-region
+   (list
+    (cdr (assq 'keyfile cont))
+    (mapcar #'cdr (cl-remove-if-not (apply-partially #'equal 'certfile)
+                                    cont
+                                    :key #'car-safe))))
   (goto-char (point-min))
   (while (search-forward "\r\n" nil t)
     (replace-match "\n" t t))
@@ -167,21 +171,23 @@ mml-smime-openssl-sign-query
   (when (null smime-keys)
     (customize-variable 'smime-keys)
     (error "No S/MIME keys configured, use customize to add your key"))
-  (list 'keyfile
-	(if (= (length smime-keys) 1)
-	    (cadar smime-keys)
-	  (or (let ((from (cadr (mail-extract-address-components
-				 (or (save-excursion
-				       (save-restriction
-					 (message-narrow-to-headers)
-					 (message-fetch-field "from")))
-				     "")))))
-		(and from (smime-get-key-by-email from)))
-	      (smime-get-key-by-email
-	       (gnus-completing-read "Sign this part with what signature"
-                                     (mapcar #'car smime-keys) nil nil nil
-                                     (and (listp (car-safe smime-keys))
-                                          (caar smime-keys))))))))
+  (let ((key-with-certs
+	 (if (= (length smime-keys) 1)
+	     (cdar smime-keys)
+	   (or (let ((from (cadr (mail-extract-address-components
+				  (or (save-excursion
+				        (save-restriction
+					  (message-narrow-to-headers)
+					  (message-fetch-field "from")))
+				      "")))))
+		 (and from (smime-get-key-with-certs-by-email from)))
+	       (smime-get-key-with-certs-by-email
+	        (gnus-completing-read "Sign this part with what signature"
+                                      (mapcar #'car smime-keys) nil nil nil
+                                      (and (listp (car-safe smime-keys))
+                                           (caar smime-keys))))))))
+    (append (list 'keyfile (car key-with-certs))
+            (mapcan (apply-partially #'list 'certfile) (cadr key-with-certs)))))
 
 (defun mml-smime-get-file-cert ()
   (ignore-errors
diff --git a/lisp/gnus/smime.el b/lisp/gnus/smime.el
index b61579912dd..987bc7273db 100644
--- a/lisp/gnus/smime.el
+++ b/lisp/gnus/smime.el
@@ -261,7 +261,7 @@ smime-sign-region
 If signing fails, the buffer is not modified.  Region is assumed to
 have proper MIME tags.  KEYFILE is expected to contain a PEM encoded
 private key and certificate as its car, and a list of additional
-certificates to include in its caar.  If no additional certificates is
+certificates to include in its cadr.  If no additional certificates are
 included, KEYFILE may be the file containing the PEM encoded private
 key and certificate itself."
   (smime-new-details-buffer)
@@ -327,7 +327,10 @@ smime-encrypt-region
 
 (defun smime-sign-buffer (&optional keyfile buffer)
   "S/MIME sign BUFFER with key in KEYFILE.
-KEYFILE should contain a PEM encoded key and certificate."
+KEYFILE is expected to contain a PEM encoded private key and certificate
+as its car, and a list of additional certificates to include in its
+cadr.  If no additional certificates are included, KEYFILE may be the
+file containing the PEM encoded private key and certificate itself."
   (interactive)
   (with-current-buffer (or buffer (current-buffer))
     (unless (smime-sign-region
-- 
2.39.2

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Eli Zaretskii]

> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 17780@debbugs.gnu.org,
>  Stefan Kangas <stefankangas@gmail.com>, Jan Beich <jbeich@vfemail.net>,
>  67931@debbugs.gnu.org
> From: Illia Ostapyshyn <illia@yshyn.com>
> Date: Mon, 06 May 2024 20:46:33 +0200
> 
> Sorry, forgot to attach the patch, sending it with this email.

Thanks, I'm adding Eric to the discussion.

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Illia Ostapyshyn]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 17780@debbugs.gnu.org,
>>  Stefan Kangas <stefankangas@gmail.com>, Jan Beich <jbeich@vfemail.net>,
>>  67931@debbugs.gnu.org
>> From: Illia Ostapyshyn <illia@yshyn.com>
>> Date: Mon, 06 May 2024 20:46:33 +0200
>> 
>> Sorry, forgot to attach the patch, sending it with this email.
>
> Thanks, I'm adding Eric to the discussion.

Thanks!

I've realized that reusing certfile parameter for signing will have
unintended side-effects when encrypting and signing a message.  When a
single signencrypt MML tag is used for both this results in all
certfiles passed to both `smime-encrypt-buffer' and `smime-sign-buffer'.

I'm sending a new patch that introduces a parameter called chainfile for
signatures instead.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: PATCH --]
[-- Type: text/x-patch, Size: 6446 bytes --]

From 6b6fb491247506becacb7a217e994b828be2ea2d Mon Sep 17 00:00:00 2001
From: Illia Ostapyshyn <illia@yshyn.com>
Date: Mon, 6 May 2024 20:24:22 +0200
Subject: [PATCH] Use proper smime-keys entry for S/MIME signatures using
 OpenSSL

* doc/misc/emacs-mime.texi (MML Definition):
* lisp/gnus/mml.el (mml-parse-1): Add chainfile parameter to sign tags.
* lisp/gnus/mml-smime.el (mml-smime-openssl-sign-query): Include the
additional certificates from smime-keys in MML tag generation as
chainfile parameters.
(mml-smime-openssl-sign): Forward chainfile entries from the parsed tag
alist to smime-sign-buffer.
; * lisp/gnus/smime.el (smime-sign-region): Fix typo in documentation.
; (smime-sign-buffer): Improve documentation to match smime-sign-region.
---
 doc/misc/emacs-mime.texi |  4 ++++
 lisp/gnus/mml-smime.el   | 46 +++++++++++++++++++++++-----------------
 lisp/gnus/mml.el         |  8 +++++++
 lisp/gnus/smime.el       |  7 ++++--
 4 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/doc/misc/emacs-mime.texi b/doc/misc/emacs-mime.texi
index 96a6328cd47..ef7ea614f8b 100644
--- a/doc/misc/emacs-mime.texi
+++ b/doc/misc/emacs-mime.texi
@@ -787,6 +787,10 @@ MML Definition
 @item keyfile
 File containing key and certificate for signer.
 
+@item chainfile
+File containing an additional certificate to be included with the
+message.
+
 @end table
 
 Parameters for @samp{encrypt=smime}:
diff --git a/lisp/gnus/mml-smime.el b/lisp/gnus/mml-smime.el
index 3064c46d2a3..9218bc079db 100644
--- a/lisp/gnus/mml-smime.el
+++ b/lisp/gnus/mml-smime.el
@@ -129,11 +129,15 @@ mml-smime-verify-test
     (if func
 	(funcall func handle ctl))))
 
-(defun mml-smime-openssl-sign (_cont)
-  (when (null smime-keys)
-    (customize-variable 'smime-keys)
-    (error "No S/MIME keys configured, use customize to add your key"))
-  (smime-sign-buffer (cdar smime-keys))
+(defun mml-smime-openssl-sign (cont)
+  (smime-sign-buffer
+   ;; List with key and certificate as its car, and a list of additional
+   ;; certificates to include in its cadr for smime-sign-region
+   (list
+    (cdr (assq 'keyfile cont))
+    (mapcar #'cdr (cl-remove-if-not (apply-partially #'equal 'chainfile)
+                                    cont
+                                    :key #'car-safe))))
   (goto-char (point-min))
   (while (search-forward "\r\n" nil t)
     (replace-match "\n" t t))
@@ -167,21 +171,23 @@ mml-smime-openssl-sign-query
   (when (null smime-keys)
     (customize-variable 'smime-keys)
     (error "No S/MIME keys configured, use customize to add your key"))
-  (list 'keyfile
-	(if (= (length smime-keys) 1)
-	    (cadar smime-keys)
-	  (or (let ((from (cadr (mail-extract-address-components
-				 (or (save-excursion
-				       (save-restriction
-					 (message-narrow-to-headers)
-					 (message-fetch-field "from")))
-				     "")))))
-		(and from (smime-get-key-by-email from)))
-	      (smime-get-key-by-email
-	       (gnus-completing-read "Sign this part with what signature"
-                                     (mapcar #'car smime-keys) nil nil nil
-                                     (and (listp (car-safe smime-keys))
-                                          (caar smime-keys))))))))
+  (let ((key-with-certs
+	 (if (= (length smime-keys) 1)
+	     (cdar smime-keys)
+	   (or (let ((from (cadr (mail-extract-address-components
+				  (or (save-excursion
+				        (save-restriction
+					  (message-narrow-to-headers)
+					  (message-fetch-field "from")))
+				      "")))))
+		 (and from (smime-get-key-with-certs-by-email from)))
+	       (smime-get-key-with-certs-by-email
+	        (gnus-completing-read "Sign this part with what signature"
+                                      (mapcar #'car smime-keys) nil nil nil
+                                      (and (listp (car-safe smime-keys))
+                                           (caar smime-keys))))))))
+    (append (list 'keyfile (car key-with-certs))
+            (mapcan (apply-partially #'list 'chainfile) (cadr key-with-certs)))))
 
 (defun mml-smime-get-file-cert ()
   (ignore-errors
diff --git a/lisp/gnus/mml.el b/lisp/gnus/mml.el
index edb3c286242..e3bc3932529 100644
--- a/lisp/gnus/mml.el
+++ b/lisp/gnus/mml.el
@@ -233,6 +233,10 @@ mml-parse-1
 					      (if (eq (car-safe tag) 'certfile)
 						  (cdr tag)))
 					    taginfo)))
+               (chainfiles (delq nil (mapcar (lambda (tag)
+                                               (if (eq (car-safe tag) 'chainfile)
+                                                   (cdr tag)))
+                                             taginfo)))
 	       (recipients (cdr (assq 'recipients taginfo)))
 	       (sender (cdr (assq 'sender taginfo)))
 	       (location (cdr (assq 'tag-location taginfo)))
@@ -267,6 +271,10 @@ mml-parse-1
 			    (mapcar (lambda (certfile)
 				      (list "certfile" certfile))
 				    certfiles))
+                   ,@(apply #'append
+                            (mapcar (lambda (chainfile)
+                                      (list "chainfile" chainfile))
+                                    chainfiles))
 		   ,(if recipients "recipients")
 		   ,recipients
 		   ,(if sender "sender")
diff --git a/lisp/gnus/smime.el b/lisp/gnus/smime.el
index b61579912dd..987bc7273db 100644
--- a/lisp/gnus/smime.el
+++ b/lisp/gnus/smime.el
@@ -261,7 +261,7 @@ smime-sign-region
 If signing fails, the buffer is not modified.  Region is assumed to
 have proper MIME tags.  KEYFILE is expected to contain a PEM encoded
 private key and certificate as its car, and a list of additional
-certificates to include in its caar.  If no additional certificates is
+certificates to include in its cadr.  If no additional certificates are
 included, KEYFILE may be the file containing the PEM encoded private
 key and certificate itself."
   (smime-new-details-buffer)
@@ -327,7 +327,10 @@ smime-encrypt-region
 
 (defun smime-sign-buffer (&optional keyfile buffer)
   "S/MIME sign BUFFER with key in KEYFILE.
-KEYFILE should contain a PEM encoded key and certificate."
+KEYFILE is expected to contain a PEM encoded private key and certificate
+as its car, and a list of additional certificates to include in its
+cadr.  If no additional certificates are included, KEYFILE may be the
+file containing the PEM encoded private key and certificate itself."
   (interactive)
   (with-current-buffer (or buffer (current-buffer))
     (unless (smime-sign-region
-- 
2.39.2

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Eric Abrahamsen]

Illia Ostapyshyn <illia@yshyn.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 17780@debbugs.gnu.org,
>>>  Stefan Kangas <stefankangas@gmail.com>, Jan Beich <jbeich@vfemail.net>,
>>>  67931@debbugs.gnu.org
>>> From: Illia Ostapyshyn <illia@yshyn.com>
>>> Date: Mon, 06 May 2024 20:46:33 +0200
>>> 
>>> Sorry, forgot to attach the patch, sending it with this email.
>>
>> Thanks, I'm adding Eric to the discussion.
>
> Thanks!
>
> I've realized that reusing certfile parameter for signing will have
> unintended side-effects when encrypting and signing a message.  When a
> single signencrypt MML tag is used for both this results in all
> certfiles passed to both `smime-encrypt-buffer' and `smime-sign-buffer'.
>
> I'm sending a new patch that introduces a parameter called chainfile for
> signatures instead.

Thanks for the report, and the code. I haven't been able to get the
reproducer to work so far (in Emacs -Q), because it always ends up at
`mml-smime-sign-query' instead of `mml-smime-sign-buffer', and the
latter seems to be the only way to (eventually) end up at
`mml-smime-openssl-sign', where the problem is:

- mml-smime-sign-buffer
- mml-smime-sign
- (funcall (nth 1 (assq 'openssl mml-smime-function-alist)))
- mml-smime-openssl-sign

`mml-smime-sign' is the only place that does (nth 1 (assq 'openssl
mml-smime-function-alist))

The only way to call `mml-smime-sign-buffer' instead of
`mml-smime-sign-query' is if some code ran:

(funcall (nth 1 (assoc "smime" mml-sign-alist)))

And so far as I can tell, no code does that.

Obviously you arrived at that function somehow, otherwise we wouldn't
have this bug report, but so far I can't figure out how!

Thanks,
Eric

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Eric Abrahamsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Illia Ostapyshyn <illia@yshyn.com> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 17780@debbugs.gnu.org,
>>>>  Stefan Kangas <stefankangas@gmail.com>, Jan Beich <jbeich@vfemail.net>,
>>>>  67931@debbugs.gnu.org
>>>> From: Illia Ostapyshyn <illia@yshyn.com>
>>>> Date: Mon, 06 May 2024 20:46:33 +0200
>>>> 
>>>> Sorry, forgot to attach the patch, sending it with this email.
>>>
>>> Thanks, I'm adding Eric to the discussion.
>>
>> Thanks!
>>
>> I've realized that reusing certfile parameter for signing will have
>> unintended side-effects when encrypting and signing a message.  When a
>> single signencrypt MML tag is used for both this results in all
>> certfiles passed to both `smime-encrypt-buffer' and `smime-sign-buffer'.
>>
>> I'm sending a new patch that introduces a parameter called chainfile for
>> signatures instead.
>
> Thanks for the report, and the code. I haven't been able to get the
> reproducer to work so far (in Emacs -Q), because it always ends up at
> `mml-smime-sign-query' instead of `mml-smime-sign-buffer', and the
> latter seems to be the only way to (eventually) end up at
> `mml-smime-openssl-sign', where the problem is:
>
> - mml-smime-sign-buffer
> - mml-smime-sign
> - (funcall (nth 1 (assq 'openssl mml-smime-function-alist)))
> - mml-smime-openssl-sign
>
> `mml-smime-sign' is the only place that does (nth 1 (assq 'openssl
> mml-smime-function-alist))
>
> The only way to call `mml-smime-sign-buffer' instead of
> `mml-smime-sign-query' is if some code ran:
>
> (funcall (nth 1 (assoc "smime" mml-sign-alist)))
>
> And so far as I can tell, no code does that.
>
> Obviously you arrived at that function somehow, otherwise we wouldn't
> have this bug report, but so far I can't figure out how!

Bah, I'm sorry, I didn't realize that was only half the recipe. Hang on...

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Eric Abrahamsen]

Illia Ostapyshyn <illia@yshyn.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 17780@debbugs.gnu.org,
>>>  Stefan Kangas <stefankangas@gmail.com>, Jan Beich <jbeich@vfemail.net>,
>>>  67931@debbugs.gnu.org
>>> From: Illia Ostapyshyn <illia@yshyn.com>
>>> Date: Mon, 06 May 2024 20:46:33 +0200
>>> 
>>> Sorry, forgot to attach the patch, sending it with this email.
>>
>> Thanks, I'm adding Eric to the discussion.
>
> Thanks!
>
> I've realized that reusing certfile parameter for signing will have
> unintended side-effects when encrypting and signing a message.  When a
> single signencrypt MML tag is used for both this results in all
> certfiles passed to both `smime-encrypt-buffer' and `smime-sign-buffer'.
>
> I'm sending a new patch that introduces a parameter called chainfile for
> signatures instead.

The patch seems to work as intended -- I won't claim to know enough
about SMIME to know if it does the right thing or not. Can you briefly
explain what the additional certificates actually do, and why they're
useful in signing but not in encryption?

Thanks,
Eric

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Illia Ostapyshyn]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> The patch seems to work as intended -- I won't claim to know enough
> about SMIME to know if it does the right thing or not. Can you briefly
> explain what the additional certificates actually do, and why they're
> useful in signing but not in encryption?

End-user SMIME certificates are signed by the (intermediate) CAs that
issued them.  The issuer's certificate can be in turn signed by another
CA up the hierarchy, resulting in a chain that ends with the implicitly
trusted root authority.  When signing a message, you can include the
intermediate CA certificates, allowing the recipient to verify the whole
chain.  With openssl, this is done via the -certfile argument [1]:

-certfile file
    Allows additional certificates to be specified. When signing these
    will be included with the message. When verifying these will be
    searched for the signers certificates. ...

Encryption is orthogonal to this: it only uses the public keys of your
recipients from their certificates, the chain is irrelevant.

The MML tag parameter names are a bit unfortunate here: the new
`chainfile' parameter translates to "-cerfile" arguments and the
existing `certfile' parameters translate to positional "recipcert"
arguments of openssl [1].

[1] https://www.openssl.org/docs/manmaster/man1/openssl-smime.html

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Eric Abrahamsen]

Illia Ostapyshyn <illia@yshyn.com> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> The patch seems to work as intended -- I won't claim to know enough
>> about SMIME to know if it does the right thing or not. Can you briefly
>> explain what the additional certificates actually do, and why they're
>> useful in signing but not in encryption?
>
> End-user SMIME certificates are signed by the (intermediate) CAs that
> issued them.  The issuer's certificate can be in turn signed by another
> CA up the hierarchy, resulting in a chain that ends with the implicitly
> trusted root authority.  When signing a message, you can include the
> intermediate CA certificates, allowing the recipient to verify the whole
> chain.  With openssl, this is done via the -certfile argument [1]:
>
> -certfile file
>     Allows additional certificates to be specified. When signing these
>     will be included with the message. When verifying these will be
>     searched for the signers certificates. ...

Thanks! So basically like TLS cert chaining.

> Encryption is orthogonal to this: it only uses the public keys of your
> recipients from their certificates, the chain is irrelevant.

I'm mostly trying to understand how broken this was, prior to this
patch. Obviously there was the hard-coding of the key, the original
issue. Has encryption been broken this whole time, too?

Encryption is a separate MML tag, right? And also a separate cert (the
recipient's, not the user's). Why would additional certificates on your
own certfile interfere with the process of encrypting to the user?

I'm not trying to be difficult, I'd just like to have a better grasp of
what's going on here!

> The MML tag parameter names are a bit unfortunate here: the new
> `chainfile' parameter translates to "-cerfile" arguments and the
> existing `certfile' parameters translate to positional "recipcert"
> arguments of openssl [1].

I'm not too concerned about that, the vast majority of the time this
process should be automatic.

Eric

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[illia]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> I'm mostly trying to understand how broken this was, prior to this
> patch. Obviously there was the hard-coding of the key, the original
> issue. Has encryption been broken this whole time, too?

Encryption is working as intended, I haven't encountered any problems
with it yet.

> Encryption is a separate MML tag, right? And also a separate cert (the
> recipient's, not the user's). Why would additional certificates on your
> own certfile interfere with the process of encrypting to the user?

Actually, when signing and encrypting at the same time, both use a
single "signencrypt" tag. This is what mml-secure-message-encrypt-smime
outputs currently:

<#secure method=smime mode=signencrypt keyfile=keyfile.pem certfile=recip.gpg>

mml-parse-1 converts this into an alist, spliting "signencrypt" into two
separate "sign" and "encrypt" parameters.  These are then processed in
mml-generate-mime-1, which consults mml-signencrypt-style-alist if it
encounters both sign and encrypt in the same tag.

With my previous patch (6 May) reusing the certfile parameter, the tag
would include chain certificates as certfiles:

<#secure method=smime mode=signencrypt keyfile=keyfile.pem certfile=chain.pem certfile=recip.pem>

With the same alist is passed to both mml-smime-openssl-sign and
mml-smime-openssl-encrypt, this had the unintended effect of (1)
encrypting for chain.pem and (2) including recip{1,2}.pem in the message
when signing.

With the latest patch, the tag looks like this:

<#secure method=smime mode=signencrypt keyfile=keyfile.pem chainfile=chain.pem certfile=recip.pem>

As mml-smime-openssl-sign expects chainfiles, mml-smime-openssl-encrypt
expects certfiles, and they don't interfere with each other anymore.

> I'm not trying to be difficult, I'd just like to have a better grasp of
> what's going on here!

No worries, I appreciate the additional caution with security-sensitive
code.  Also that part of the code seems to have been a bit neglected.

Illia

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Eric Abrahamsen]

illia@yshyn.com writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> I'm mostly trying to understand how broken this was, prior to this
>> patch. Obviously there was the hard-coding of the key, the original
>> issue. Has encryption been broken this whole time, too?
>
> Encryption is working as intended, I haven't encountered any problems
> with it yet.
>
>> Encryption is a separate MML tag, right? And also a separate cert (the
>> recipient's, not the user's). Why would additional certificates on your
>> own certfile interfere with the process of encrypting to the user?
>
> Actually, when signing and encrypting at the same time, both use a
> single "signencrypt" tag. This is what mml-secure-message-encrypt-smime
> outputs currently:
>
> <#secure method=smime mode=signencrypt keyfile=keyfile.pem certfile=recip.gpg>
>
> mml-parse-1 converts this into an alist, spliting "signencrypt" into two
> separate "sign" and "encrypt" parameters.  These are then processed in
> mml-generate-mime-1, which consults mml-signencrypt-style-alist if it
> encounters both sign and encrypt in the same tag.
>
> With my previous patch (6 May) reusing the certfile parameter, the tag
> would include chain certificates as certfiles:
>
> <#secure method=smime mode=signencrypt keyfile=keyfile.pem certfile=chain.pem certfile=recip.pem>
>
> With the same alist is passed to both mml-smime-openssl-sign and
> mml-smime-openssl-encrypt, this had the unintended effect of (1)
> encrypting for chain.pem and (2) including recip{1,2}.pem in the message
> when signing.
>
> With the latest patch, the tag looks like this:
>
> <#secure method=smime mode=signencrypt keyfile=keyfile.pem chainfile=chain.pem certfile=recip.pem>
>
> As mml-smime-openssl-sign expects chainfiles, mml-smime-openssl-encrypt
> expects certfiles, and they don't interfere with each other anymore.

Thank you very much, this was the hand-holding I needed.

>> I'm not trying to be difficult, I'd just like to have a better grasp of
>> what's going on here!
>
> No worries, I appreciate the additional caution with security-sensitive
> code.  Also that part of the code seems to have been a bit neglected.

As we can see from the previous bug report, no one seems to understand
how this works! Though the punchline probably is: you're the only one
still using S/MIME.

Anyway, I'm feeling okay about this. If you think this is ready to go,
I'll put it in.

Thanks,
Eric

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Illia Ostapyshyn]

Hi Eric,

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> As we can see from the previous bug report, no one seems to understand
> how this works! Though the punchline probably is: you're the only one
> still using S/MIME.

My workplace recommends using S/MIME and provides certificates, but I
haven't seen it used in the wild otherwise.  I would prefer OpenPGP though.

> Anyway, I'm feeling okay about this. If you think this is ready to go,
> I'll put it in.

I am satisfied with the patch and would be happy to have it installed.
I did my copyright assignment in May 2023.

Thanks,
Illia

bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL

[Eric Abrahamsen]

Illia Ostapyshyn <illia@yshyn.com> writes:

> Hi Eric,
>
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> As we can see from the previous bug report, no one seems to understand
>> how this works! Though the punchline probably is: you're the only one
>> still using S/MIME.
>
> My workplace recommends using S/MIME and provides certificates, but I
> haven't seen it used in the wild otherwise.  I would prefer OpenPGP though.
>
>> Anyway, I'm feeling okay about this. If you think this is ready to go,
>> I'll put it in.
>
> I am satisfied with the patch and would be happy to have it installed.
> I did my copyright assignment in May 2023.

Just applied. Thanks very much.

Eric