Stricter test of allowed signature packet compositions.

There is still one problem to solve.

NEWS

@@ -51,6 +51,10 @@ Noteworthy changes in version 1.4.3
       are HTTP and finger, plus anything that cURL supplies, if built
       with cURL support.
 
+    * Files containing several signed messages are not anymore allowed
+      because there is no clean way to report the status of such files
+      back to the caller.
+
 
 Noteworthy changes in version 1.4.2 (2005-07-26)
 ------------------------------------------------

checks/ChangeLog

@@ -1,3 +1,13 @@
+2006-03-06  Werner Koch  <wk@g10code.com>
+
+	* defs.inc: Print error messages also to stderr.  Allow for
+	verbose environment variable.
+	(linefeed): New.
+	(suspend_error, resume_error): New.
+	* verify.test:  More tests.
+	* multisig.test: Better error printing.
+	(sig_1ls1ls_valid, sig_ls_valid):  Moved to the non-valid group.
+	
 2006-02-14  Werner Koch  <wk@gnupg.org>
 
 	* verify.test: New.

checks/defs.inc

@@ -31,22 +31,52 @@ LANGUAGE=
 LC_ALL=
 LC_MESSAGES=
 
+# Internal use.
+defs_stop_on_error=no
+defs_error_seen=no
+
 #--------------------------------
 #------ utility functions -------
 #--------------------------------
 
 fatal () {
     echo "$pgmname: fatal:" $* >&2
+    [ -n "${BASH_VERSION+set}" ] && echo "$pgmname: fatal:" $* >&5
     exit 1;
 }
 
 error () {
     echo "$pgmname:" $* >&2
+    defs_error_seen=yes
+    [ -n "${BASH_VERSION+set}" ] && echo "$pgmname:" $* >&5
+    if [ x$defs_stop_on_error != xyes ]; then
       exit 1 
+    fi
+}
+
+# Call this at the start of a test and resume_error at the end to keep
+# on running all subtests without immediately exiting on error.
+suspend_error () {
+   defs_stop_on_error=yes
+}
+
+resume_error () {
+    if [ x$defs_error_seen = xyes ]; then
+        exit 1
+    fi
+    defs_stop_on_error=no
+    defs_error_seen=no
 }
 
 info () {
     echo "$pgmname:" $* >&2
+    if [ -n "${verbose+set}" ]; then
+      [ -n "${BASH_VERSION+set}" ] && echo "$pgmname:" $* >&5
+    fi
+}
+
+linefeed () {
+    echo >&2
 }
 
 
@@ -126,6 +156,8 @@ fi
 
 GPG="../g10/gpg --no-permission-warning --homedir . "
 
+[ -n "${BASH_VERSION+set}" ] && exec 5>/dev/stderr
+
 exec 2> ${pgmname}.log
 
 :

checks/multisig.test

@@ -2,12 +2,14 @@
 # Check that gpg verifies only signatures where there is no ambiguity
 # in the order of packets.  Needs the Demo Keys Lima and Mike.
 
+# Note: We do son't support multiple signaturess anymore thus thsi test is 
+# not really needed becuase verify could do the same.  We keep it anyway.
+
 . $srcdir/defs.inc || exit 3
 
-# (variable intialization was created using:
+suspend_error
-#   for i in files; do echo "`echo $i | sed 's,[.-],_,g'`='"; \
+
-#   gpg --no-version --enarmor <$i | grep -v ^Comment:; echo "'" ; done 
+
-# )
 
 sig_1ls1ls_valid='
 -----BEGIN PGP ARMORED FILE-----
@@ -119,13 +121,11 @@ cnksIEkgY2FuJ3QgZG8gdGhhdAo=
 -----END PGP ARMORED FILE-----
 '
 
