gpg: Remove the extra prompt for Curve25519.
[gnupg.git] / g10 / parse-packet.c
index e0370aa..3407848 100644 (file)
@@ -2,6 +2,7 @@
  * Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006,
  *               2007, 2009, 2010 Free Software Foundation, Inc.
  * Copyright (C) 2014 Werner Koch
+ * Copyright (C) 2015 g10 Code GmbH
  *
  * This file is part of GnuPG.
  *
 #include "options.h"
 #include "main.h"
 #include "i18n.h"
+#include "host2net.h"
+
+
+/* Maximum length of packets to avoid excessive memory allocation.  */
+#define MAX_KEY_PACKET_LENGTH     (256 * 1024)
+#define MAX_UID_PACKET_LENGTH     (  2 * 1024)
+#define MAX_COMMENT_PACKET_LENGTH ( 64 * 1024)
+#define MAX_ATTR_PACKET_LENGTH    ( 16 * 1024*1024)
+
 
 static int mpi_print_mode;
 static int list_mode;
@@ -78,21 +88,23 @@ static int parse_mdc (IOBUF inp, int pkttype, unsigned long pktlen,
 static int parse_gpg_control (IOBUF inp, int pkttype, unsigned long pktlen,
                              PACKET * packet, int partial);
 
+/* Read a 16-bit value in MSB order (big endian) from an iobuf.  */
 static unsigned short
 read_16 (IOBUF inp)
 {
   unsigned short a;
-  a = iobuf_get_noeof (inp) << 8;
+  a = (unsigned short)iobuf_get_noeof (inp) << 8;
   a |= iobuf_get_noeof (inp);
   return a;
 }
 
 
+/* Read a 32-bit value in MSB order (big endian) from an iobuf.  */
 static unsigned long
 read_32 (IOBUF inp)
 {
   unsigned long a;
-  a = iobuf_get_noeof (inp) << 24;
+  a = (unsigned long)iobuf_get_noeof (inp) << 24;
   a |= iobuf_get_noeof (inp) << 16;
   a |= iobuf_get_noeof (inp) << 8;
   a |= iobuf_get_noeof (inp);
@@ -100,11 +112,18 @@ read_32 (IOBUF inp)
 }
 
 
-/* Read an external representation of an mpi and return the MPI.  The
- * external format is a 16 bit unsigned value stored in network byte
- * order, giving the number of bits for the following integer. The
- * integer is stored with MSB first (left padded with zeroes to align
- * on a byte boundary).  */
+/* Read an external representation of an MPI and return the MPI.  The
+   external format is a 16-bit unsigned value stored in network byte
+   order giving the number of bits for the following integer.  The
+   integer is stored MSB first and is left padded with zero bits to
+   align on a byte boundary.
+
+   The caller must set *RET_NREAD to the maximum number of bytes to
+   read from the pipeline INP.  This function sets *RET_NREAD to be
+   the number of bytes actually read from the pipeline.
+
+   If SECURE is true, the integer is stored in secure memory
+   (allocated using gcry_xmalloc_secure).  */
 static gcry_mpi_t
 mpi_read (iobuf_t inp, unsigned int *ret_nread, int secure)
 {
@@ -141,10 +160,15 @@ mpi_read (iobuf_t inp, unsigned int *ret_nread, int secure)
   p[1] = c2;
   for (i = 0; i < nbytes; i++)
     {
-      p[i + 2] = iobuf_get (inp) & 0xff;
       if (nread == nmax)
-        goto overflow;
-      nread++;
+       goto overflow;
+
+      c = iobuf_get (inp);
+      if (c == -1)
+       goto leave;
+
+      p[i + 2] = c;
+      nread ++;
     }
 
   if (gcry_mpi_scan (&a, GCRYMPI_FMT_PGP, buf, nread, &nread))
@@ -168,28 +192,44 @@ set_packet_list_mode (int mode)
 {
   int old = list_mode;
   list_mode = mode;
-  /* FIXME(gcrypt) mpi_print_mode = DBG_MPI; */
-  /* We use stdout print only if invoked by the --list-packets command
+
+  /* We use stdout only if invoked by the --list-packets command
      but switch to stderr in all other cases.  This breaks the
      previous behaviour but that seems to be more of a bug than
      intentional.  I don't believe that any application makes use of
      this long standing annoying way of printing to stdout except when
      doing a --list-packets. If this assumption fails, it will be easy
      to add an option for the listing stream.  Note that we initialize
-     it only once; mainly because some code may switch the option
-     value later back to 1 and we want to have all output to the same
-     stream.
+     it only once; mainly because there is code which switches
+     opt.list_mode back to 1 and we want to have all output to the
+     same stream.  The MPI_PRINT_MODE will be enabled if the
+     corresponding debug flag is set or if we are in --list-packets
+     and --verbose is given.
 
      Using stderr is not actually very clean because it bypasses the
      logging code but it is a special thing anyway.  I am not sure
      whether using log_stream() would be better.  Perhaps we should
-     enable the list mdoe only with a special option. */
+     enable the list mode only with a special option. */
   if (!listfp)
-    listfp = opt.list_packets == 2 ? es_stdout : es_stderr;
+    {
+      if (opt.list_packets == 2)
+        {
+          listfp = es_stdout;
+          if (opt.verbose)
+            mpi_print_mode = 1;
+        }
+      else
+        listfp = es_stderr;
+
+      if (opt.debug && DBG_MPI_VALUE)
+        mpi_print_mode = 1;
+    }
   return old;
 }
 
 
+/* If OPT.VERBOSE is set, print a warning that the algorithm ALGO is
+   not suitable for signing and encryption.  */
 static void
 unknown_pubkey_warning (int algo)
 {
@@ -222,12 +262,6 @@ unknown_pubkey_warning (int algo)
 }
 
 
-/* Parse a packet and return it in packet structure.
- * Returns: 0 := valid packet in pkt
- *        -1 := no more packets
- *        >0 := error
- * Note: The function may return an error and a partly valid packet;
- * caller must free this packet.   */
 #ifdef DEBUG_PARSE_PACKET
 int
 dbg_parse_packet (IOBUF inp, PACKET *pkt, const char *dbg_f, int dbg_l)
@@ -238,7 +272,7 @@ dbg_parse_packet (IOBUF inp, PACKET *pkt, const char *dbg_f, int dbg_l)
     {
       rc = parse (inp, pkt, 0, NULL, &skip, NULL, 0, "parse", dbg_f, dbg_l);
     }
-  while (skip);
+  while (skip && ! rc);
   return rc;
 }
 #else /*!DEBUG_PARSE_PACKET*/
@@ -251,7 +285,7 @@ parse_packet (IOBUF inp, PACKET * pkt)
     {
       rc = parse (inp, pkt, 0, NULL, &skip, NULL, 0);
     }
-  while (skip);
+  while (skip && ! rc);
   return rc;
 }
 #endif /*!DEBUG_PARSE_PACKET*/
