Fixed serious bug related to multiple cleartext signatures.

NEWS

@@ -3,7 +3,9 @@
       be used to verify signatures against a list of trusted keys.
 
     * Rijndael (AES) is now supported and listed as first preference.
-   
+
+    * Fixed a serious bug which could lead to false signature verification
+      results when more than one clearsigned signatures are in one file.
 
 Noteworthy changes in version 1.0.3 (2000-09-18)
 ------------------------------------------------

g10/ChangeLog

@@ -1,5 +1,16 @@
 2000-10-13  Werner Koch  <wk@gnupg.org>
 
+	* mainproc.c (add_gpg_control): New.
+	(do_proc_packets): use it.
+	(proc_plaintext): Changed logic to detect clearsigns.
+	(proc_tree): Check the cleartext sig with some new code.
+
+	* packet.h: New packet PKT_GPG_CONTROL.
+	* parse-packet.c (parse_gpg_control): New.
+	* misc.c (get_session_marker): New.
+	* armor.c (armor_filter): Replaced the faked 1-pass packet by the
+	new control packet.
+
 	* keyedit.c (keyedit_menu): Allow batchmode with a command_fd. 
 	* status.c (my_read): New.
 	(do_get_from_fd): use it.

g10/armor.c

@@ -502,7 +502,7 @@ fake_packet( armor_filter_context_t *afx, IOBUF a,
 	    /* the buffer is always allocated with enough space to append
 	     * the removed [CR], LF and a Nul
 	     * The reason for this complicated procedure is to keep at least
-	     * the original tupe of lineending - handling of the removed
+	     * the original type of lineending - handling of the removed
 	     * trailing spaces seems to be impossible in our method
 	     * of faking a packet; either we have to use a temporary file
 	     * or calculate the hash here in this module and somehow find
@@ -815,7 +815,9 @@ armor_filter( void *opaque, int control,
 	*ret_len = n;
     }
     else if( control == IOBUFCTRL_UNDERFLOW ) {
-	if( size < 15+(4*15) )	/* need space for up to 4 onepass_sigs */
+        /* We need some space for the faked packet.  The minmum required
+         * size is ~18 + length of the session marker */
+	if( size < 50 ) 
 	    BUG(); /* supplied buffer too short */
 
 	if( afx->faked )
@@ -831,7 +833,14 @@ armor_filter( void *opaque, int control,
 		    rc = -1;
 	    }
 	    else if( afx->faked ) {
-		unsigned hashes = afx->hashes;
+		unsigned int hashes = afx->hashes;
+                const byte *sesmark;
+                size_t sesmarklen;
+                
+                sesmark = get_session_marker( &sesmarklen );
+                if ( sesmarklen > 20 )
+                    BUG();
+
 		/* the buffer is at least 15+n*15 bytes long, so it
 		 * is easy to construct the packets */
 
@@ -842,36 +851,21 @@ armor_filter( void *opaque, int control,
 			afx->pgp2mode = 1;
 		}
 		n=0;
-		do {
+                /* first a gpg control packet */
-		    /* first some onepass signature packets */
+                buf[n++] = 0xff; /* new format, type 63, 1 length byte */
-		    buf[n++] = 0x90; /* old format, type 4, 1 length byte */
+                n++;   /* see below */
-		    buf[n++] = 13;   /* length */
+                memcpy(buf+n, sesmark, sesmarklen ); n+= sesmarklen;
-		    buf[n++] = 3;    /* version */
+                buf[n++] = 1;    /* control type */
-		    buf[n++] = afx->not_dash_escaped? 0:1; /* sigclass */
+                buf[n++] = afx->not_dash_escaped? 0:1; /* sigclass */
-		    if( hashes & 1 ) {
+                if( hashes & 1 ) 
-			hashes &= ~1;
+                    buf[n++] = DIGEST_ALGO_RMD160;
-			buf[n++] = DIGEST_ALGO_RMD160;
+                if( hashes & 2 ) 
-		    }
+                    buf[n++] = DIGEST_ALGO_SHA1;
-		    else if( hashes & 2 ) {
+                if( hashes & 4 ) 
-			hashes &= ~2;
+                    buf[n++] = DIGEST_ALGO_MD5;
-			buf[n++] = DIGEST_ALGO_SHA1;
+                if( hashes & 8 ) 
-		    }
+                    buf[n++] = DIGEST_ALGO_TIGER;
-		    else if( hashes & 4 ) {
+                buf[1] = n - 2;
-			hashes &= ~4;
-			buf[n++] = DIGEST_ALGO_MD5;
-		    }
-		    else if( hashes & 8 ) {
-			hashes &= ~8;
-			buf[n++] = DIGEST_ALGO_TIGER;
-		    }
-		    else
-			buf[n++] = 0;	 /* (don't know) */
-
-		    buf[n++] = 0;    /* public key algo (don't know) */
-		    memset(buf+n, 0, 8); /* don't know the keyid */
-		    n += 8;
-		    buf[n++] = !hashes;   /* last one */
-		} while( hashes );
 
 		/* followed by a plaintext packet */
 		buf[n++] = 0xaf; /* old packet format, type 11, var length */

g10/main.h

@@ -60,6 +60,7 @@ u16 checksum( byte *p, unsigned n );
 u16 checksum_mpi( MPI a );
 u16 checksum_mpi_counted_nbits( MPI a );
 u32 buffer_to_u32( const byte *buffer );
+const byte *get_session_marker( size_t *rlen );
 
 /*-- helptext.c --*/
 void display_online_help( const char *keyword );

g10/mainproc.c

@@ -119,6 +119,24 @@ add_onepass_sig( CTX c, PACKET *pkt )
 }
 
 
+static int
+add_gpg_control( CTX c, PACKET *pkt )
+{
+    if ( pkt->pkt.gpg_control->control == 1 ) {
+        /* New clear text signature.
+         * Process the last one and reset everything */
+        release_list(c);
+    }   
+
+    if( c->list )  /* add another packet */
+        add_kbnode( c->list, new_kbnode( pkt ));
+    else /* insert the first one */
+	c->list = new_kbnode( pkt );
+
+    return 1;
+}
+
+
 
 static int
 add_user_id( CTX c, PACKET *pkt )
@@ -412,20 +430,23 @@ proc_plaintext( CTX c, PACKET *pkt )
 	    }
 	    if( n->pkt->pkt.onepass_sig->sig_class != 0x01 )
 		only_md5 = 0;