-save_IFS="${IFS}"
+
-IFS=""
+for i in  sig_sl_valid ; do
-for i in "$sig_1ls1ls_valid" "$sig_ls_valid" "$sig_sl_valid"; do
+    eval "(IFS=; echo \"\$$i\")" | ./gpg_dearmor >x
-    echo "$i" | ./gpg_dearmor >x
+    $GPG --verify x 2>/dev/null || error "valid is invalid ($i)"
-    IFS="${save_IFS}"
+    linefeed
-    $GPG --verify x 2>/dev/null || error "valid is invalid"
-    IFS=""
 done
 #for i in "$sig_11lss_valid_but_is_not" "$sig_11lss11lss_valid_but_is_not" \
 #         "$sig_ssl_valid_but_is_not"; do
@@ -133,13 +133,13 @@ done
 #    $GPG --verify <x 2>/dev/null || error "valid is invalid"
 #done
 
-# without the +e ksh seems to terminate the for loop
+for i in sig_1ls1ls_valid sig_ls_valid \
-set +e
+         sig_1lsls_invalid sig_lsls_invalid \
-for i in "$sig_1lsls_invalid" "$sig_lsls_invalid" \
+         sig_lss_invalid sig_slsl_invalid ; do
-         "$sig_lss_invalid" "$sig_slsl_invalid" ; do
+    eval "(IFS=; echo \"\$$i\")" | ./gpg_dearmor >x
-    echo "$i" | ./gpg_dearmor >x
+    $GPG --verify <x 2>/dev/null && error "invalid is valid ($i)"
-    IFS="${save_IFS}"
+    linefeed
-    $GPG --verify <x 2>/dev/null && error "invalid is valid"
-    IFS=""
 done
-IFS="${save_IFS}"
+
+
+resume_error

checks/verify.test

@@ -2,10 +2,126 @@
 
 . $srcdir/defs.inc || exit 3
 
-#info check that verify fails for bad input data
+suspend_error
+
+#
+# Two simple tests to check that verify fails for bad input data
+#
+info "checking bogus signature 1"
 ../tools/mk-tdata --char 0x2d 64 >x
 $GPG --verify x data-500 && error "no error code from verify"
+info "checking bogus signature 2"
 ../tools/mk-tdata --char 0xca 64 >x
 $GPG --verify x data-500 && error "no error code from verify"
 
