BEGIN_ENCRYPTION status output happens later in 2.4.1 (breaks Emacs's EasyPG)
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	fgunbin
	May 5 2023, 3:37 PM

Description

Hello,

Emacs's epg.el uses command like this:

/opt/local/bin/gpg2 --no-tty --status-fd 1 --yes --enable-progress-filter --command-fd 0 --output ~/tmp/1.txt --pinentry-mode loopback --encrypt -r 1F41483804A67C2C

With 2.4.0:

[GNUPG:] KEY_CONSIDERED F5246A427219554078FD87291F41483804A67C2C 0
[GNUPG:] PROGRESS stdin ? 0 0 B
[GNUPG:] BEGIN_ENCRYPTION 2 9
asdf
^D
[GNUPG:] PROGRESS stdin ? 5 0 B
[GNUPG:] END_ENCRYPTION

But in 2.4.1:

[GNUPG:] KEY_CONSIDERED F5246A427219554078FD87291F41483804A67C2C 0
asdf
^D
[GNUPG:] PROGRESS stdin ? 0 0 B
[GNUPG:] BEGIN_ENCRYPTION 2 9
[GNUPG:] PROGRESS stdin ? 5 0 B
[GNUPG:] END_ENCRYPTION

epg.el checks BEGIN_ENCRYPTION in the output before sending input data (when data is a string, not a file).

I can see that using --command-fd 0 is not ideal: we're mixing commands with data input. OTOH I currently can't see a good way to fix it in Emacs. What would you recommend? Should it be considered a bug solely in Emacs, or can be reverted in GnuPG?

Thanks.

Details

External Link: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=63256
Version: 2.4.1

Revisions and Commits

rG GnuPG
	rG960877b10f42 gpg: Report BEGIN_* status before examining the input.
	rG2f872fa68c65 gpg: Report BEGIN_* status before examining the input.

Related Objects

Mentioned In: T7189: Release GnuPG 2.5.0
T6578: Release GnuPG 2.4.4
Mentioned Here: rG2f872fa68c65: gpg: Report BEGIN_* status before examining the input.
rG60963d98cfd8: gpg: Detect already compressed data also when using a pipe.
T6332: GPG: Extend / rework "is_file_compressed"

Event Timeline

fgunbin created this task.May 5 2023, 3:37 PM

I have not yet experienced that although I am using Gnus with encrypted mail all the time. My guess is that this is due to the improved compressed input detection in gpg. You might be able to work around it by adding compress-level 0 to gpg.conf

I'll check next week for the exact reason. In the long term I wish that we could have gpgme support in Emacs - either by using a plugin or the new FFI. It will be quite some work to do this but at least for me it has the advantage that I test all pieces of my own dog food (i.e. gpgme).

stardiviner added a subscriber: stardiviner.May 7 2023, 5:05 PM

@werner I tested by switch back to GnuPG 2.4.1 (I downgraded to 2.4.0 before to temporary work around issue), adding compress-level 0 to gpg.conf file. It's not working. The problem still exist.

The change rG60963d98cfd8: gpg: Detect already compressed data also when using a pipe. for T6332 introduce IOBUF_IOCTL_PEEK.

It seems that it blocks (on read) at file_filter when IOBUF_IOCTL_PEEK.

Well okay, then I have no workaround. However, I won't consider this a bug because BEGIN_ENCRYPTION marks the start of the actual encryption process but not when it starts to read input data.

I wonder why we have this wait anyway:

;; `gpgsm' does not read passphrase from stdin, so waiting is not needed.
(unless (eq (epg-context-protocol context) 'CMS)
  (epg-wait-for-status context
                       (if sign '("BEGIN_SIGNING") '("BEGIN_ENCRYPTION"))))

epg-context-set-passphrase-callback even remarks:
The callback may not be called if you use GnuPG 2.x, which relies
on the external program called `gpg-agent' for passphrase query.
If you really want to intercept passphrase query, consider

So I would suggest to remove the wait. I am actually looking at gpg.el 1.0.0 from 2018, though.

@werner We could make the wait conditional on (equal epg-gpg-program "gpg"), that is, only when user has GnuPG 1.x.

But, I can see that with epg-pinentry-mode 'loopback, the passphrase is of course queried, for example when decrypting:

/opt/local/bin/gpg2 --no-tty --status-fd 1 --yes --enable-progress-filter --command-fd 0 --output /var/folders/yb/0cdss5q52d1848_2th2kp_740000gn/T/epg-outputZlOqx8 --pinentry-mode loopback --decrypt -- /Users/fgunbin/rhome/etc/authinfo.gpg
[GNUPG:] PROGRESS /Users/fgunbin/rhome ? 0 899 B
[GNUPG:] ENC_TO 37F513F5369D3007 1 0
[GNUPG:] KEY_CONSIDERED F5246A427219554078FD87291F41483804A67C2C 0
[GNUPG:] KEY_CONSIDERED F5246A427219554078FD87291F41483804A67C2C 0
[GNUPG:] USERID_HINT 37F513F5369D3007 Filipp Gunbin <fgunbin@fastmail.fm>
[GNUPG:] NEED_PASSPHRASE 37F513F5369D3007 1F41483804A67C2C 1 0
[GNUPG:] INQUIRE_MAXLEN 100
[GNUPG:] GET_HIDDEN passphrase.enter
[GNUPG:] GOT_IT
[GNUPG:] KEY_CONSIDERED F5246A427219554078FD87291F41483804A67C2C 0
[GNUPG:] DECRYPTION_KEY 9D028F52B1FF862C768ABFFC37F513F5369D3007 F5246A427219554078FD87291F41483804A67C2C u
[GNUPG:] BEGIN_DECRYPTION
[GNUPG:] DECRYPTION_INFO 2 9 0
[GNUPG:] PROGRESS /Users/fgunbin/rhome ? 899 899 B
[GNUPG:] PLAINTEXT 62 1683304850
[GNUPG:] DECRYPTION_OKAY
[GNUPG:] GOODMDC
[GNUPG:] END_DECRYPTION

So it seems that we _should_ wait for when gpg is really asking for data input.
Could we (you) perhaps introduce some other status for that, better than BEGIN_ENCRYPTION? I don't have other ideas currently :-(

Regarding the use of GPGME in Emacs, that would of course be great to have. However, I think we would like to keep the default of calling gpg via cli - to keep the libraries we require at a minimum. Perhaps GPGME could be integrated as an Emacs dynamic module.

Thanks.

amutiso added a subscriber: amutiso.May 22 2023, 4:45 PM

I use epg.el with the change of removing the wait:

diff --git a/lisp/epg.el b/lisp/epg.el
index 9da5a36ba3d..aeefad83f6c 100644
--- a/lisp/epg.el
+++ b/lisp/epg.el
@@ -1443,10 +1443,7 @@ If you are unsure, use synchronous version of this function
     (error "Not a file"))
   (setf (epg-context-operation context) 'decrypt)
   (setf (epg-context-result context) nil)
-  (epg--start context (list "--decrypt" "--" (epg-data-file cipher)))
-  ;; `gpgsm' does not read passphrase from stdin, so waiting is not needed.
-  (unless (eq (epg-context-protocol context) 'CMS)
-    (epg-wait-for-status context '("BEGIN_DECRYPTION"))))
+  (epg--start context (list "--decrypt" "--" (epg-data-file cipher))))
 
 (defun epg--check-error-for-decrypt (context)
   (let ((errors (epg-context-result-for context 'error)))
@@ -1642,9 +1639,6 @@ If you are unsure, use synchronous version of this function
 		      (epg-context-sig-notations context))
 		     (if (epg-data-file plain)
 			 (list "--" (epg-data-file plain)))))
-  ;; `gpgsm' does not read passphrase from stdin, so waiting is not needed.
-  (unless (eq (epg-context-protocol context) 'CMS)
-    (epg-wait-for-status context '("BEGIN_SIGNING")))
   (when (epg-data-string plain)
     (if (eq (process-status (epg-context-process context)) 'run)
 	(process-send-string (epg-context-process context)
@@ -1751,10 +1745,6 @@ If you are unsure, use synchronous version of this function
 			     recipients))
 		     (if (epg-data-file plain)
 			 (list "--" (epg-data-file plain)))))
-  ;; `gpgsm' does not read passphrase from stdin, so waiting is not needed.
-  (unless (eq (epg-context-protocol context) 'CMS)
-    (epg-wait-for-status context
-                         (if sign '("BEGIN_SIGNING") '("BEGIN_ENCRYPTION"))))
   (when (epg-data-string plain)
     (if (eq (process-status (epg-context-process context)) 'run)
 	(process-send-string (epg-context-process context)

It works well for my use cases. Please try.

Some points for discussion:
(1) Waiting for BEGIN_ENCRYPTION is not needed when it's for public-key encryption, because there is no change for pinentry to ask passphrase.
(2) I think that current epg.el implementation assumes GnuPG 2.1 or later anyway, since there is no support of --pinentry-mode in GnuPG 1.4 and 2.0.
(3) the changed epg.el may fail when:
(3-1) input is string and it's for symmetric encryption
(3-2) input is string and it's for decryption

I think that it those cases can be handled in egp.el to use a file with make-temp-file (some code is available within the file already).

OK, here is my changes which always use make-temp-file (to avoid confusion between data input and passphrase input).

0001-lisp-epg.el-Don-t-use-fd-0-but-a-file-for-input.patch7 KBDownload

Orthogonally, here is possible change for GnuPG, if we need to support the workaround of compress-level 0 in ~/.gnupg/gpg.conf.

diff --git a/common/iobuf.c b/common/iobuf.c
index 62cde27f9..373f25ced 100644
--- a/common/iobuf.c
+++ b/common/iobuf.c
@@ -3057,3 +3057,123 @@ iobuf_skip_rest (iobuf_t a, unsigned long n, int partial)
 	}
     }
 }
+
+
+/* Check whether (BUF,LEN) is valid header for an OpenPGP compressed
+ * packet.  LEN should be at least 6.  */
+static int
+is_openpgp_compressed_packet (const unsigned char *buf, size_t len)
+{
+  int c, ctb, pkttype;
+  int lenbytes;
+
+  ctb = *buf++; len--;
+  if (!(ctb & 0x80))
+    return 0; /* Invalid packet.  */
+
+  if ((ctb & 0x40)) /* New style (OpenPGP) CTB.  */
+    {
+      pkttype = (ctb & 0x3f);
+      if (!len)
+        return 0; /* Expected first length octet missing.  */
+      c = *buf++; len--;
+      if (c < 192)
+        ;
+      else if (c < 224)
+        {
+          if (!len)
+            return 0; /* Expected second length octet missing. */
+        }
+      else if (c == 255)
+        {
+          if (len < 4)
+            return 0; /* Expected length octets missing */
+        }
+    }
+  else /* Old style CTB.  */
+    {
+      pkttype = (ctb>>2)&0xf;
+      lenbytes = ((ctb&3)==3)? 0 : (1<<(ctb & 3));
+      if (len < lenbytes)
+        return 0; /* Not enough length bytes.  */
+    }
+
+  return (pkttype == 8);
+}
+
+
+/*
+ * Check if the file is compressed.  You need to pass the first bytes
+ * of the file as (BUF,BUFLEN).  Returns true if the buffer seems to
+ * be compressed.
+ */
+int
+is_file_compressed (iobuf_t inp)
+{
+  int i;
+  char buf[32];
+  int buflen;
+
+  struct magic_compress_s
+  {
+    byte len;
+    byte extchk;
+    byte magic[5];
+  } magic[] =
+      {
+       { 3, 0, { 0x42, 0x5a, 0x68, 0x00 } }, /* bzip2 */
+       { 3, 0, { 0x1f, 0x8b, 0x08, 0x00 } }, /* gzip */
+       { 4, 0, { 0x50, 0x4b, 0x03, 0x04 } }, /* (pk)zip */
+       { 5, 0, { '%', 'P', 'D', 'F', '-'} }, /* PDF */
+       { 4, 1, { 0xff, 0xd8, 0xff, 0xe0 } }, /* Maybe JFIF */
+       { 5, 2, { 0x89, 'P','N','G', 0x0d} }  /* Likely PNG */
+  };
+
+  if (!inp)
+    return 0;
+
+  for ( ; inp->chain; inp = inp->chain )
+    ;
+
+  buflen = iobuf_ioctl (inp, IOBUF_IOCTL_PEEK, sizeof buf, buf);
+  if (buflen < 0)
+    {
+      buflen = 0;
+      log_debug ("peeking at input failed\n");
+    }
+
+  if ( buflen < 6 )
+    {
+      return 0;  /* Too short to check - assume uncompressed.  */
+    }
+
+  for ( i = 0; i < DIM (magic); i++ )
+    {
+      if (!memcmp( buf, magic[i].magic, magic[i].len))
+        {
+          switch (magic[i].extchk)
+            {
+            case 0:
+              return 1; /* Is compressed.  */
+            case 1:
+              if (buflen > 11 && !memcmp (buf + 6, "JFIF", 5))
+                return 1; /* JFIF: this likely a compressed JPEG.  */
+              break;
+            case 2:
+              if (buflen > 8
+                  && buf[5] == 0x0a && buf[6] == 0x1a && buf[7] == 0x0a)
+                return 1; /* This is a PNG.  */
+              break;
+            default:
+              break;
+            }
+        }
+    }
+
+  if (buflen >= 6 && is_openpgp_compressed_packet (buf, buflen))
+    {
+      return 1; /* Already compressed.  */
+    }
+
+  return 0;  /* Not detected as compressed.  */
+}
diff --git a/common/iobuf.h b/common/iobuf.h
index c132c2f3c..3545ed6fb 100644
--- a/common/iobuf.h
+++ b/common/iobuf.h
@@ -629,6 +629,8 @@ void iobuf_set_partial_body_length_mode (iobuf_t a, size_t len);
    from the following filter (which may or may not return EOF).  */
 void iobuf_skip_rest (iobuf_t a, unsigned long n, int partial);
 
+int is_file_compressed (iobuf_t inp);
+
 #define iobuf_where(a)	"[don't know]"
 
 /* Each time a filter is allocated (via iobuf_alloc()), a
diff --git a/common/miscellaneous.c b/common/miscellaneous.c
index f19cc539d..1a090b1f5 100644
--- a/common/miscellaneous.c
+++ b/common/miscellaneous.c
@@ -415,112 +415,6 @@ decode_c_string (const char *src)
 }
 
 
-/* Check whether (BUF,LEN) is valid header for an OpenPGP compressed
- * packet.  LEN should be at least 6.  */
-static int
-is_openpgp_compressed_packet (const unsigned char *buf, size_t len)
-{
-  int c, ctb, pkttype;
-  int lenbytes;
-
-  ctb = *buf++; len--;
-  if (!(ctb & 0x80))
-    return 0; /* Invalid packet.  */
-
-  if ((ctb & 0x40)) /* New style (OpenPGP) CTB.  */
-    {
-      pkttype = (ctb & 0x3f);
-      if (!len)
-        return 0; /* Expected first length octet missing.  */
-      c = *buf++; len--;
-      if (c < 192)
-        ;
-      else if (c < 224)
-        {
-          if (!len)
-            return 0; /* Expected second length octet missing. */
-        }
-      else if (c == 255)
-        {
-          if (len < 4)
-            return 0; /* Expected length octets missing */
-        }
-    }
-  else /* Old style CTB.  */
-    {
-      pkttype = (ctb>>2)&0xf;
-      lenbytes = ((ctb&3)==3)? 0 : (1<<(ctb & 3));
-      if (len < lenbytes)
-        return 0; /* Not enough length bytes.  */
-    }
-
-  return (pkttype == 8);
-}
-
-
-
-/*
- * Check if the file is compressed.  You need to pass the first bytes
- * of the file as (BUF,BUFLEN).  Returns true if the buffer seems to
- * be compressed.
- */
-int
-is_file_compressed (const byte *buf, unsigned int buflen)
-{
-  int i;
-
-  struct magic_compress_s
-  {
-    byte len;
-    byte extchk;
-    byte magic[5];
-  } magic[] =
-      {
-       { 3, 0, { 0x42, 0x5a, 0x68, 0x00 } }, /* bzip2 */
-       { 3, 0, { 0x1f, 0x8b, 0x08, 0x00 } }, /* gzip */
-       { 4, 0, { 0x50, 0x4b, 0x03, 0x04 } }, /* (pk)zip */
-       { 5, 0, { '%', 'P', 'D', 'F', '-'} }, /* PDF */
-       { 4, 1, { 0xff, 0xd8, 0xff, 0xe0 } }, /* Maybe JFIF */
-       { 5, 2, { 0x89, 'P','N','G', 0x0d} }  /* Likely PNG */
-  };
-
-  if ( buflen < 6 )
-    {
-      return 0;  /* Too short to check - assume uncompressed.  */
-    }
-
-  for ( i = 0; i < DIM (magic); i++ )
-    {
-      if (!memcmp( buf, magic[i].magic, magic[i].len))
-        {
-          switch (magic[i].extchk)
-            {
-            case 0:
-              return 1; /* Is compressed.  */
-            case 1:
-              if (buflen > 11 && !memcmp (buf + 6, "JFIF", 5))
-                return 1; /* JFIF: this likely a compressed JPEG.  */
-              break;
-            case 2:
-              if (buflen > 8
-                  && buf[5] == 0x0a && buf[6] == 0x1a && buf[7] == 0x0a)
-                return 1; /* This is a PNG.  */
-              break;
-            default:
-              break;
-            }
-        }
-    }
-
-  if (buflen >= 6 && is_openpgp_compressed_packet (buf, buflen))
-    {
-      return 1; /* Already compressed.  */
-    }
-
-  return 0;  /* Not detected as compressed.  */
-}
-
-
 /* Try match against each substring of multistr, delimited by | */
 int
 match_multistr (const char *multistr,const char *match)
diff --git a/common/util.h b/common/util.h
index aa24e39e6..6b948510e 100644
--- a/common/util.h
+++ b/common/util.h
@@ -360,8 +360,6 @@ char *try_make_printable_string (const void *p, size_t n, int delim);
 char *make_printable_string (const void *p, size_t n, int delim);
 char *decode_c_string (const char *src);
 
-int is_file_compressed (const byte *buf, unsigned int buflen);
-
 int match_multistr (const char *multistr,const char *match);
 
 int gnupg_compare_version (const char *a, const char *b);
diff --git a/g10/encrypt.c b/g10/encrypt.c
index 687b4344e..624098faf 100644
--- a/g10/encrypt.c
+++ b/g10/encrypt.c
@@ -410,8 +410,6 @@ encrypt_simple (const char *filename, int mode, int use_seskey)
   text_filter_context_t tfx;
   progress_filter_context_t *pfx;
   int do_compress = !!default_compress_algo();
-  char peekbuf[32];
-  int  peekbuflen;
 
   if (!gnupg_rng_is_compliant (opt.compliance))
     {
@@ -448,14 +446,6 @@ encrypt_simple (const char *filename, int mode, int use_seskey)
       return rc;
     }
 
-  peekbuflen = iobuf_ioctl (inp, IOBUF_IOCTL_PEEK, sizeof peekbuf, peekbuf);
-  if (peekbuflen < 0)
-    {
-      peekbuflen = 0;
-      if (DBG_FILTER)
-        log_debug ("peeking at input failed\n");
-    }
-
   handle_progress (pfx, inp, filename);
 
   if (opt.textmode)
@@ -520,12 +510,14 @@ encrypt_simple (const char *filename, int mode, int use_seskey)
   if (do_compress
       && cfx.dek
       && (cfx.dek->use_mdc || cfx.dek->use_aead)
-      && !opt.explicit_compress_option
-      && is_file_compressed (peekbuf, peekbuflen))
+      && !opt.explicit_compress_option)
     {
-      if (opt.verbose)
-        log_info(_("'%s' already compressed\n"), filename? filename: "[stdin]");
-      do_compress = 0;
+      if (is_file_compressed (inp))
+	{
+	  if (opt.verbose)
+	    log_info(_("'%s' already compressed\n"), filename? filename: "[stdin]");
+	  do_compress = 0;
+	}
     }
 
   if ( rc || (rc = open_outfile (-1, filename, opt.armor? 1:0, 0, &out )))
@@ -792,8 +784,6 @@ encrypt_crypt (ctrl_t ctrl, int filefd, const char *filename,
   progress_filter_context_t *pfx;
   PK_LIST pk_list;
   int do_compress;
-  char peekbuf[32];
-  int  peekbuflen;
 
   if (filefd != -1 && filename)
     return gpg_error (GPG_ERR_INV_ARG);  /* Both given.  */
@@ -866,14 +856,6 @@ encrypt_crypt (ctrl_t ctrl, int filefd, const char *filename,
   if (opt.verbose)
     log_info (_("reading from '%s'\n"), iobuf_get_fname_nonnull (inp));
 
-  peekbuflen = iobuf_ioctl (inp, IOBUF_IOCTL_PEEK, sizeof peekbuf, peekbuf);
-  if (peekbuflen < 0)
-    {
-      peekbuflen = 0;
-      if (DBG_FILTER)
-        log_debug ("peeking at input failed\n");
-    }
-
   handle_progress (pfx, inp, filename);
 
   if (opt.textmode)
@@ -906,12 +888,14 @@ encrypt_crypt (ctrl_t ctrl, int filefd, const char *filename,
    * ciphertext attacks. */
   if (do_compress
       && (cfx.dek->use_mdc || cfx.dek->use_aead)
-      && !opt.explicit_compress_option
-      && is_file_compressed (peekbuf, peekbuflen))
+      && !opt.explicit_compress_option)
     {
-      if (opt.verbose)
-        log_info(_("'%s' already compressed\n"), filename? filename: "[stdin]");
-      do_compress = 0;
+      if (is_file_compressed (inp))
+	{
+	  if (opt.verbose)
+	    log_info(_("'%s' already compressed\n"), filename? filename: "[stdin]");
+	  do_compress = 0;
+	}
     }
   if (rc2)
     {
diff --git a/g10/sign.c b/g10/sign.c
index b5e9d422d..fcb1bb749 100644
--- a/g10/sign.c
+++ b/g10/sign.c
@@ -1035,9 +1035,6 @@ sign_file (ctrl_t ctrl, strlist_t filenames, int detached, strlist_t locusr,
   int multifile = 0;
   u32 duration=0;
   pt_extra_hash_data_t extrahash = NULL;
-  char peekbuf[32];
-  int  peekbuflen = 0;
-
 
   pfx = new_progress_context ();
   afx = new_armor_context ();
@@ -1096,14 +1093,6 @@ sign_file (ctrl_t ctrl, strlist_t filenames, int detached, strlist_t locusr,
           goto leave;
 	}
 
-      peekbuflen = iobuf_ioctl (inp, IOBUF_IOCTL_PEEK, sizeof peekbuf, peekbuf);
-      if (peekbuflen < 0)
-        {
-          peekbuflen = 0;
-          if (DBG_FILTER)
-            log_debug ("peeking at input failed\n");
-        }
-
       handle_progress (pfx, inp, fname);
     }
 
@@ -1261,7 +1250,7 @@ sign_file (ctrl_t ctrl, strlist_t filenames, int detached, strlist_t locusr,
       int compr_algo = opt.compress_algo;
 
       if (!opt.explicit_compress_option
-          && is_file_compressed (peekbuf, peekbuflen))
+          && is_file_compressed (inp))
         {
           if (opt.verbose)
             log_info(_("'%s' already compressed\n"), fname? fname: "[stdin]");

Hmm, for the latter this:

diff --git a/g10/encrypt.c b/g10/encrypt.c
index 687b4344e..c9a871aa2 100644
--- a/g10/encrypt.c
+++ b/g10/encrypt.c
@@ -448,12 +448,17 @@ encrypt_simple (const char *filename, int mode, int use_seskey)
       return rc;
     }
 
-  peekbuflen = iobuf_ioctl (inp, IOBUF_IOCTL_PEEK, sizeof peekbuf, peekbuf);
-  if (peekbuflen < 0)
+  if (opt.explicit_compress_option && !opt.compress_level)
+    peekbuflen = 0;
+  else
     {
-      peekbuflen = 0;
-      if (DBG_FILTER)
-        log_debug ("peeking at input failed\n");
+      peekbuflen = iobuf_ioctl (inp, IOBUF_IOCTL_PEEK, sizeof peekbuf, peekbuf);
+      if (peekbuflen < 0)
+        {
+          peekbuflen = 0;
+          if (DBG_FILTER)
+            log_debug ("peeking at input failed\n");
+        }
     }
 
   handle_progress (pfx, inp, filename);

looks simpler to me. Needs to be added to sign.c too, though.

looks simpler to me.

Supporting compress-level 0, then, my intention is going further to recover same behavior of status report.
I believe it may be done if it's OK to introduce is_file_compressed with IOBUF.

• gniibe added a commit: rG2f872fa68c65: gpg: Report BEGIN_* status before examining the input..May 24 2023, 3:48 AM

I pushed the change which keeps old status report behavior to master.
Let me test the change.

simonmichael added a subscriber: simonmichael.Jun 3 2023, 7:17 PM

thesamesam added a subscriber: thesamesam.Jun 6 2023, 10:03 PM

ulm added a subscriber: ulm.Jun 9 2023, 2:17 PM

Is there a commit we could backport downstream to 2.4.x? We've had quite a few reports of this.

Ah, I see https://dev.gnupg.org/rG2f872fa68c6576724b9dabee9fb0844266f55d0d applies cleanly. I guess can go with that, although would prefer it if on the 2.4 branch.

To summarize, here is the situation:

Ideally, it would be good to modify GnuPG and Emacs EasyPG to implement status handling and input handling in better way.

(1) I pasted a patch for EasyPG in this page which uses a file for data from Emacs to GnuPG, to avoid mixture of data input and passphrase input from GnuPG, putting no assumption to GnuPG behavior which of data/passphrase comes first. --- possible solution #1

(2) Possible drawback for #1: There might be a situation where use of a file for data is considered not-good (say, worry about adding an attack vector).

(3) The best (in future) is: allowing mixture of data input and passphrase input in a single stream, but no hidden assumption to GnuPG behavior.
- To do that correctly, both of GnuPG and Emacs EasyPG need to be fixed; GnuPG should emit notification to distinguish asking passphrase and asking data somehow more explicitly in status output, so that Emacs EasyPG can handle that.

It is also possible for GnuPG to keep the behavior of 2.4.0, so that it doesn't confuse EasyPG.
- This is a possible solution #2: change in master: rG2f872fa68c65: gpg: Report BEGIN_* status before examining the input.
- But... it is difficult for implementation of GnuPG to guarantee this kind of behavior.

For a while, distributions can apply rG2f872fa68c65 for 2.4 series.

I consider the entire idea of receiving a passphrase and data on the same channel to be a bad for security and robust coding. The whole thing is a historical oddity which we kept for the sake of mutt(1)'s legacy way of invoking pgp. Thus I won't consider 3) the best option.

DamienCassou added a subscriber: DamienCassou.Aug 2 2023, 2:13 PM

zuza added a subscriber: zuza.Aug 11 2023, 3:48 PM

We have a fix for now and thus I lower the priority. Given that EasyPG mimics the GPGME API we should here also use another pipe to convey the passphrase (e.g. for symmetric encryption).

@werner: What do you mean by "We have a fix for now"? Are you referring to @gniibe's patch?

ack

wolfish17 added a subscriber: wolfish17.Oct 16 2023, 2:26 PM

• gniibe added a commit: rG960877b10f42: gpg: Report BEGIN_* status before examining the input..Nov 17 2023, 12:42 AM

Applied to 2.4, too.

pgpbpadilla added a subscriber: pgpbpadilla.Dec 8 2023, 10:28 PM

meliache added a subscriber: meliache.Dec 12 2023, 3:53 PM

ra1nb0w added a subscriber: ra1nb0w.Dec 21 2023, 9:32 AM

In T6481#171399, @gniibe wrote:

It is also possible for GnuPG to keep the behavior of 2.4.0, so that it doesn't confuse EasyPG.

This is a possible solution #2: change in master: rG2f872fa68c65: gpg: Report BEGIN_* status before examining the input.

But... it is difficult for implementation of GnuPG to guarantee this kind of behavior.

For a while, distributions can apply rG2f872fa68c65 for 2.4 series.

Although commit rG2f872fa68c65: gpg: Report BEGIN_* status before examining the input. was applied in the 2.4.3 branch i still observe the same behavior:

$ gpg2  --no-tty --status-fd 1 --yes --enable-progress-filter --command-fd 0 --output /tmp/epg-outputO5MhpE --encrypt -r A3D9562A589874AB
[GNUPG:] KEY_CONSIDERED 355BDB97ED4724E6B3A450E7A3D9562A589874AB 0
<WAITING_FOR_INPUT>

i still observe the same behavior:

What do you mean? I can't replicate the behavior described by you, using the GnuPG from the repo, or the one of Debian 2.4.3-2.

In T6481#181724, @gniibe wrote:

i still observe the same behavior:

What do you mean? I can't replicate the behavior described by you, using the GnuPG from the repo, or the one of Debian 2.4.3-2.

Emacs's EasyPG still hangs (waiting for BEGIN_ENCRYPTION).

I have tested the following environments:

Arch Linux: gnupg 2.4.3-2
FreeBSD Stable 14.0: gnupg-2.4.3_4

simonmichael removed a subscriber: simonmichael.Jan 22 2024, 7:57 PM

Works as expected on openSUSE Tumbleweed with gpg2-2.4.3-4.2.x86_64:

$ gpg2 --version
gpg (GnuPG) 2.4.3
libgcrypt 1.10.3
[...]

$ gpg2  --no-tty --status-fd 1 --yes --enable-progress-filter --command-fd 0 --output /tmp/epg-outputO5MhpE --encrypt -r B81CE112B26A8EA8BE7B95D2E375339BF4C51840
[GNUPG:] KEY_CONSIDERED B81CE112B26A8EA8BE7B95D2E375339BF4C51840 0
[GNUPG:] PROGRESS stdin ? 0 0 B
[GNUPG:] BEGIN_ENCRYPTION 2 9
^C
gpg: signal Interrupt caught ... exiting

Does the problem also occur with an empty ~/.gnupg, i.e. without any configuration files and just with the imported key A3D9562A589874AB?

Arch Linux: https://gitlab.archlinux.org/archlinux/packaging/packages/gnupg
FreeBSD: https://cgit.freebsd.org/ports/tree/security/gnupg

I don't see the patch is applied. Please wait for GnuPG release 2.4.4.

Indeed, openSUSE has applied the patch: https://build.opensuse.org/package/show/openSUSE%3AFactory/gpg2

In T6481#181779, @gniibe wrote:

Arch Linux: https://gitlab.archlinux.org/archlinux/packaging/packages/gnupg
FreeBSD: https://cgit.freebsd.org/ports/tree/security/gnupg

I don't see the patch is applied. Please wait for GnuPG release 2.4.4.

Sorry for the confusion I have caused. Indeed rG2f872fa68c65: gpg: Report BEGIN_* status before examining the input. is part of the STABLE-BRANCH-2-4 branch but not part of gnupg-2.4.3 release.

• werner mentioned this in T6578: Release GnuPG 2.4.4.Jan 25 2024, 11:37 AM

Fixed in GnuPG 2.4.4.

Thanks @gniibe and everybody!

I upgraded to gnupg 1.4.4 now, the problem is gone. Thanks for working.

Any chance this could also be fixed in the 2.2.x series, where it seems to have been introduced in 2.2.42?

Sorry, no. The change is too large to back port it w/o risking a regression. As mentioned in T6481#170366 I don't consider this a bug. We are anyway working towards version 2.6 which will be the next LTS version.

Are you saying that concern about "risking a regression" is the reason to not fix this bug, which is itself a regression, and was introduced into the a point release in the current "long term support" branch?

This is not a bug. We changed it as a convenience for some Emacs users.

fwiw, i've just shipped a patch to correct this change in behavior in the 2.2 branch debian. Many thanks to @gniibe , on whose work in the 2.4 branch this is based, and to @ametzler1, who did the backporting to 2.2. I've also written a test which tries to tickle this bug. It fails with unpatched 2.2.43 as emacs times out signing and encrypting mail as epg.el deadlocks with gpg.

I would be happy if this patch was applied to the 2.2 branch upstream as well.

• werner mentioned this in T7189: Release GnuPG 2.5.0.Jul 5 2024, 2:42 PM