-
-	    /* Check whether this is a cleartext signature.  We assume that
-	     * we have one if the sig_class is 1 and the keyid is 0, that
-	     * are the faked packets produced by armor.c.  There is a
-	     * possibility that this fails, but there is no other easy way
-	     * to do it. (We could use a special packet type to indicate
-	     * this, but this may also be faked - it simply can't be verified
-	     * and is _no_ security issue)
-	     */
-	    if( n->pkt->pkt.onepass_sig->sig_class == 0x01
-		&& !n->pkt->pkt.onepass_sig->keyid[0]
-		&& !n->pkt->pkt.onepass_sig->keyid[1] )
-		clearsig = 1;
 	}
+	else if( n->pkt->pkttype == PKT_GPG_CONTROL
+                 && n->pkt->pkt.gpg_control->control == 1 ) {
+            size_t datalen = n->pkt->pkt.gpg_control->datalen;
+            const byte *data = n->pkt->pkt.gpg_control->data;
+
+            /* check that we have at least the sigclass and one hash */
+            if ( datalen < 2 )
+                log_fatal("invalid control packet of type 1\n"); 
+            /* Note that we don't set the clearsig flag for not-dash-escaped
+             * documents */
+            clearsig = (*data == 0x01);
+            for( data++, datalen--; datalen; datalen--, data++ )
+                md_enable( c->mfx.md, *data );
+            any = 1;
+            break;  /* no pass signature pakets are expected */
+        }
     }
 
     if( !any && !opt.skip_verify ) {
@@ -993,6 +1014,7 @@ do_proc_packets( CTX c, IOBUF a )
 	      case PKT_PLAINTEXT:   proc_plaintext( c, pkt ); break;
 	      case PKT_COMPRESSED:  proc_compressed( c, pkt ); break;
 	      case PKT_ONEPASS_SIG: newpkt = add_onepass_sig( c, pkt ); break;
+              case PKT_GPG_CONTROL: newpkt = add_gpg_control(c, pkt); break;
 	      default: newpkt = 0; break;
 	    }
 	}