@@ -274,7 +308,7 @@ dbg_search_packet (IOBUF inp, PACKET * pkt, off_t * retpos, int with_uid,
        parse (inp, pkt, with_uid ? 2 : 1, retpos, &skip, NULL, 0, "search",
               dbg_f, dbg_l);
     }
-  while (skip);
+  while (skip && ! rc);
   return rc;
 }
 #else /*!DEBUG_PARSE_PACKET*/
@@ -287,7 +321,7 @@ search_packet (IOBUF inp, PACKET * pkt, off_t * retpos, int with_uid)
     {
       rc = parse (inp, pkt, with_uid ? 2 : 1, retpos, &skip, NULL, 0);
     }
-  while (skip);
+  while (skip && ! rc);
   return rc;
 }
 #endif /*!DEBUG_PARSE_PACKET*/
@@ -302,6 +336,10 @@ dbg_copy_all_packets (IOBUF inp, IOBUF out, const char *dbg_f, int dbg_l)
 {
   PACKET pkt;
   int skip, rc = 0;
+
+  if (! out)
+    log_bug ("copy_all_packets: OUT may not be NULL.\n");
+
   do
     {
       init_packet (&pkt);
@@ -317,6 +355,10 @@ copy_all_packets (IOBUF inp, IOBUF out)
 {
   PACKET pkt;
   int skip, rc = 0;
+
+  if (! out)
+    log_bug ("copy_all_packets: OUT may not be NULL.\n");
+
   do
     {
       init_packet (&pkt);
@@ -401,12 +443,33 @@ skip_some_packets (IOBUF inp, unsigned n)
 #endif /*!DEBUG_PARSE_PACKET*/
 
 
-/*
- * Parse packet.  Stores 1 at SKIP 1 if the packet should be skipped;
- * this is the case if either ONLYKEYPKTS is set and the parsed packet
- * isn't a key packet or the packet-type is 0, indicating deleted
- * stuff.  If OUT is not NULL, a special copymode is used.
- */
+/* Parse a packet and save it in *PKT.
+
+   If OUT is not NULL and the packet is valid (its type is not 0),
+   then the header, the initial length field and the packet's contents
+   are written to OUT.  In this case, the packet is not saved in *PKT.
+
+   ONLYKEYPKTS is a simple packet filter.  If ONLYKEYPKTS is set to 1,
+   then only public subkey packets, public key packets, private subkey
+   packets and private key packets are parsed.  The rest are skipped
+   (i.e., the header and the contents are read from the pipeline and
+   discarded).  If ONLYKEYPKTS is set to 2, then in addition to the
+   above 4 types of packets, user id packets are also accepted.
+
+   DO_SKIP is a more coarse grained filter.  Unless ONLYKEYPKTS is set
+   to 2 and the packet is a user id packet, all packets are skipped.
+
+   Finally, if a packet is invalid (it's type is 0), it is skipped.
+
+   If a packet is skipped and SKIP is not NULL, then *SKIP is set to
+   1.
+
+   Note: ONLYKEYPKTS and DO_SKIP are only respected if OUT is NULL,
+   i.e., the packets are not simply being copied.
+
+   If RETPOS is not NULL, then the position of INP (as returned by
+   iobuf_tell) is saved there before any data is read from INP.
+  */
 static int
 parse (IOBUF inp, PACKET * pkt, int onlykeypkts, off_t * retpos,
        int *skip, IOBUF out, int do_skip
@@ -434,6 +497,8 @@ parse (IOBUF inp, PACKET * pkt, int onlykeypkts, off_t * retpos,
   else
     pos = 0; /* (silence compiler warning) */
 
+  /* The first byte of a packet is the so-called tag.  The highest bit
+     must be set.  */
   if ((ctb = iobuf_get (inp)) == -1)
     {
       rc = -1;
@@ -441,17 +506,31 @@ parse (IOBUF inp, PACKET * pkt, int onlykeypkts, off_t * retpos,
     }
   hdrlen = 0;
   hdr[hdrlen++] = ctb;
+
   if (!(ctb & 0x80))
     {
       log_error ("%s: invalid packet (ctb=%02x)\n", iobuf_where (inp), ctb);
       rc = gpg_error (GPG_ERR_INV_PACKET);
       goto leave;
     }
+
+  /* Immediately following the header is the length.  There are two
+     formats: the old format and the new format.  If bit 6 (where the
+     least significant bit is bit 0) is set in the tag, then we are
+     dealing with a new format packet.  Otherwise, it is an old format
+     packet.  */
   pktlen = 0;
   new_ctb = !!(ctb & 0x40);
   if (new_ctb)
     {
+      /* Get the packet's type.  This is encoded in the 6 least
+        significant bits of the tag.  */
       pkttype = ctb & 0x3f;
+
+      /* Extract the packet's length.  New format packets have 4 ways
+        to encode the packet length.  The value of the first byte
+        determines the encoding and partially determines the length.
+        See section 4.2.2 of RFC 4880 for details.  */
       if ((c = iobuf_get (inp)) == -1)
        {
          log_error ("%s: 1st length byte missing\n", iobuf_where (inp));
@@ -478,16 +557,21 @@ parse (IOBUF inp, PACKET * pkt, int onlykeypkts, off_t * retpos,
         }
       else if (c == 255)
         {
-          pktlen = (hdr[hdrlen++] = iobuf_get_noeof (inp)) << 24;
-          pktlen |= (hdr[hdrlen++] = iobuf_get_noeof (inp)) << 16;
-          pktlen |= (hdr[hdrlen++] = iobuf_get_noeof (inp)) << 8;
-          if ((c = iobuf_get (inp)) == -1)
+         int i;
+         char value[4];
+
+         for (i = 0; i < 4; i ++)
             {
-              log_error ("%s: 4 byte length invalid\n", iobuf_where (inp));
-              rc = gpg_error (GPG_ERR_INV_PACKET);
-              goto leave;
+              if ((c = iobuf_get (inp)) == -1)
+                {
+                  log_error ("%s: 4 byte length invalid\n", iobuf_where (inp));
+                  rc = gpg_error (GPG_ERR_INV_PACKET);
+                  goto leave;
+                }
+              value[i] = hdr[hdrlen++] = c;
             }
-          pktlen |= (hdr[hdrlen++] = c);
+
+         pktlen = buf32_to_ulong (value);
         }
       else /* Partial body length.  */
         {
@@ -497,13 +581,13 @@ parse (IOBUF inp, PACKET * pkt, int onlykeypkts, off_t * retpos,
             case PKT_ENCRYPTED:
             case PKT_ENCRYPTED_MDC:
             case PKT_COMPRESSED:
-              iobuf_set_partial_block_mode (inp, c & 0xff);
+              iobuf_set_partial_body_length_mode (inp, c & 0xff);
               pktlen = 0;      /* To indicate partial length.  */
               partial = 1;
               break;
 
             default:
-              log_error ("%s: partial length for invalid"
+              log_error ("%s: partial length invalid for"
                          " packet type %d\n", iobuf_where (inp), pkttype);
               rc = gpg_error (GPG_ERR_INV_PACKET);
               goto leave;
@@ -512,8 +596,13 @@ parse (IOBUF inp, PACKET * pkt, int onlykeypkts, off_t * retpos,
 
     }
   else
+    /* This is an old format packet.  */
     {
+      /* Extract the packet's type.  This is encoded in bits 2-5.  */
       pkttype = (ctb >> 2) & 0xf;
+
+      /* The type of length encoding is encoded in bits 0-1 of the
+        tag.  */
       lenbytes = ((ctb & 3) == 3) ? 0 : (1 << (ctb & 3));
       if (!lenbytes)
        {
@@ -535,12 +624,26 @@ parse (IOBUF inp, PACKET * pkt, int onlykeypkts, off_t * retpos,
          for (; lenbytes; lenbytes--)
            {
              pktlen <<= 8;
-             pktlen |= hdr[hdrlen++] = iobuf_get_noeof (inp);
+             c = iobuf_get (inp);
+             if (c == -1)
+               {
+                 log_error ("%s: length invalid\n", iobuf_where (inp));
+                 rc = gpg_error (GPG_ERR_INV_PACKET);
+                 goto leave;
+               }
+             pktlen |= hdr[hdrlen++] = c;
            }
        }
     }
 
-  if (pktlen == (unsigned long) (-1))
+  /* Sometimes the decompressing layer enters an error state in which
+     it simply outputs 0xff for every byte read.  If we have a stream
+     of 0xff bytes, then it will be detected as a new format packet
+     with type 63 and a 4-byte encoded length that is 4G-1.  Since
+     packets with type 63 are private and we use them as a control
+     packet, which won't be 4 GB, we reject such packets as
+     invalid.  */
+  if (pkttype == 63 && pktlen == 0xFFFFFFFF)
     {
       /* With some probability this is caused by a problem in the
        * the uncompressing layer - in some error cases it just loops
@@ -551,6 +654,17 @@ parse (IOBUF inp, PACKET * pkt, int onlykeypkts, off_t * retpos,
 
   if (out && pkttype)
     {
+      /* This type of copying won't work if the packet uses a partial
+        body length.  (In other words, this only works if HDR is
+        actually the length.)  Currently, no callers require this
+        functionality so we just log this as an error.  */
+      if (partial)
+       {
+         log_error ("parse: Can't copy partial packet.  Aborting.\n");
+         rc = gpg_error (GPG_ERR_INV_PACKET);
+         goto leave;
+       }
+
       rc = iobuf_write (out, hdr, hdrlen);
       if (!rc)
        rc = copy_packet (inp, out, pkttype, pktlen, partial);
@@ -558,9 +672,13 @@ parse (IOBUF inp, PACKET * pkt, int onlykeypkts, off_t * retpos,
     }
 
   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.  */
     ;
   else if (do_skip
+          /* type==0 is not allowed.  This is an invalid packet.  */
           || !pkttype
+          /* When ONLYKEYPKTS is set, we don't skip keys.  */
           || (onlykeypkts && pkttype != PKT_PUBLIC_SUBKEY
               && pkttype != PKT_PUBLIC_KEY
               && pkttype != PKT_SECRET_SUBKEY && pkttype != PKT_SECRET_KEY))
@@ -587,11 +705,11 @@ parse (IOBUF inp, PACKET * pkt, int onlykeypkts, off_t * retpos,
   if (list_mode)
     es_fprintf (listfp, "# off=%lu ctb=%02x tag=%d hlen=%d plen=%lu%s%s\n",
                 (unsigned long)pos, ctb, pkttype, hdrlen, pktlen,
-                partial? " partial":"",
+                partial? (new_ctb ? " partial" : " indeterminate") :"",
                 new_ctb? " new-ctb":"");
 
   pkt->pkttype = pkttype;
-  rc = G10ERR_UNKNOWN_PACKET;  /* default error */
+  rc = GPG_ERR_UNKNOWN_PACKET; /* default error */
   switch (pkttype)
     {
     case PKT_PUBLIC_KEY:
@@ -650,14 +768,15 @@ parse (IOBUF inp, PACKET * pkt, int onlykeypkts, off_t * retpos,
       rc = parse_marker (inp, pkttype, pktlen);
       break;
     default:
+      /* Unknown packet.  Skip it.  */
       skip_packet (inp, pkttype, pktlen, partial);
       break;
     }
 
  leave:
-  /* FIXME: Do we leak in case of an error?  */
+  /* FIXME: We leak in case of an error (see the xmalloc's above).  */
   if (!rc && iobuf_error (inp))
-    rc = G10ERR_INV_KEYRING;
+    rc = GPG_ERR_INV_KEYRING;
 
   /* FIXME: We use only the error code for now to avoid problems with
      callers which have not been checked to always use gpg_err_code()
@@ -684,6 +803,20 @@ dump_hex_line (int c, int *i)
 }
 
 
+/* Copy the contents of a packet from the pipeline IN to the pipeline
+   OUT.
+
+   The header and length have already been read from INP and the
+   decoded values are given as PKGTYPE and PKTLEN.
+
+   If the packet is a partial body length packet (RFC 4880, Section
+   4.2.2.4), then iobuf_set_partial_block_mode should already have
+   been called on INP and PARTIAL should be set.
+
+   If PARTIAL is set or PKTLEN is 0 and PKTTYPE is PKT_COMPRESSED,
+   copy until the first EOF is encountered on INP.
+
+   Returns 0 on success and an error code if an error occurs.  */
 static int
 copy_packet (IOBUF inp, IOBUF out, int pkttype,
             unsigned long pktlen, int partial)
@@ -694,7 +827,7 @@ copy_packet (IOBUF inp, IOBUF out, int pkttype,
 
   if (partial)
     {
-      while ((n = iobuf_read (inp, buf, 100)) != -1)
+      while ((n = iobuf_read (inp, buf, sizeof (buf))) != -1)
        if ((rc = iobuf_write (out, buf, n)))
          return rc;            /* write error */
     }
@@ -702,7 +835,7 @@ copy_packet (IOBUF inp, IOBUF out, int pkttype,
     {
       log_debug ("copy_packet: compressed!\n");
       /* compressed packet, copy till EOF */
-      while ((n = iobuf_read (inp, buf, 100)) != -1)
+      while ((n = iobuf_read (inp, buf, sizeof (buf))) != -1)
        if ((rc = iobuf_write (out, buf, n)))
          return rc;            /* write error */
     }
@@ -710,7 +843,7 @@ copy_packet (IOBUF inp, IOBUF out, int pkttype,
     {
       for (; pktlen; pktlen -= n)
        {
-         n = pktlen > 100 ? 100 : pktlen;
+         n = pktlen > sizeof (buf) ? sizeof (buf) : pktlen;
          n = iobuf_read (inp, buf, n);
          if (n == -1)
            return gpg_error (GPG_ERR_EOF);
@@ -722,6 +855,9 @@ copy_packet (IOBUF inp, IOBUF out, int pkttype,
 }
 
 
+/* Skip an unknown packet.  PKTTYPE is the packet's type, PKTLEN is
+   the length of the packet's content and PARTIAL is whether partial
+   body length encoding in used (in this case PKTLEN is ignored).  */
 static void
 skip_packet (IOBUF inp, int pkttype, unsigned long pktlen, int partial)
 {
@@ -756,8 +892,9 @@ skip_packet (IOBUF inp, int pkttype, unsigned long pktlen, int partial)
 
 
 /* Read PKTLEN bytes form INP and return them in a newly allocated
-   buffer.  In case of an error NULL is returned and a error messages
-   printed.  */
+   buffer.  In case of an error (including reading fewer than PKTLEN
+   bytes from INP before EOF is returned), NULL is returned and an
+   error message is logged.  */
 static void *
 read_rest (IOBUF inp, size_t pktlen)
 {
@@ -878,7 +1015,7 @@ parse_marker (IOBUF inp, int pkttype, unsigned long pktlen)
   if (list_mode)
     es_fputs (":marker packet: [invalid]\n", listfp);
   iobuf_skip_rest (inp, pktlen, 0);
-  return G10ERR_INVALID_PACKET;
+  return GPG_ERR_INV_PACKET;
 }
 
 
@@ -1088,6 +1225,12 @@ parse_pubkeyenc (IOBUF inp, int pkttype, unsigned long pktlen,
 }
 
 
+/* Dump a subpacket to LISTFP.  BUFFER contains the subpacket in
+   question and points to the type field in the subpacket header (not
+   the start of the header).  TYPE is the subpacket's type with the
+   critical bit cleared.  CRITICAL is the value of the CRITICAL bit.
+   BUFLEN is the length of the buffer and LENGTH is the length of the
+   subpacket according to the subpacket's header.  */
 static void
 dump_sig_subpkt (int hashed, int type, int critical,
                 const byte * buffer, size_t buflen, size_t length)
@@ -1124,14 +1267,14 @@ dump_sig_subpkt (int hashed, int type, int critical,
     case SIGSUBPKT_SIG_CREATED:
       if (length >= 4)
        es_fprintf (listfp, "sig created %s",
-                    strtimestamp (buffer_to_u32 (buffer)));
+                    strtimestamp (buf32_to_u32 (buffer)));
       break;
     case SIGSUBPKT_SIG_EXPIRE:
       if (length >= 4)
        {
-         if (buffer_to_u32 (buffer))
+         if (buf32_to_u32 (buffer))
            es_fprintf (listfp, "sig expires after %s",
-                        strtimevalue (buffer_to_u32 (buffer)));
+                        strtimevalue (buf32_to_u32 (buffer)));
          else
            es_fprintf (listfp, "sig does not expire");
        }
@@ -1151,7 +1294,11 @@ dump_sig_subpkt (int hashed, int type, int critical,
       if (!length)
        p = "[invalid regexp subpacket]";
       else
-       es_fprintf (listfp, "regular expression: \"%s\"", buffer);
+        {
+          es_fprintf (listfp, "regular expression: \"");
+          es_write_sanitized (listfp, buffer, length, "\"", NULL);
+          p = "\"";
+        }
       break;
     case SIGSUBPKT_REVOCABLE:
       if (length)
@@ -1160,9 +1307,9 @@ dump_sig_subpkt (int hashed, int type, int critical,
     case SIGSUBPKT_KEY_EXPIRE:
       if (length >= 4)
        {
-         if (buffer_to_u32 (buffer))
+         if (buf32_to_u32 (buffer))
            es_fprintf (listfp, "key expires after %s",
-                        strtimevalue (buffer_to_u32 (buffer)));
+                        strtimevalue (buf32_to_u32 (buffer)));
          else
            es_fprintf (listfp, "key does not expire");
        }
@@ -1186,8 +1333,8 @@ dump_sig_subpkt (int hashed, int type, int critical,
     case SIGSUBPKT_ISSUER:
       if (length >= 8)
        es_fprintf (listfp, "issuer key ID %08lX%08lX",
-                    (ulong) buffer_to_u32 (buffer),
-                    (ulong) buffer_to_u32 (buffer + 4));
+                    (ulong) buf32_to_u32 (buffer),
+                    (ulong) buf32_to_u32 (buffer + 4));
       break;
     case SIGSUBPKT_NOTATION:
       {
@@ -1383,10 +1530,12 @@ can_handle_critical (const byte * buffer, size_t n, int type)
     {
     case SIGSUBPKT_NOTATION:
       if (n >= 8)
-       return can_handle_critical_notation (buffer + 8,
-                                            (buffer[4] << 8) | buffer[5]);
-      else
-       return 0;
+       {
+         size_t notation_len = ((buffer[4] << 8) | buffer[5]);
+         if (n - 8 >= notation_len)
+           return can_handle_critical_notation (buffer + 8, notation_len);
+       }
+      return 0;
     case SIGSUBPKT_SIGNATURE:
     case SIGSUBPKT_SIG_CREATED:
     case SIGSUBPKT_SIG_EXPIRE:
@@ -1447,8 +1596,7 @@ enum_sig_subpkt (const subpktarea_t * pktbuf, sigsubpkttype_t reqtype,
        {
          if (buflen < 4)
            goto too_short;
-         n = (buffer[0] << 24) | (buffer[1] << 16)
-           | (buffer[2] << 8) | buffer[3];
+         n = buf32_to_size_t (buffer);
          buffer += 4;
          buflen -= 4;
        }
@@ -1519,7 +1667,11 @@ enum_sig_subpkt (const subpktarea_t * pktbuf, sigsubpkttype_t reqtype,
       buflen -= n;
     }
   if (reqtype == SIGSUBPKT_TEST_CRITICAL)
-    return buffer;  /* Used as True to indicate that there is no. */
+    /* Returning NULL means we found a subpacket with the critical bit
+       set that we don't grok.  We've iterated over all the subpackets
+       and haven't found such a packet so we need to return a non-NULL
+       value.  */
+    return buffer;
 
   /* Critical bit we don't understand. */
   if (start)
@@ -1544,14 +1696,13 @@ parse_sig_subpkt (const subpktarea_t * buffer, sigsubpkttype_t reqtype,
 
 
 const byte *
-parse_sig_subpkt2 (PKT_signature * sig, sigsubpkttype_t reqtype,
-                  size_t * ret_n)
+parse_sig_subpkt2 (PKT_signature * sig, sigsubpkttype_t reqtype)
 {
   const byte *p;
 
-  p = parse_sig_subpkt (sig->hashed, reqtype, ret_n);
+  p = parse_sig_subpkt (sig->hashed, reqtype, NULL);
   if (!p)
-    p = parse_sig_subpkt (sig->unhashed, reqtype, ret_n);
+    p = parse_sig_subpkt (sig->unhashed, reqtype, NULL);
   return p;
 }
 
@@ -1560,25 +1711,31 @@ parse_sig_subpkt2 (PKT_signature * sig, sigsubpkttype_t reqtype,
 void
 parse_revkeys (PKT_signature * sig)
 {
-  struct revocation_key *revkey;
+  const byte *revkey;
   int seq = 0;
   size_t len;
 
   if (sig->sig_class != 0x1F)
     return;
 
-  while ((revkey =
-         (struct revocation_key *) enum_sig_subpkt (sig->hashed,
-                                                    SIGSUBPKT_REV_KEY,
-                                                    &len, &seq, NULL)))
+  while ((revkey = enum_sig_subpkt (sig->hashed, SIGSUBPKT_REV_KEY,
+                                   &len, &seq, NULL)))
     {
-      if (len == sizeof (struct revocation_key)
-          && (revkey->class & 0x80))  /* 0x80 bit must be set.  */
+      if (/* The only valid length is 22 bytes.  See RFC 4880
+            5.2.3.15.  */
+         len == 22
+         /* 0x80 bit must be set on the class.  */
+          && (revkey[0] & 0x80))
        {
          sig->revkey = xrealloc (sig->revkey,
-                                 sizeof (struct revocation_key *) *
+                                 sizeof (struct revocation_key) *
                                  (sig->numrevkeys + 1));
-         sig->revkey[sig->numrevkeys] = revkey;
+
+         /* Copy the individual fields.  */
+         sig->revkey[sig->numrevkeys].class = revkey[0];
+         sig->revkey[sig->numrevkeys].algid = revkey[1];
+         memcpy (sig->revkey[sig->numrevkeys].fpr, &revkey[2], 20);
+
          sig->numrevkeys++;
        }
     }
@@ -1618,13 +1775,19 @@ parse_signature (IOBUF inp, int pkttype, unsigned long pktlen,
 
   if (!is_v4)
     {
+      if (pktlen == 0)
+       goto underflow;
       md5_len = iobuf_get_noeof (inp);
       pktlen--;
     }
+  if (pktlen == 0)
+    goto underflow;
   sig->sig_class = iobuf_get_noeof (inp);
   pktlen--;
   if (!is_v4)
     {
+      if (pktlen < 12)
+       goto underflow;
       sig->timestamp = read_32 (inp);
       pktlen -= 4;
       sig->keyid[0] = read_32 (inp);
@@ -1632,6 +1795,8 @@ parse_signature (IOBUF inp, int pkttype, unsigned long pktlen,
       sig->keyid[1] = read_32 (inp);
       pktlen -= 4;
     }
+  if (pktlen < 2)
+    goto underflow;
   sig->pubkey_algo = iobuf_get_noeof (inp);
   pktlen--;
   sig->digest_algo = iobuf_get_noeof (inp);
@@ -1640,14 +1805,18 @@ parse_signature (IOBUF inp, int pkttype, unsigned long pktlen,
   sig->flags.revocable = 1;
   if (is_v4) /* Read subpackets.  */
     {
+      if (pktlen < 2)
+       goto underflow;
       n = read_16 (inp);
       pktlen -= 2;  /* Length of hashed data. */
+      if (pktlen < n)
+       goto underflow;
       if (n > 10000)
        {
          log_error ("signature packet: hashed data too long\n");
           if (list_mode)
             es_fputs (":signature packet: [hashed data too long]\n", listfp);
-         rc = G10ERR_INVALID_PACKET;
+         rc = GPG_ERR_INV_PACKET;
          goto leave;
        }
       if (n)
@@ -1666,14 +1835,18 @@ parse_signature (IOBUF inp, int pkttype, unsigned long pktlen,
            }
          pktlen -= n;
        }
+      if (pktlen < 2)
+       goto underflow;
       n = read_16 (inp);
       pktlen -= 2;  /* Length of unhashed data.  */
+      if (pktlen < n)
+       goto underflow;
       if (n > 10000)
        {
          log_error ("signature packet: unhashed data too long\n");
           if (list_mode)
             es_fputs (":signature packet: [unhashed data too long]\n", listfp);
-         rc = G10ERR_INVALID_PACKET;
+         rc = GPG_ERR_INV_PACKET;
          goto leave;
        }
       if (n)
@@ -1694,15 +1867,8 @@ parse_signature (IOBUF inp, int pkttype, unsigned long pktlen,
        }
     }
 
-  if (pktlen < 5)  /* Sanity check.  */
-    {
-      log_error ("packet(%d) too short\n", pkttype);
-      if (list_mode)
-        es_fputs (":signature packet: [too short]\n", listfp);
-      rc = G10ERR_INVALID_PACKET;
-      goto leave;
-    }
-
+  if (pktlen < 2)
+    goto underflow;
   sig->digest_start[0] = iobuf_get_noeof (inp);
   pktlen--;
   sig->digest_start[1] = iobuf_get_noeof (inp);
@@ -1721,24 +1887,24 @@ parse_signature (IOBUF inp, int pkttype, unsigned long pktlen,
 
       p = parse_sig_subpkt (sig->hashed, SIGSUBPKT_SIG_CREATED, NULL);
       if (p)
-       sig->timestamp = buffer_to_u32 (p);
+       sig->timestamp = buf32_to_u32 (p);
       else if (!(sig->pubkey_algo >= 100 && sig->pubkey_algo <= 110)
               && opt.verbose)
        log_info ("signature packet without timestamp\n");
 
-      p = parse_sig_subpkt2 (sig, SIGSUBPKT_ISSUER, NULL);
+      p = parse_sig_subpkt2 (sig, SIGSUBPKT_ISSUER);
       if (p)
        {
-         sig->keyid[0] = buffer_to_u32 (p);
-         sig->keyid[1] = buffer_to_u32 (p + 4);
+         sig->keyid[0] = buf32_to_u32 (p);
+         sig->keyid[1] = buf32_to_u32 (p + 4);
        }
       else if (!(sig->pubkey_algo >= 100 && sig->pubkey_algo <= 110)
               && opt.verbose)
        log_info ("signature packet without keyid\n");
 
       p = parse_sig_subpkt (sig->hashed, SIGSUBPKT_SIG_EXPIRE, NULL);
-      if (p && buffer_to_u32 (p))
-       sig->expiredate = sig->timestamp + buffer_to_u32 (p);
+      if (p && buf32_to_u32 (p))
+       sig->expiredate = sig->timestamp + buf32_to_u32 (p);
       if (sig->expiredate && sig->expiredate <= make_timestamp ())
        sig->flags.expired = 1;
 
@@ -1780,7 +1946,7 @@ parse_signature (IOBUF inp, int pkttype, unsigned long pktlen,
          unhashed area.  In theory, anyway, we should never see this
          packet off of a local keyring. */
 
-      p = parse_sig_subpkt2 (sig, SIGSUBPKT_EXPORTABLE, NULL);
+      p = parse_sig_subpkt2 (sig, SIGSUBPKT_EXPORTABLE);
       if (p && *p == 0)
        sig->flags.exportable = 0;
 
@@ -1819,7 +1985,7 @@ parse_signature (IOBUF inp, int pkttype, unsigned long pktlen,
          /* We include a limit to avoid too trivial DoS attacks by
             having gpg allocate too much memory.  */
          log_error ("signature packet: too much data\n");
-         rc = G10ERR_INVALID_PACKET;
+         rc = GPG_ERR_INV_PACKET;
        }
       else
        {
@@ -1842,13 +2008,22 @@ parse_signature (IOBUF inp, int pkttype, unsigned long pktlen,
              es_putc ('\n', listfp);
            }
          if (!sig->data[i])
-           rc = G10ERR_INVALID_PACKET;
+           rc = GPG_ERR_INV_PACKET;
        }
     }
 
  leave:
   iobuf_skip_rest (inp, pktlen, 0);
   return rc;
+
+ underflow:
+  log_error ("packet(%d) too short\n", pkttype);
+  if (list_mode)
+    es_fputs (":signature packet: [too short]\n", listfp);
+
+  iobuf_skip_rest (inp, pktlen, 0);
+
+  return GPG_ERR_INV_PACKET;
 }
 
 
@@ -1948,7 +2123,7 @@ parse_key (IOBUF inp, int pkttype, unsigned long pktlen,
   else if (version == 4)
     {
       /* The only supported version.  Use an older gpg
-         versions (i.e. gpg 1.4 to parse v3 packets).  */
+         version (i.e. gpg 1.4) to parse v3 packets.  */
     }
   else if (version == 2 || version == 3)
     {
@@ -1957,7 +2132,7 @@ parse_key (IOBUF inp, int pkttype, unsigned long pktlen,
       if (list_mode)
         es_fprintf (listfp, ":key packet: [obsolete version %d]\n", version);
       pk->version = version;
-      err = gpg_error (GPG_ERR_INV_PACKET);
+      err = gpg_error (GPG_ERR_LEGACY_KEY);
       goto leave;
     }
   else
@@ -1977,6 +2152,14 @@ parse_key (IOBUF inp, int pkttype, unsigned long pktlen,
       err = gpg_error (GPG_ERR_INV_PACKET);
       goto leave;
     }
+  else if (pktlen > MAX_KEY_PACKET_LENGTH)
+    {
+      log_error ("packet(%d) too large\n", pkttype);
+      if (list_mode)
+        es_fputs (":key packet: [too larget]\n", listfp);
+      err = gpg_error (GPG_ERR_INV_PACKET);
+      goto leave;
+    }
 
   timestamp = read_32 (inp);
   pktlen -= 4;
@@ -2050,8 +2233,8 @@ parse_key (IOBUF inp, int pkttype, unsigned long pktlen,
                    || algorithm == PUBKEY_ALGO_ECDH) && i==0)
                 {
                   char *curve = openpgp_oid_to_str (pk->pkey[0]);
-                  es_fprintf (listfp, " %s (%s)",
-                              openpgp_oid_to_curve (curve), curve);
+                  const char *name = openpgp_oid_to_curve (curve, 0);
+                  es_fprintf (listfp, " %s (%s)", name?name:"", curve);
                   xfree (curve);
                 }
               es_putc ('\n', listfp);
@@ -2067,6 +2250,12 @@ parse_key (IOBUF inp, int pkttype, unsigned long pktlen,
       byte temp[16];
       size_t snlen = 0;
 
+      if (pktlen < 1)
+        {
+          err = gpg_error (GPG_ERR_INV_PACKET);
+          goto leave;
+        }
+
       pk->seckey_info = ski = xtrycalloc (1, sizeof *ski);
       if (!pk->seckey_info)
         {
@@ -2124,6 +2313,11 @@ parse_key (IOBUF inp, int pkttype, unsigned long pktlen,
                case 3:
                  for (i = 0; i < 8 && pktlen; i++, pktlen--)
                    temp[i] = iobuf_get_noeof (inp);
+                  if (i < 8)
+                    {
+                     err = gpg_error (GPG_ERR_INV_PACKET);
+                     goto leave;
+                    }
                  memcpy (ski->s2k.salt, temp, 8);
                  break;
                }
@@ -2239,7 +2433,7 @@ parse_key (IOBUF inp, int pkttype, unsigned long pktlen,
               err = gpg_error (GPG_ERR_INV_PACKET);
              goto leave;
            }
-         for (i = 0; i < ski->ivlen && pktlen; i++, pktlen--)
+         for (i = 0; i < ski->ivlen; i++, pktlen--)
            temp[i] = iobuf_get_noeof (inp);
          if (list_mode)
            {
@@ -2267,6 +2461,12 @@ parse_key (IOBUF inp, int pkttype, unsigned long pktlen,
        }
       else if (ski->is_protected)
        {
+         if (pktlen < 2) /* At least two bytes for the length.  */
+           {
+              err = gpg_error (GPG_ERR_INV_PACKET);
+             goto leave;
+           }
+
          /* Ugly: The length is encrypted too, so we read all stuff
           * up to the end of the packet into the first SKEY
           * element.  */
@@ -2287,7 +2487,14 @@ parse_key (IOBUF inp, int pkttype, unsigned long pktlen,
           /* Not encrypted.  */
          for (i = npkey; i < nskey; i++)
            {
-              unsigned int n = pktlen;
+              unsigned int n;
+
+              if (pktlen < 2) /* At least two bytes for the length.  */
+                {
+                  err = gpg_error (GPG_ERR_INV_PACKET);
+                  goto leave;
+                }
+              n = pktlen;
               pk->pkey[i] = mpi_read (inp, &n, 0);
               pktlen -= n;
               if (list_mode)
@@ -2303,6 +2510,11 @@ parse_key (IOBUF inp, int pkttype, unsigned long pktlen,
          if (err)
            goto leave;
 
+         if (pktlen < 2)
+           {
+              err = gpg_error (GPG_ERR_INV_PACKET);
+             goto leave;
+           }
          ski->csum = read_16 (inp);
          pktlen -= 2;
          if (list_mode)
@@ -2310,6 +2522,7 @@ parse_key (IOBUF inp, int pkttype, unsigned long pktlen,
        }
     }
 
+  /* Note that KEYID below has been initialized above in list_mode.  */
   if (list_mode)
     es_fprintf (listfp, "\tkeyid: %08lX%08lX\n",
                 (ulong) keyid[0], (ulong) keyid[1]);
@@ -2343,8 +2556,7 @@ parse_attribute_subpkts (PKT_user_id * uid)
        {
          if (buflen < 4)
            goto too_short;
-         n = (buffer[0] << 24) | (buffer[1] << 16)
-           | (buffer[2] << 8) | buffer[3];
+         n = buf32_to_size_t (buffer);
          buffer += 4;
          buflen -= 4;
        }
@@ -2359,8 +2571,16 @@ parse_attribute_subpkts (PKT_user_id * uid)
       if (buflen < n)
        goto too_short;
 
-      attribs =
-       xrealloc (attribs, (count + 1) * sizeof (struct user_attribute));
+      if (!n)
+        {
+          /* Too short to encode the subpacket type.  */
+          if (opt.verbose)
+            log_info ("attribute subpacket too short\n");
+          break;
+        }
+
+      attribs = xrealloc (attribs,
+                          (count + 1) * sizeof (struct user_attribute));
       memset (&attribs[count], 0, sizeof (struct user_attribute));
 
       type = *buffer;
@@ -2401,13 +2621,13 @@ parse_user_id (IOBUF inp, int pkttype, unsigned long pktlen, PACKET * packet)
      allocatable, and a very large pktlen could actually cause our
      allocation to wrap around in xmalloc to a small number. */
 
-  if (pktlen > 2048)
+  if (pktlen > MAX_UID_PACKET_LENGTH)
     {
       log_error ("packet(%d) too large\n", pkttype);
       if (list_mode)
         es_fprintf (listfp, ":user ID packet: [too large]\n");
       iobuf_skip_rest (inp, pktlen, 0);
-      return G10ERR_INVALID_PACKET;
+      return GPG_ERR_INV_PACKET;
     }
 
   packet->pkt.user_id = xmalloc_clear (sizeof *packet->pkt.user_id + pktlen);
@@ -2482,13 +2702,13 @@ parse_attribute (IOBUF inp, int pkttype, unsigned long pktlen,
   /* We better cap the size of an attribute packet to make DoS not too
      easy.  16MB should be more then enough for one attribute packet
      (ie. a photo).  */
-  if (pktlen > 16*1024*1024)
+  if (pktlen > MAX_ATTR_PACKET_LENGTH)
     {
       log_error ("packet(%d) too large\n", pkttype);
       if (list_mode)
         es_fprintf (listfp, ":attribute packet: [too large]\n");
       iobuf_skip_rest (inp, pktlen, 0);
-      return G10ERR_INVALID_PACKET;
+      return GPG_ERR_INV_PACKET;
     }
 
 #define EXTRA_UID_NAME_SPACE 71
@@ -2526,14 +2746,14 @@ parse_comment (IOBUF inp, int pkttype, unsigned long pktlen, PACKET * packet)
      overflow in the malloc below.  Comment packets are actually not
      anymore define my OpenPGP and we even stopped to use our
      private comment packet.  */
-  if (pktlen > 65536)
+  if (pktlen > MAX_COMMENT_PACKET_LENGTH)
     {
       log_error ("packet(%d) too large\n", pkttype);
       if (list_mode)
         es_fprintf (listfp, ":%scomment packet: [too large]\n",
                     pkttype == PKT_OLD_COMMENT ? "OpenPGP draft " : "");
       iobuf_skip_rest (inp, pktlen, 0);
-      return G10ERR_INVALID_PACKET;
+      return GPG_ERR_INV_PACKET;
     }
   packet->pkt.comment = xmalloc (sizeof *packet->pkt.comment + pktlen - 1);
   packet->pkt.comment->len = pktlen;
@@ -2566,11 +2786,11 @@ parse_trust (IOBUF inp, int pkttype, unsigned long pktlen, PACKET * pkt)
 
   (void) pkttype;
 
+  pkt->pkt.ring_trust = xmalloc (sizeof *pkt->pkt.ring_trust);
   if (pktlen)
     {
       c = iobuf_get_noeof (inp);
       pktlen--;
-      pkt->pkt.ring_trust = xmalloc (sizeof *pkt->pkt.ring_trust);
       pkt->pkt.ring_trust->trustval = c;
       pkt->pkt.ring_trust->sigcache = 0;
       if (!c && pktlen == 1)
@@ -2589,6 +2809,8 @@ parse_trust (IOBUF inp, int pkttype, unsigned long pktlen, PACKET * pkt)
     }
   else
     {
+      pkt->pkt.ring_trust->trustval = 0;
+      pkt->pkt.ring_trust->sigcache = 0;
       if (list_mode)
        es_fprintf (listfp, ":trust packet: empty\n");
     }
@@ -2645,7 +2867,6 @@ parse_plaintext (IOBUF inp, int pkttype, unsigned long pktlen,
     pktlen -= 4;
   pt->len = pktlen;
   pt->buf = inp;
-  pktlen = 0;
 
   if (list_mode)
     {
@@ -2741,7 +2962,7 @@ parse_encrypted (IOBUF inp, int pkttype, unsigned long pktlen,
       log_error ("packet(%d) too short\n", pkttype);
       if (list_mode)
         es_fputs (":encrypted data packet: [too short]\n", listfp);
-      rc = G10ERR_INVALID_PACKET;
+      rc = GPG_ERR_INV_PACKET;
       iobuf_skip_rest (inp, pktlen, partial);
       goto leave;
     }
@@ -2803,7 +3024,7 @@ parse_mdc (IOBUF inp, int pkttype, unsigned long pktlen,
 /*
  * This packet is internally generated by us (ibn armor.c) to transfer
  * some information to the lower layer.  To make sure that this packet
- * is really a GPG faked one and not one comming from outside, we
+ * is really a GPG faked one and not one coming from outside, we
  * first check that there is a unique tag in it.
  *
  * The format of such a control packet is:
@@ -2837,7 +3058,7 @@ parse_gpg_control (IOBUF inp, int pkttype, unsigned long pktlen,
     goto skipit;  /* Definitely too large.  We skip it to avoid an
                      overflow in the malloc.  */
   if (list_mode)
-    puts ("- gpg control packet");
+    es_fputs ("- gpg control packet", listfp);
 
   packet->pkt.gpg_control = xmalloc (sizeof *packet->pkt.gpg_control
                                     + pktlen - 1);