gpg: Fix actual leak and possible leaks in the packet parser.
authorWerner Koch <wk@gnupg.org>
Thu, 30 Mar 2017 14:01:52 +0000 (16:01 +0200)
committerWerner Koch <wk@gnupg.org>
Thu, 30 Mar 2017 14:01:52 +0000 (16:01 +0200)
* g10/packet.h (struct parse_packet_ctx_s): Change LAST_PKT deom a
pointer to its struct.
(init_parse_packet): Adjust for LAST_PKT not being a pointer.
* g10/parse-packet.c (parse): Ditto. Free the last packet before
storing a new one in case of a deep link.
(parse_ring_trust): Adjust for LAST_PKT not being a pointer.
* g10/free-packet.c (free_packet): Ditto.
* g10/t-keydb-get-keyblock.c (do_test): Release keyblock.
--

Fixes-commit: afa86809087909a8ba2f9356588bf90cc923529c
Signed-off-by: Werner Koch <wk@gnupg.org>
g10/build-packet.c
g10/free-packet.c
g10/packet.h
g10/parse-packet.c
g10/t-keydb-get-keyblock.c

index 1ee57e0..fa2674b 100644 (file)
@@ -420,7 +420,7 @@ do_user_id( IOBUF out, int ctb, PKT_user_id *uid )
   log_assert (ctb_pkttype (ctb) == PKT_USER_ID
               || ctb_pkttype (ctb) == PKT_ATTRIBUTE);
 
   log_assert (ctb_pkttype (ctb) == PKT_USER_ID
               || ctb_pkttype (ctb) == PKT_ATTRIBUTE);
 
-  /* We need to take special care that doe user ID with a length of 0:
+  /* We need to take special care of a user ID with a length of 0:
    * Without forcing HDRLEN to 2 in this case an indeterminate length
    * packet would be written which is not allowed.  Note that we are
    * always called with a CTB indicating an old packet header format,
    * Without forcing HDRLEN to 2 in this case an indeterminate length
    * packet would be written which is not allowed.  Note that we are
    * always called with a CTB indicating an old packet header format,
index c144246..cd222a2 100644 (file)
@@ -409,14 +409,15 @@ free_packet (PACKET *pkt, parse_packet_ctx_t parsectx)
 {
   if (!pkt || !pkt->pkt.generic)
     {
 {
   if (!pkt || !pkt->pkt.generic)
     {
-      if (parsectx && parsectx->last_pkt)
+      if (parsectx && parsectx->last_pkt.pkt.generic)
         {
           if (parsectx->free_last_pkt)
             {
         {
           if (parsectx->free_last_pkt)
             {
-              free_packet (parsectx->last_pkt, NULL);
+              free_packet (&parsectx->last_pkt, NULL);
               parsectx->free_last_pkt = 0;
             }
               parsectx->free_last_pkt = 0;
             }
-          parsectx->last_pkt = NULL;
+          parsectx->last_pkt.pkttype = 0;
+          parsectx->last_pkt.pkt.generic = NULL;
         }
       return;
     }
         }
       return;
     }
@@ -427,8 +428,11 @@ free_packet (PACKET *pkt, parse_packet_ctx_t parsectx)
   /* If we have a parser context holding PKT then do not free the
    * packet but set a flag that the packet in the parser context is
    * now a deep copy.  */
   /* If we have a parser context holding PKT then do not free the
    * packet but set a flag that the packet in the parser context is
    * now a deep copy.  */
-  if (parsectx && parsectx->last_pkt == pkt && !parsectx->free_last_pkt)
+  if (parsectx && !parsectx->free_last_pkt
+      && parsectx->last_pkt.pkttype == pkt->pkttype
+      && parsectx->last_pkt.pkt.generic == pkt->pkt.generic)
     {
     {
+      parsectx->last_pkt = *pkt;
       parsectx->free_last_pkt = 1;
       pkt->pkt.generic = NULL;
       return;
       parsectx->free_last_pkt = 1;
       pkt->pkt.generic = NULL;
       return;
index b23298a..f5f22b6 100644 (file)
@@ -621,7 +621,7 @@ int set_packet_list_mode( int mode );
 struct parse_packet_ctx_s
 {
   iobuf_t inp;       /* The input stream with the packets.  */
 struct parse_packet_ctx_s
 {
   iobuf_t inp;       /* The input stream with the packets.  */
-  PACKET *last_pkt;  /* The last parsed packet.  */
+  struct packet_struct last_pkt; /* The last parsed packet.  */
   int free_last_pkt; /* Indicates that LAST_PKT must be freed.  */
   int skip_meta;     /* Skip right trust packets.  */
 };
   int free_last_pkt; /* Indicates that LAST_PKT must be freed.  */
   int skip_meta;     /* Skip right trust packets.  */
 };
@@ -629,7 +629,8 @@ typedef struct parse_packet_ctx_s *parse_packet_ctx_t;
 
 #define init_parse_packet(a,i) do { \
     (a)->inp = (i);                 \
 
 #define init_parse_packet(a,i) do { \
     (a)->inp = (i);                 \
-    (a)->last_pkt = NULL;           \
+    (a)->last_pkt.pkttype = 0;      \
+    (a)->last_pkt.pkt.generic= NULL;\
     (a)->free_last_pkt = 0;         \
     (a)->skip_meta = 0;             \
   } while (0)
     (a)->free_last_pkt = 0;         \
     (a)->skip_meta = 0;             \
   } while (0)
