[PATCH GnuPG] Disallow compressed signatures and certificates

Demi Marie Obenour demiobenour at gmail.com
Mon Oct 10 02:12:49 CEST 2022


Compressed packets have significant attack surface, due to the potential
for both denial of service (zip bombs and the like) and for code
execution via memory corruption vulnerabilities in the decompressor.
Furthermore, I am not aware of any implementation that uses them in keys
or detached signatures.  Therefore, disallow their use in such contexts
entirely.  This includes signatures that are part of a cleartext-signed
message.

When parsing detached signatures, forbid any packet that is not a
signature or marker packet.  When parsing keys, return an error when
encountering a compressed packet, instead of decompressing the packet.
When parsing a cleartext-signed message, the signature (and any data
that follows it) is treated as a detached signature.

Furthermore, certificates, keys, and signatures are not allowed to
contain partial-length or indeterminate-length packets.  Reject those in
parse_packet, rather than activating the partial-length filter code.

GnuPG-bug-id: T5993
Signed-off-by: Demi Marie Obenour <demiobenour at gmail.com>
---
 g10/import.c       | 18 ++----------------
 g10/mainproc.c     | 34 ++++++++++++++++++++++++++++++++--
 g10/packet.h       |  2 ++
 g10/parse-packet.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 79 insertions(+), 19 deletions(-)

diff --git a/g10/import.c b/g10/import.c
index bb0bf67934a8316130cde182cd43d56353e0171d..a8136351f6f7dae8c65634ed8e1c242d323e2009 100644
--- a/g10/import.c
+++ b/g10/import.c
@@ -1042,22 +1042,8 @@ read_block( IOBUF a, unsigned int options,
       switch (pkt->pkttype)
         {
         case PKT_COMPRESSED:
-          if (check_compress_algo (pkt->pkt.compressed->algorithm))
-            {
-              rc = GPG_ERR_COMPR_ALGO;
-              goto ready;
-            }
-          else
-            {
-              compress_filter_context_t *cfx = xmalloc_clear( sizeof *cfx );
-              pkt->pkt.compressed->buf = NULL;
-              if (push_compress_filter2 (a, cfx,
-                                         pkt->pkt.compressed->algorithm, 1))
-                xfree (cfx); /* e.g. in case of compression_algo NONE.  */
-            }
-          free_packet (pkt, &parsectx);
-          init_packet(pkt);
-          break;
+          rc = GPG_ERR_UNEXPECTED;
+          goto ready;
 
         case PKT_RING_TRUST:
           /* Skip those packets unless we are in restore mode.  */
diff --git a/g10/mainproc.c b/g10/mainproc.c
index f8f3c15bccf5e1507f1c4929004d56f70508c8ce..ccdbe57e7c846bad7dacabeab7d744c15989795c 100644
--- a/g10/mainproc.c
+++ b/g10/mainproc.c
@@ -105,6 +105,7 @@ struct mainproc_context
                                      has been seen. */
     unsigned int data:1;          /* Any data packet seen */
     unsigned int uncompress_failed:1;
+    unsigned int in_cleartext:1; /* In cleartext of cleartext signature */
   } any;
 };
 
@@ -170,6 +171,7 @@ add_onepass_sig (CTX c, PACKET *pkt)
 {
   kbnode_t node;
 
+  log_assert(!(c->sigs_only && c->signed_data.used));
   if (c->list) /* Add another packet. */
     add_kbnode (c->list, new_kbnode (pkt));
   else /* Insert the first one.  */
@@ -187,6 +189,7 @@ add_gpg_control (CTX c, PACKET *pkt)
       /* New clear text signature.
        * Process the last one and reset everything */
       release_list(c);
+      c->any.in_cleartext = 1;
     }
 
   if (c->list)  /* Add another packet.  */
@@ -1118,8 +1121,16 @@ proc_compressed (CTX c, PACKET *pkt)
   int rc;
 
   /*printf("zip: compressed data packet\n");*/
-  if (c->sigs_only)
-    rc = handle_compressed (c->ctrl, c, zd, proc_compressed_cb, c);
+  if ( literals_seen )
+    {
+      log_error ("Compressed packet follows literal data packet\n");
+      rc = gpg_error (GPG_ERR_BAD_DATA);
+    }
+  else if ( c->sigs_only )
+    {
+      log_assert(!c->signed_data.used);
+      rc = handle_compressed (c->ctrl, c, zd, proc_compressed_cb, c);
+    }
   else if( c->encrypt_only )
     rc = handle_compressed (c->ctrl, c, zd, proc_encrypt_cb, c);
   else
@@ -1638,6 +1649,7 @@ do_proc_packets (CTX c, iobuf_t a)
   c->iobuf = a;
   init_packet(pkt);
   init_parse_packet (&parsectx, a);
+  parsectx.sigs_only = c->sigs_only && c->signed_data.used;
   while ((rc=parse_packet (&parsectx, pkt)) != -1)
     {
       any_data = 1;
@@ -1649,9 +1661,24 @@ do_proc_packets (CTX c, iobuf_t a)
           if (gpg_err_code (rc) == GPG_ERR_INV_PACKET
               && opt.list_packets == 0)
             break;
+
+          if (gpg_err_code (rc) == GPG_ERR_UNEXPECTED)
+            {
+              write_status_text( STATUS_UNEXPECTED, "0" );
+              goto leave;
+            }
           continue;
 	}
       newpkt = -1;