-exit 0
+linefeed
+
+# A variable to collect the test names
+tests=""
+
+# A plain signed message created using
+#  echo abc | gpg --homedir . --passphrase-fd 0 -u Alpha -z0 -sa msg
+tests="$tests msg_ols_asc"
+msg_ols_asc='-----BEGIN PGP MESSAGE-----
+
+kA0DAAIRLXJ8x2hpdzQBrQEHYgNtc2dEDFJaSSB0aGluayB0aGF0IGFsbCByaWdo
+dC10aGlua2luZyBwZW9wbGUgaW4gdGhpcyBjb3VudHJ5IGFyZSBzaWNrIGFuZAp0
+aXJlZCBvZiBiZWluZyB0b2xkIHRoYXQgb3JkaW5hcnkgZGVjZW50IHBlb3BsZSBh
+cmUgZmVkIHVwIGluIHRoaXMKY291bnRyeSB3aXRoIGJlaW5nIHNpY2sgYW5kIHRp
+cmVkLiAgSSdtIGNlcnRhaW5seSBub3QuICBCdXQgSSdtCnNpY2sgYW5kIHRpcmVk
+IG9mIGJlaW5nIHRvbGQgdGhhdCBJIGFtLgotIE1vbnR5IFB5dGhvbgqIPwMFAEQM
+UlotcnzHaGl3NBECR4IAoJlEGTY+bHjD2HYuCixLQCmk01pbAKCIjkzLOAmkZNm0
+D8luT78c/1x45Q==
+=a29i
+-----END PGP MESSAGE-----'
+
+# A plain signed message created using
+#  echo abc | gpg --homedir . --passphrase-fd 0 -u Alpha -sa msg
+tests="$tests msg_cols_asc"
+msg_cols_asc='-----BEGIN PGP MESSAGE-----
+
+owGbwMvMwCSoW1RzPCOz3IRxLSN7EnNucboLT6Cgp0JJRmZeNpBMLFFIzMlRKMpM
+zyjRBQtm5qUrFKTmF+SkKmTmgdQVKyTnl+aVFFUqJBalKhRnJmcrJOalcJVkFqWm
+KOSnKSSlgrSU5OekQMzLL0rJzEsEKk9JTU7NK4EZBtKcBtRRWgAzlwtmbnlmSQbU
+GJjxCmDj9RQUPNVzFZJTi0oSM/NyKhXy8kuAYk6lJSBxLlTF2NziqZCYq8elq+Cb
+n1dSqRBQWZKRn8fVYc/MygAKBljYCDIFiTDMT+9seu836Q+bevyHTJ0dzPNuvCjn
+ZpgrwX38z58rJsfYDhwOSS4SkN/d6vUAAA==
+=s6sY
+-----END PGP MESSAGE-----'
+
+# A PGP 2 style message.
+tests="$tests msg_sl_asc"
+msg_sl_asc='-----BEGIN PGP MESSAGE-----
+
+iD8DBQBEDFJaLXJ8x2hpdzQRAkeCAKCZRBk2Pmx4w9h2LgosS0AppNNaWwCgiI5M
+yzgJpGTZtA/Jbk+/HP9ceOWtAQdiA21zZ0QMUlpJIHRoaW5rIHRoYXQgYWxsIHJp
+Z2h0LXRoaW5raW5nIHBlb3BsZSBpbiB0aGlzIGNvdW50cnkgYXJlIHNpY2sgYW5k
+CnRpcmVkIG9mIGJlaW5nIHRvbGQgdGhhdCBvcmRpbmFyeSBkZWNlbnQgcGVvcGxl
+IGFyZSBmZWQgdXAgaW4gdGhpcwpjb3VudHJ5IHdpdGggYmVpbmcgc2ljayBhbmQg
+dGlyZWQuICBJJ20gY2VydGFpbmx5IG5vdC4gIEJ1dCBJJ20Kc2ljayBhbmQgdGly
+ZWQgb2YgYmVpbmcgdG9sZCB0aGF0IEkgYW0uCi0gTW9udHkgUHl0aG9uCg==
+=0ukK
+-----END PGP MESSAGE-----'
+
+# An OpenPGP message lacking the onepass packet.  We used to accept
+# such messages but now consider them invalid.
+tests="$tests bad_ls_asc"
+bad_ls_asc='-----BEGIN PGP MESSAGE-----
+
+rQEHYgNtc2dEDFJaSSB0aGluayB0aGF0IGFsbCByaWdodC10aGlua2luZyBwZW9w
+bGUgaW4gdGhpcyBjb3VudHJ5IGFyZSBzaWNrIGFuZAp0aXJlZCBvZiBiZWluZyB0
+b2xkIHRoYXQgb3JkaW5hcnkgZGVjZW50IHBlb3BsZSBhcmUgZmVkIHVwIGluIHRo
+aXMKY291bnRyeSB3aXRoIGJlaW5nIHNpY2sgYW5kIHRpcmVkLiAgSSdtIGNlcnRh
+aW5seSBub3QuICBCdXQgSSdtCnNpY2sgYW5kIHRpcmVkIG9mIGJlaW5nIHRvbGQg
+dGhhdCBJIGFtLgotIE1vbnR5IFB5dGhvbgqIPwMFAEQMUlotcnzHaGl3NBECR4IA
+oJlEGTY+bHjD2HYuCixLQCmk01pbAKCIjkzLOAmkZNm0D8luT78c/1x45Q==
+=Mpiu
+-----END PGP MESSAGE-----'
+
+
+# A signed message prefixed with an unsigned literal packet.
+# (fols = faked-literal-data, one-pass, literal-data, signature)
+# This should throw an error because running gpg to extract the
+# signed data will return both literal data packets
+tests="$tests bad_fols_asc"
+bad_fols_asc='-----BEGIN PGP MESSAGE-----
+
+rF1iDG1zZy51bnNpZ25lZEQMY0x0aW1lc2hhcmluZywgbjoKCUFuIGFjY2VzcyBt
+ZXRob2Qgd2hlcmVieSBvbmUgY29tcHV0ZXIgYWJ1c2VzIG1hbnkgcGVvcGxlLgqQ
+DQMAAhEtcnzHaGl3NAGtAQdiA21zZ0QMUlpJIHRoaW5rIHRoYXQgYWxsIHJpZ2h0
+LXRoaW5raW5nIHBlb3BsZSBpbiB0aGlzIGNvdW50cnkgYXJlIHNpY2sgYW5kCnRp
+cmVkIG9mIGJlaW5nIHRvbGQgdGhhdCBvcmRpbmFyeSBkZWNlbnQgcGVvcGxlIGFy
+ZSBmZWQgdXAgaW4gdGhpcwpjb3VudHJ5IHdpdGggYmVpbmcgc2ljayBhbmQgdGly
+ZWQuICBJJ20gY2VydGFpbmx5IG5vdC4gIEJ1dCBJJ20Kc2ljayBhbmQgdGlyZWQg
+b2YgYmVpbmcgdG9sZCB0aGF0IEkgYW0uCi0gTW9udHkgUHl0aG9uCog/AwUARAxS
+Wi1yfMdoaXc0EQJHggCgmUQZNj5seMPYdi4KLEtAKaTTWlsAoIiOTMs4CaRk2bQP
+yW5Pvxz/XHjl
+=UNM4
+-----END PGP MESSAGE-----'
+
+
+
+
+#
+# Now run the tests.
+#
+for i in $tests ; do
+   info "checking: $i"
+   case "$i" in
+    msg_*_asc)
+       eval "(IFS=; echo \"\$$i\")" >x
+       $GPG --verify x || error "verify of $i failed"
+       ;;
+    bad_*_asc)
+       eval "(IFS=; echo \"\$$i\")" >x
+       $GPG --verify x && error "verify of $i succeeded but should not"
+       ;;
+    *)
+       error "No handler for test case $i"
+       ;;
+   esac
+   linefeed
+done
+
+
+resume_error

