GNU bug report logs - #67931
[PATCH] Use S/MIME key from content for mail signing via OpenSSL

Previous Next

Package: emacs;

Reported by: Illia Ostapyshyn <illia <at> yshyn.com>

Date: Wed, 20 Dec 2023 13:59:01 UTC

Severity: normal

Tags: patch

Done: Eric Abrahamsen <eric <at> ericabrahamsen.net>

Bug is archived. No further changes may be made.

Full log


View this message in rfc822 format

From: Illia Ostapyshyn <illia <at> yshyn.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 17780 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>, Illia Ostapyshyn <illia <at> yshyn.com>, 67931 <at> debbugs.gnu.org, Jan Beich <jbeich <at> vfemail.net>
Subject: bug#67931: [PATCH] Use S/MIME key from content for mail signing via OpenSSL
Date: Mon, 06 May 2024 20:43:44 +0200
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 <at> localhost with

(progn
  (setq mml-smime-use 'openssl
     	smime-keys '(("foo <at> localhost" "foo.pem" ("chain1foo.pem" "chain2foo.pem"))
                     ("bar <at> localhost" "bar.pem" ("chain1bar.pem" "chain2bar.pem"))
                     ("baz <at> localhost" "baz.pem" ("chain1baz.pem" "chain2baz.pem"))))
  (debug-on-entry #'smime-sign-buffer)
  (compose-mail "test <at> example.org" "#67931 reproducer" '((from . "bar <at> 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 <at> 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 <at> localhost, not bar <at> 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




This bug report was last modified 1 year and 100 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.