@@ -1011,6 +1033,7 @@ do_proc_packets( CTX c, IOBUF a )
 	      case PKT_PLAINTEXT:   proc_plaintext( c, pkt ); break;
 	      case PKT_COMPRESSED:  proc_compressed( c, pkt ); break;
 	      case PKT_ONEPASS_SIG: newpkt = add_onepass_sig( c, pkt ); break;
+	      case PKT_GPG_CONTROL: newpkt = add_gpg_control(c, pkt); break;
 	      default: newpkt = 0; break;
 	    }
 	}
@@ -1035,6 +1058,7 @@ do_proc_packets( CTX c, IOBUF a )
 	      case PKT_PLAINTEXT:   proc_plaintext( c, pkt ); break;
 	      case PKT_COMPRESSED:  proc_compressed( c, pkt ); break;
 	      case PKT_ONEPASS_SIG: newpkt = add_onepass_sig( c, pkt ); break;
+              case PKT_GPG_CONTROL: newpkt = add_gpg_control(c, pkt); break;
 	      case PKT_RING_TRUST:  newpkt = add_ring_trust( c, pkt ); break;
 	      default: newpkt = 0; break;
 	    }
@@ -1228,6 +1252,17 @@ proc_tree( CTX c, KBNODE node )
 	for( n1 = node; (n1 = find_next_kbnode(n1, PKT_SIGNATURE )); )
 	    check_sig_and_print( c, n1 );
     }
+    else if( node->pkt->pkttype == PKT_GPG_CONTROL
+             && node->pkt->pkt.gpg_control->control == 1 ) {
+        /* clear text signed message */
+	if( !c->have_data ) {
+            log_error("cleartext signature without data\n" );
+            return;
+        }
+	
+	for( n1 = node; (n1 = find_next_kbnode(n1, PKT_SIGNATURE )); )
+	    check_sig_and_print( c, n1 );
+    }
     else if( node->pkt->pkttype == PKT_SIGNATURE ) {
 	PKT_signature *sig = node->pkt->pkt.signature;
 

g10/misc.c

@@ -22,6 +22,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <unistd.h>
 #include <errno.h>
 #if defined(__linux__) && defined(__alpha__) && __GLIBC__ < 2
   #include <asm/sysinfo.h>
@@ -244,6 +245,32 @@ print_digest_algo_note( int algo )
 }
 
 
+/* Return a string which is used as a kind of process ID */
+const byte *
+get_session_marker( size_t *rlen )
+{
+    static byte marker[SIZEOF_UNSIGNED_LONG*2];
+    static int initialized;
+
+    if ( !initialized ) {
+        volatile ulong aa, bb; /* we really want the unitialized value */
+        ulong a, b;
+
+        initialized = 1;
+        /* also this marker is guessable it is not easy to use this 
+         * for a faked control packet because an attacker does not
+         * have enough control about the time the verification does 
+         * take place.  Of course, we can add just more random but 
+         * than we need the random generator even for verification
+         * tasks - which does not make sense. */
+        a = aa ^ (ulong)getpid();
+        b = bb ^ (ulong)time(NULL);
+        memcpy( marker, &a, SIZEOF_UNSIGNED_LONG );
+        memcpy( marker+SIZEOF_UNSIGNED_LONG, &b, SIZEOF_UNSIGNED_LONG );
+    }
+    *rlen = sizeof(marker);
+    return marker;
+}
 
 
 

g10/packet.h

@@ -50,6 +50,7 @@ typedef enum {
 	PKT_ENCRYPTED_MDC =18, /* integrity protected encrypted data */
 	PKT_MDC 	  =19, /* manipulaion detection code packet */
 	PKT_COMMENT	  =61, /* new comment packet (private) */
+        PKT_GPG_CONTROL   =63  /* internal control packet */
 } pkttype_t;
 
 typedef struct packet_struct PACKET;