+      if (c->any.in_cleartext)
+        {
+          /* Just finished a clear-text signature's plaintext */
+          if (pkt->pkttype != PKT_PLAINTEXT)
+            log_bug("Armor parser produced packet type %d where %d expected",
+                    pkt->pkttype, PKT_PLAINTEXT);
+          c->any.in_cleartext = 0;
+          parsectx.sigs_only = 1;
+        }
       if (opt.list_packets)
         {
           switch (pkt->pkttype)
@@ -1667,6 +1694,9 @@ do_proc_packets (CTX c, iobuf_t a)
 	}
       else if (c->sigs_only)
         {
+          log_assert(pkt->pkttype == PKT_SIGNATURE ||
+                     pkt->pkttype == PKT_MARKER ||
+                     !c->signed_data.used);
           switch (pkt->pkttype)
             {
             case PKT_PUBLIC_KEY:
diff --git a/g10/packet.h b/g10/packet.h
index eeea9b450facad03188d4b87485b8fb25398f48d..6de0685b61e0098cc92f8ae0e98d897614a736fe 100644
--- a/g10/packet.h
+++ b/g10/packet.h
@@ -660,6 +660,7 @@ struct parse_packet_ctx_s
   int free_last_pkt; /* Indicates that LAST_PKT must be freed.  */
   int skip_meta;     /* Skip ring trust packets.  */
   unsigned int n_parsed_packets;	/* Number of parsed packets.  */
+  int sigs_only;     /* Only accept detached signature packets */
 };
 typedef struct parse_packet_ctx_s *parse_packet_ctx_t;
 
@@ -670,6 +671,7 @@ typedef struct parse_packet_ctx_s *parse_packet_ctx_t;
     (a)->free_last_pkt = 0;         \
     (a)->skip_meta = 0;             \
     (a)->n_parsed_packets = 0;      \
+    (a)->sigs_only = 0;             \
   } while (0)
 
 #define deinit_parse_packet(a) do { \
diff --git a/g10/parse-packet.c b/g10/parse-packet.c
index b6aebbb693f4ab8dfbb6b3d8345726cc7f69dbb6..44b7edbddad6c75bdbe885c50a2a178030b76c95 100644
--- a/g10/parse-packet.c
+++ b/g10/parse-packet.c
@@ -738,6 +738,20 @@ parse (parse_packet_ctx_t ctx, PACKET *pkt, int onlykeypkts, off_t * retpos,
             case PKT_ENCRYPTED_MDC:
             case PKT_ENCRYPTED_AEAD:
             case PKT_COMPRESSED:
+              if (ctx->sigs_only)
+                {
+                  log_error (_("partial length packet of type %d in detached"
+                               " signature\n"), pkttype);
+                  rc = gpg_error (GPG_ERR_INV_PACKET);
+                  goto leave;
+                }
+              if (onlykeypkts)
+                {
+                  log_error (_("partial length packet of type %d in keyring\n"),
+                             pkttype);
+                  rc = gpg_error (GPG_ERR_INV_PACKET);
+                  goto leave;
+                }
               iobuf_set_partial_body_length_mode (inp, c & 0xff);
               pktlen = 0;	/* To indicate partial length.  */
               partial = 1;
@@ -775,6 +789,20 @@ parse (parse_packet_ctx_t ctx, PACKET *pkt, int onlykeypkts, off_t * retpos,
 	      rc = gpg_error (GPG_ERR_INV_PACKET);
 	      goto leave;
 	    }
+	  if (ctx->sigs_only)
+	    {
+	      log_error (_("indeterminate length packet of type %d in detached"
+                           " signature\n"), pkttype);
+	      rc = gpg_error (GPG_ERR_INV_PACKET);
+	      goto leave;
+	    }
+	  if (onlykeypkts)
+	    {
+	      log_error (_("indeterminate length packet of type %d in"
+                           " keyring\n"), pkttype);
+	      rc = gpg_error (GPG_ERR_INV_PACKET);
+	      goto leave;
+	    }
 	}
       else
 	{
@@ -828,7 +856,21 @@ parse (parse_packet_ctx_t ctx, PACKET *pkt, int onlykeypkts, off_t * retpos,
       goto leave;
     }
 
-  if (with_uid && pkttype == PKT_USER_ID)
+  if (ctx->sigs_only)
+    switch (pkttype)
+      {
+      case PKT_SIGNATURE:
+      case PKT_MARKER:
+	break;
+      default:
+        log_error(_("Packet type %d not allowed in detached signature\n"),
+                  pkttype);
+	iobuf_skip_rest (inp, pktlen, partial);
+	*skip = 1;
+	rc = gpg_error (GPG_ERR_BAD_DATA);
+	goto leave;
+      }
+  else if (with_uid && pkttype == PKT_USER_ID)
     /* If ONLYKEYPKTS is set to 2, then we never skip user id packets,
        even if DO_SKIP is set.  */
     ;
-- 
2.38.0




More information about the Gnupg-devel mailing list