index df04fbc..793e198 100644 (file)
@@ -833,14 +833,15 @@ parse (parse_packet_ctx_t ctx, PACKET *pkt, int onlykeypkts, off_t * retpos,
     }
 
   /* Store a shallow copy of certain packets in the context.  */
     }
 
   /* Store a shallow copy of certain packets in the context.  */
+  free_packet (NULL, ctx);
   if (!rc && (pkttype == PKT_PUBLIC_KEY
               || pkttype == PKT_SECRET_KEY
               || pkttype == PKT_USER_ID
               || pkttype == PKT_ATTRIBUTE
               || pkttype == PKT_SIGNATURE))
   if (!rc && (pkttype == PKT_PUBLIC_KEY
               || pkttype == PKT_SECRET_KEY
               || pkttype == PKT_USER_ID
               || pkttype == PKT_ATTRIBUTE
               || pkttype == PKT_SIGNATURE))
-    ctx->last_pkt = pkt;
-  else
-    ctx->last_pkt = NULL;
+    {
+      ctx->last_pkt = *pkt;
+    }
 
  leave:
   /* FIXME: We leak in case of an error (see the xmalloc's above).  */
 
  leave:
   /* FIXME: We leak in case of an error (see the xmalloc's above).  */
@@ -2992,12 +2993,12 @@ parse_ring_trust (parse_packet_ctx_t ctx, unsigned long pktlen)
 
   /* Now transfer the data to the respective packet.  Do not do this
    * if SKIP_META is set.  */
 
   /* Now transfer the data to the respective packet.  Do not do this
    * if SKIP_META is set.  */
-  if (!ctx->last_pkt || ctx->skip_meta)
+  if (!ctx->last_pkt.pkt.generic || ctx->skip_meta)
     ;
   else if (rt.subtype == RING_TRUST_SIG
     ;
   else if (rt.subtype == RING_TRUST_SIG
-           && ctx->last_pkt->pkttype == PKT_SIGNATURE)
+           && ctx->last_pkt.pkttype == PKT_SIGNATURE)
     {
     {
-      PKT_signature *sig = ctx->last_pkt->pkt.signature;
+      PKT_signature *sig = ctx->last_pkt.pkt.signature;
 
       if ((rt.sigcache & 1))
         {
 
       if ((rt.sigcache & 1))
         {
@@ -3006,10 +3007,10 @@ parse_ring_trust (parse_packet_ctx_t ctx, unsigned long pktlen)
         }
     }
   else if (rt.subtype == RING_TRUST_UID
         }
     }
   else if (rt.subtype == RING_TRUST_UID
-           && (ctx->last_pkt->pkttype == PKT_USER_ID
-               || ctx->last_pkt->pkttype == PKT_ATTRIBUTE))
+           && (ctx->last_pkt.pkttype == PKT_USER_ID
+               || ctx->last_pkt.pkttype == PKT_ATTRIBUTE))
     {
     {
-      PKT_user_id *uid = ctx->last_pkt->pkt.user_id;
+      PKT_user_id *uid = ctx->last_pkt.pkt.user_id;
 
       uid->keysrc = rt.keysrc;
       uid->keyupdate = rt.keyupdate;
 
       uid->keysrc = rt.keysrc;
       uid->keyupdate = rt.keyupdate;
@@ -3017,10 +3018,10 @@ parse_ring_trust (parse_packet_ctx_t ctx, unsigned long pktlen)
       rt.url = NULL;
     }
   else if (rt.subtype == RING_TRUST_KEY
       rt.url = NULL;
     }
   else if (rt.subtype == RING_TRUST_KEY
-           && (ctx->last_pkt->pkttype == PKT_PUBLIC_KEY
-               || ctx->last_pkt->pkttype == PKT_SECRET_KEY))
+           && (ctx->last_pkt.pkttype == PKT_PUBLIC_KEY
+               || ctx->last_pkt.pkttype == PKT_SECRET_KEY))
     {
     {
-      PKT_public_key *pk = ctx->last_pkt->pkt.public_key;
+      PKT_public_key *pk = ctx->last_pkt.pkt.public_key;
 
       pk->keysrc = rt.keysrc;
       pk->keyupdate = rt.keyupdate;
 
       pk->keysrc = rt.keysrc;
       pk->keyupdate = rt.keyupdate;
index 993d879..167a9bb 100644 (file)
@@ -61,4 +61,5 @@ do_test (int argc, char *argv[])
   TEST_P ("", ! rc);
 
   keydb_release (hd1);
   TEST_P ("", ! rc);
 
   keydb_release (hd1);
+  release_kbnode (kb1);
 }
 }