@@ -194,6 +195,11 @@ typedef struct {
     char name[1];
 } PKT_plaintext;
 
+typedef struct {
+    int  control;
+    size_t datalen;
+    char data[1];
+} PKT_gpg_control;
 
 /* combine all packets into a union */
 struct packet_struct {
@@ -213,6 +219,7 @@ struct packet_struct {
 	PKT_mdc 	*mdc;		/* PKT_MDC */
 	PKT_ring_trust	*ring_trust;	/* PKT_RING_TRUST */
 	PKT_plaintext	*plaintext;	/* PKT_PLAINTEXT */
+        PKT_gpg_control *gpg_control;   /* PKT_GPG_CONTROL */
     } pkt;
 };
 

g10/parse-packet.c

@@ -75,6 +75,8 @@ static int  parse_encrypted( IOBUF inp, int pkttype, unsigned long pktlen,
 					       PACKET *packet, int new_ctb);
 static int  parse_mdc( IOBUF inp, int pkttype, unsigned long pktlen,
 					       PACKET *packet, int new_ctb);
+static int  parse_gpg_control( IOBUF inp, int pkttype, unsigned long pktlen,
+                               PACKET *packet );
 
 static unsigned short
 read_16(IOBUF inp)
@@ -446,6 +448,9 @@ parse( IOBUF inp, PACKET *pkt, int reqtype, ulong *retpos,
       case PKT_MDC:
 	rc = parse_mdc(inp, pkttype, pktlen, pkt, new_ctb );
 	break;
+      case PKT_GPG_CONTROL:
+        rc = parse_gpg_control(inp, pkttype, pktlen, pkt );
+        break;
       default:
 	skip_packet(inp, pkttype, pktlen);
 	break;
@@ -1783,3 +1788,66 @@ parse_mdc( IOBUF inp, int pkttype, unsigned long pktlen,
     return 0;
 }
 
+
+/*
+ * This packet is internally generated by PGG (by 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 first check that tehre is a unique tag in it.
+ * The format of such a control packet is:
+ *   n byte  session marker
+ *   1 byte  control type: 1 = Clearsign hash info
+ *   m byte  control data
+ */
+
+static int
+parse_gpg_control( IOBUF inp,
+                   int pkttype, unsigned long pktlen, PACKET *packet )
+{
+    byte *p;
+    const byte *sesmark;
+    size_t sesmarklen;
+    int i;
+
+    if ( list_mode )
+        printf(":packet 63: length %lu ",  pktlen);
+
+    sesmark = get_session_marker ( &sesmarklen );
+    if ( pktlen < sesmarklen+1 ) /* 1 is for the control bytes */
+        goto skipit;
+    for( i=0; i < sesmarklen; i++, pktlen-- ) {
+	if ( sesmark[i] != iobuf_get_noeof(inp) )
+            goto skipit;
+    }
+    if ( list_mode )
+        puts ("- gpg control packet");
+
+    packet->pkt.gpg_control = m_alloc(sizeof *packet->pkt.gpg_control
+                                      + pktlen - 1);
+    packet->pkt.gpg_control->control = iobuf_get_noeof(inp); pktlen--;
+    packet->pkt.gpg_control->datalen = pktlen;
+    p = packet->pkt.gpg_control->data;
+    for( ; pktlen; pktlen--, p++ )
+	*p = iobuf_get_noeof(inp);
+
+    return 0;
+
+ skipit:
+    if ( list_mode ) {
+        int c, i=0 ;
+        printf("- private (rest length %lu)\n",  pktlen);
+        if( iobuf_in_block_mode(inp) ) {
+            while( (c=iobuf_get(inp)) != -1 )
+                dump_hex_line(c, &i);
+        }
+        else {
+            for( ; pktlen; pktlen-- )
+                dump_hex_line(iobuf_get(inp), &i);
+        }
+        putchar('\n');
+    }
+    skip_rest(inp,pktlen);
+    return G10ERR_INVALID_PACKET;
+}
+
+