Re: Re: Debian SKS packages was: [pgp-keyserver-folk] new pgp key server

On Sun, Sep 19, 2004 at 10:18:16PM -0400, Yaron Minsky wrote:
> On Sun, 19 Sep 2004 16:20:30 -0400, David Shaw <dshaw-wh+mT2OhP0WF0gnf/[email protected]> wrote:
> > On Sun, Sep 19, 2004 at 02:42:34PM -0400, Yaron Minsky wrote:
> > > On Sun, 19 Sep 2004 11:33:55 -0400, David Shaw <dshaw-wh+mT2OhP0WF0gnf/[email protected]> wrote:
> 
> > > Interesting.  This wouldn't be difficult from the way SKS handles
> > > things.  Basically, SKS first parses the key down to the packet level,
> > > which works without problems for this key.  Things like MPIs are only
> > > looked at within the packet level.  If one such is found to be bogus,
> > > the packet could then easily be thrown out.  I'm somewhat surprised
> > > it's not handled similarly in GPG.
> > 
> > But that's a problem also - you can't packetize something if the
> > packet boundaries are bogus.  Or rather you can, but you don't know
> > what you're going to get.
> 
> I'm not sure what you mean.  You can just handle this at different
> levels.  The packet-level structure of this key must be A-OK, since
> otherwise SKS would have barfed on it.  Once you break it up into
> packets, you can dissect those.  If any of the packets are bogus, you
> toss them out.  It seems simple enough, and I don't see what the
> pitfalls are of doing this.

No, you can't know the packet structure is ok without adding some
knowledge of packet internals or packet grammar.  It is actually
fairly easy for a corrupt packet to parse (as a raw packet) perfectly
well - and for the garbage that comes after it to parse as well.  For
example, let's take a public key packet that claims length 100, but is
really 150.  The packetizer reads off 100 bytes, then goes to read the
next "packet".  The chance of the next few bytes being readable as a
sane packet header is actually disturbingly good: the next byte read
has to have the 0x80 bit set, and that's about it.  The rest of the
initial byte doesn't matter very much to a raw packetizer, and the
length bytes are just numbers.

Now it's very likely that the bogus length bytes point off the end of
the data, so you know this second packet is bogus.  So it looks like a
good first packet, and a bogus second one.  Of course, that's not true
- they're both bogus.  There is no good internal sanity checking in
the OpenPGP packet format.

At a higher level, something could (as GnuPG does) look and wonder why
a public key packet is followed by a literal data packet or
unallocated packet number or something nonsensical like that, but then
we're talking about looking at grammar.

My point here is that while you can know fairly well that a given set
of packets is sane (if they all parse out and end in the right place
length-wise), but when they are not sane, you can't easily know
(without looking inside the packets) exactly WHERE things went insane.
This makes it hard to discard just the bad packets.

> You point out that this is a basic syntactic bug in the key format,
> and that it should be easy to discard the bogus packets.  That's
> true, but how much of the syntax of the RFC should I enforce?  I'm
> sure there are lots of syntactic grounds on which one could toss out
> keys, even though those keys might be perfectly well parseable with
> GPG and PGP.  The goal, I thought, was for keyservers to be
> generally lax, and for client software to clean up after.  I still
> feel like SKS is basically doing the right thing here, and that the
> problem is that GPG isn't cleaning up the bad data well enough.
> (That said, just fixing GPG isn't a great solution, given the
> installed-base problem.)

I am not fully convinced that having GnuPG try and massage garbage
into a usable key is a good idea.

To a certain degree, I want this to be the responsibility of the
storage (i.e. the keyservers).  The keyservers are charged with
delivering keys safely.  If someone is uploading garbage, then the
keyservers should not accept it.

I have been forced into key repair in the past with the PKS multiple
subkey nonsense, and there is also code in GnuPG to undo the PKS
duplicated user ID problem.  Where, though, does it end?  Every time
there is a new form of mangling on the keyservers, I don't want to add
code to GnuPG to try and de-mangle it.  I don't know of many pieces of
software that are expected to clean up after broken storage.
Photoshop doesn't try to reassemble bits and pieces of JPEG after the
file gets mangled.

To have GnuPG fix this particular key is actually pretty trivial.
I've attached a patch for the curious.  It's against CVS GnuPG, though
you could probably massage it into 1.2.x.  I'm just concerned that in
an effort to de-mangle a key, GnuPG might accept something it should
not.

There is a lot going for what you say.  This is something I need to
think about more.

David

Index: mpi/mpicoder.c
===================================================================
RCS file: /cvs/gnupg/gnupg/mpi/mpicoder.c,v
retrieving revision 1.31
diff -u -r1.31 mpicoder.c
--- mpi/mpicoder.c	24 May 2003 17:54:56 -0000	1.31
+++ mpi/mpicoder.c	20 Sep 2004 03:41:15 -0000
@@ -84,11 +84,11 @@
     if( (c = iobuf_get(inp)) == -1 )
 	goto leave;
     nbits |= c;
+    nread = 2;
     if( nbits > MAX_EXTERN_MPI_BITS ) {
 	log_error("mpi too large (%u bits)\n", nbits);
 	goto leave;
     }
-    nread = 2;
 
     nbytes = (nbits+7) / 8;
     nlimbs = (nbytes+BYTES_PER_MPI_LIMB-1) / BYTES_PER_MPI_LIMB;
Index: g10/import.c
===================================================================
RCS file: /cvs/gnupg/gnupg/g10/import.c,v
retrieving revision 1.113
diff -u -r1.113 import.c
--- g10/import.c	11 Sep 2004 15:42:19 -0000	1.113
+++ g10/import.c	20 Sep 2004 03:41:17 -0000
@@ -343,11 +343,30 @@
     init_packet(pkt);
     while( (rc=parse_packet(a, pkt)) != -1 ) {
 	if( rc ) {  /* ignore errors */
-	    if( rc != G10ERR_UNKNOWN_PACKET ) {
-		log_error("read_block: read error: %s\n", g10_errstr(rc) );
-		rc = G10ERR_INV_KEYRING;
-		goto ready;
-	    }
+	    if( rc != G10ERR_UNKNOWN_PACKET )
+	      {
+		if(rc==G10ERR_INVALID_PACKET
+		   && (pkt->pkttype==PKT_PUBLIC_SUBKEY
+		       || pkt->pkttype==PKT_SIGNATURE))
+		  {
+		    /* In this case we just drop the packet.  We've
+		       gotten far enough in parsing to know that it is
+		       a subkey or signature, but yet didn't get far
+		       enough in parsing to know the packet was
+		       complete.  Most likely, we had some MPI
+		       problems.  Note the minor gotcha here - if we
+		       drop a broken subkey, we might still accept a
+		       binding signature after it.  No harm, but it is
+		       aesthetically displeasing. */
+		    log_info("WARNING: key was partially corrupt\n");
+		  }
+		else
+		  {
+		    log_error("read_block: read error: %s\n", g10_errstr(rc) );
+		    rc = G10ERR_INV_KEYRING;
+		    goto ready;
+		  }
+	      }
 	    free_packet( pkt );
 	    init_packet(pkt);
 	    continue;

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.3.90-cvs (GNU/Linux)

iGoEARECACoFAkFOUmcjGGh0dHA6Ly93d3cuamFiYmVyd29ja3kuY29tL2tleS5h
c2MACgkQ4mZch0nhy8lFHgCeMHNc8OBJbpYDPpkwCJlOSBS61jsAn3ESBBg3v54I
ObnW04okMWmKMEHG
=8SDF
-----END PGP SIGNATURE-----