g10/ChangeLog

@@ -1,7 +1,8 @@
 2006-03-06  Werner Koch  <wk@g10code.com>
 
-	* mainproc.c (check_sig_and_print): Check for multiple plaintexts
+	* mainproc.c (check_sig_and_print): Made the composition test more
-	before a signature.  Reported by Tavis Ormandy.
+	tight.  This is due to another bug report by Tavis Ormandy.
+	(add_onepass_sig): Simplified.
 
 2006-03-05  Werner Koch  <wk@g10code.com>
 

g10/mainproc.c

@@ -116,21 +116,8 @@ add_onepass_sig( CTX c, PACKET *pkt )
 {
   KBNODE node;
 
-    if( c->list ) { /* add another packet */
+  if ( c->list ) /* add another packet */
-        /* We can only append another onepass packet if the list
-         * does contain only onepass packets */
-        for( node=c->list; node && node->pkt->pkttype == PKT_ONEPASS_SIG;
-             node = node->next )
-            ;
-	if( node ) {
-            /* this is not the case, so we flush the current thing and 
-             * allow this packet to start a new verification thing */
-	   release_list( c );
-	   c->list = new_kbnode( pkt );
-	}
-	else
     add_kbnode( c->list, new_kbnode( pkt ));
-    }
   else /* insert the first one */
     c->list = node = new_kbnode( pkt );
 
@@ -1420,89 +1407,114 @@ check_sig_and_print( CTX c, KBNODE node )
   const char *astr;
   int rc, is_expkey=0, is_revkey=0;
 
-    if( opt.skip_verify ) {
+  if (opt.skip_verify)
+    {
       log_info(_("signature verification suppressed\n"));
       return 0;
     }
 
-    /* It is not in all cases possible to check multiple signatures:
+  /* Check that the message composition is valid.
-     * PGP 2 (which is also allowed by OpenPGP), does use the packet
+
-     * sequence: sig+data,  OpenPGP does use onepas+data=sig and GnuPG
+     Per RFC-2440bis (-15) allowed:
-     * sometimes uses (because I did'nt read the specs right) data+sig.
+
-     * Because it is possible to create multiple signatures with
+     S{1,n}           -- detached signature.
-     * different packet sequence (e.g. data+sig and sig+data) it might
+     S{1,n} P         -- old style PGP2 signature
-     * not be possible to get it right:  let's say we have:
+     O{1,n} P S{1,n}  -- standard OpenPGP signature.
-     * data+sig, sig+data,sig+data and we have not yet encountered the last
+     C P S{1,n}       -- cleartext signature.
-     * data, we could also see this a one data with 2 signatures and then 
+
-     * data+sig.
+        
-     * To protect against this we check that all signatures follow
+          O = One-Pass Signature packet.
-     * without any intermediate packets.  Note, that we won't get this
+          S = Signature packet.
-     * error when we use onepass packets or cleartext signatures because
+          P = OpenPGP Message packet (Encrypted | Compressed | Literal)
-     * we reset the list every time
+                 (Note that the current rfc2440bis draft also allows
-     *
+                  for a signed message but that does not work as it
-     * FIXME: Now that we have these marker packets, we should create a 
+                  introduces ambiguities.)
-     * real grammar and check against this.
+              We keep track of these packages using the marker packet
+              CTRLPKT_PLAINTEXT_MARK.
+          C = Marker packet for cleartext signatures.
+
+     We reject all other messages.
+     
+     Actually we are calling this too often, i.e. for verification of
+     each message but better have some duplicate work than to silently
+     introduce a bug here.
   */
   {
     KBNODE n;
-        int n_sig = 0;
+    int n_onepass, n_sig;
-        int n_plaintext = 0;
-        int sig_seen, onepass_seen;
 
-        for (n=c->list; n; n=n->next ) 
+    log_debug ("checking signature packet composition\n");
-          {
+    dump_kbnode (c->list);
+
+    n = c->list;
+    assert (n);
     if ( n->pkt->pkttype == PKT_SIGNATURE ) 
-              n_sig++;
+      {
+        /* This is either "S{1,n}" case (detached signature) or
+           "S{1,n} P" (old style PGP2 signature). */
+        for (n = n->next; n; n = n->next)
+          if (n->pkt->pkttype != PKT_SIGNATURE)
+            break;
+        if (!n)
+          ; /* Okay, this is a detached signature.  */
         else if (n->pkt->pkttype == PKT_GPG_CONTROL
                  && (n->pkt->pkt.gpg_control->control
                      == CTRLPKT_PLAINTEXT_MARK) )
-              n_plaintext++;
+          {
+            if (n->next)
+              goto ambiguous;  /* We only allow one P packet. */
+          }
+        else
+          goto ambiguous;
+      }
+    else if (n->pkt->pkttype == PKT_ONEPASS_SIG) 
+      {
+        /* This is the "O{1,n} P S{1,n}" case (standard signature). */
+        for (n_onepass=1, n = n->next;
+             n && n->pkt->pkttype == PKT_ONEPASS_SIG; n = n->next)
+          n_onepass++;
+        if (!n || !(n->pkt->pkttype == PKT_GPG_CONTROL
+                    && (n->pkt->pkt.gpg_control->control
+                        == CTRLPKT_PLAINTEXT_MARK)))
+          goto ambiguous;
+        for (n_sig=0, n = n->next;
+             n && n->pkt->pkttype == PKT_SIGNATURE; n = n->next)
+          n_sig++;
+        if (n || !n_sig)
+          goto ambiguous;
+        if (n_onepass != n_sig)
+          {
+            log_info ("number of one-pass packets does not match "
+                      "number of signature packets\n");
+            goto ambiguous;
+          }
+      }
+    else if (n->pkt->pkttype == PKT_GPG_CONTROL
+             && n->pkt->pkt.gpg_control->control == CTRLPKT_CLEARSIGN_START )
+      {
+        /* This is the "C P S{1,n}" case (clear text signature). */
+        n = n->next;
+        if (!n || !(n->pkt->pkttype == PKT_GPG_CONTROL
+                    && (n->pkt->pkt.gpg_control->control
+                        == CTRLPKT_PLAINTEXT_MARK)))
+          goto ambiguous;
+        for (n_sig=0, n = n->next;
+             n && n->pkt->pkttype == PKT_SIGNATURE; n = n->next)
+          n_sig++;
+        if (n || !n_sig)
+          goto ambiguous;
+      }
+    else 
+      {
+      ambiguous:
+        log_error(_("can't handle this ambiguous signature data\n"));
+        return 0;
       }
 
-        for (sig_seen=onepass_seen=0,n=c->list; n; n=n->next ) 
-          {
-            if (n->pkt->pkttype == PKT_ONEPASS_SIG) 
-              {
-                onepass_seen++;
-              }
-            else if (n->pkt->pkttype == PKT_GPG_CONTROL
-                     && (n->pkt->pkt.gpg_control->control
-                         == CTRLPKT_CLEARSIGN_START) ) 
-              {
-                onepass_seen++; /* Handle the same way as a onepass. */
-              }
-            else if ( (sig_seen && n->pkt->pkttype != PKT_SIGNATURE) ) 
-              {
-                log_error(_("can't handle these multiple signatures\n"));
-                return 0;
-              }
-            else if ( n->pkt->pkttype == PKT_SIGNATURE ) 
-              {
-                sig_seen = 1;
-              }
-            else if (n_sig > 1 && !sig_seen && !onepass_seen 
-                     && n->pkt->pkttype == PKT_GPG_CONTROL
-                     && (n->pkt->pkt.gpg_control->control
-                            == CTRLPKT_PLAINTEXT_MARK) )
-              {
-                /* Plaintext before signatures but no onepass
-                   signature packets. */
-                log_error(_("can't handle these multiple signatures\n"));
-                return 0;
-              }
-            else if (n_plaintext > 1 && !sig_seen && !onepass_seen 
-                     && n->pkt->pkttype == PKT_GPG_CONTROL
-                     && (n->pkt->pkt.gpg_control->control
-                            == CTRLPKT_PLAINTEXT_MARK) )
-              {
-                /* More than one plaintext before a signature but no
-                   onepass packets.  */
-                log_error(_("can't handle this ambiguous signed data\n"));
-                return 0;
-              }
-          }
   }
 
+  /* (Indendation below not yet changed to GNU style.) */
+
     astr = pubkey_algo_to_string( sig->pubkey_algo );
     if(keystrlen()>8)
       {
@@ -1926,7 +1938,8 @@ proc_tree( CTX c, KBNODE node )
 	    /* prepare to create all requested message digests */
 	    c->mfx.md = md_open(0, 0);
 
-	    /* fixme: why looking for the signature packet and not 1passpacket*/
+	    /* fixme: why looking for the signature packet and not the
+               one-pass packet? */
 	    for( n1 = node; (n1 = find_next_kbnode(n1, PKT_SIGNATURE )); ) {
 		md_enable( c->mfx.md, n1->pkt->pkt.signature->digest_algo);
 	    }