openpgp: don't treat extra subkey selfsigs as uid sigs

Consider the following packet ordering scenario:
    PUBKEY UID SELFSIG SUBKEY REV SELFSIG

In this scenario, addSubkey would only consume the REV signature after
the subkey, leaving SELFSIG to be read by ReadEntity, which in turn
would add the last SELFSIG to the UID's signatures, which is wrong to do
because this is a SUBKEY SELFSIG, not a UID signature.

Remove "current" from the ReadEntity scope, it should only be visible
to the UserId packet handling code.

Keep the warning about signature packets found before user id packets.
Without it, I would not have found this bug.

Modify addSubKey so that it consumes all signatures following the SUBKEY
packet, keeping eithier the first valid signature (like we did before)
or any valid revocation.

In a follow-up patch, we can improve this further by keeping the
most recent signature, as suggested by RFC4880:
> An implementation that encounters multiple self-signatures on the
> same object may resolve the ambiguity in any way it sees fit, but it
> is RECOMMENDED that priority be given to the most recent self-
> signature.

Fixes golang/go#26449

Change-Id: Id992676ef2363779a7028f4799180efb027fcf47
Reviewed-on: https://go-review.googlesource.com/118957
Reviewed-by: Filippo Valsorda <filippo@golang.org>

openpgp/keys.go

@@ -333,7 +333,6 @@ func ReadEntity(packets *packet.Reader) (*Entity, error) {
 		return nil, errors.StructuralError("primary key cannot be used for signatures")
 	}
 
-	var current *Identity
 	var revocations []*packet.Signature
 EachPacket:
 	for {
@@ -349,7 +348,7 @@ EachPacket:
 			// Make a new Identity object, that we might wind up throwing away.
 			// We'll only add it if we get a valid self-signature over this
 			// userID.
-			current = new(Identity)
+			current := new(Identity)
 			current.Name = pkt.Id
 			current.UserId = pkt
 
@@ -384,11 +383,9 @@ EachPacket:
 				// TODO: RFC4880 5.2.1 permits signatures
 				// directly on keys (eg. to bind additional
 				// revocation keys).
-			} else if current == nil {
-				return nil, errors.StructuralError("signature packet found before user id packet")
-			} else {
-				current.Signatures = append(current.Signatures, pkt)
 			}
+			// Else, ignoring the signature as it does not follow anything
+			// we would know to attach it to.
 		case *packet.PrivateKey:
 			if pkt.IsSubkey == false {
 				packets.Unread(p)
@@ -433,26 +430,45 @@ func addSubkey(e *Entity, packets *packet.Reader, pub *packet.PublicKey, priv *p
 	var subKey Subkey
 	subKey.PublicKey = pub
 	subKey.PrivateKey = priv
-	p, err := packets.Next()
-	if err == io.EOF {
-		return io.ErrUnexpectedEOF
-	}
-	if err != nil {
-		return errors.StructuralError("subkey signature invalid: " + err.Error())
+
+	for {
+		p, err := packets.Next()
+		if err == io.EOF {
+			break
+		} else if err != nil {
+			return errors.StructuralError("subkey signature invalid: " + err.Error())
+		}
+
+		sig, ok := p.(*packet.Signature)
+		if !ok {
+			packets.Unread(p)
+			break
+		}
+
+		if sig.SigType != packet.SigTypeSubkeyBinding && sig.SigType != packet.SigTypeSubkeyRevocation {
+			return errors.StructuralError("subkey signature with wrong type")
+		}
+
+		if err := e.PrimaryKey.VerifyKeySignature(subKey.PublicKey, sig); err != nil {
+			return errors.StructuralError("subkey signature invalid: " + err.Error())
+		}
+
+		switch sig.SigType {
+		case packet.SigTypeSubkeyRevocation:
+			subKey.Sig = sig
+		case packet.SigTypeSubkeyBinding:
+			if subKey.Sig == nil {
+				subKey.Sig = sig
+			}
+		}
 	}
-	var ok bool
-	subKey.Sig, ok = p.(*packet.Signature)
-	if !ok {
+
+	if subKey.Sig == nil {
 		return errors.StructuralError("subkey packet not followed by signature")
 	}
-	if subKey.Sig.SigType != packet.SigTypeSubkeyBinding && subKey.Sig.SigType != packet.SigTypeSubkeyRevocation {
-		return errors.StructuralError("subkey signature with wrong type")
-	}
-	err = e.PrimaryKey.VerifyKeySignature(subKey.PublicKey, subKey.Sig)
-	if err != nil {
-		return errors.StructuralError("subkey signature invalid: " + err.Error())
-	}
+
 	e.Subkeys = append(e.Subkeys, subKey)
+
 	return nil
 }
 

openpgp/keys_test.go

@@ -180,6 +180,44 @@ func TestKeyRevocation(t *testing.T) {
 	}
 }
 
+func TestKeyWithRevokedSubKey(t *testing.T) {
+	// This key contains a revoked sub key:
+	//  pub   rsa1024/0x4CBD826C39074E38 2018-06-14 [SC]
+	//        Key fingerprint = 3F95 169F 3FFA 7D3F 2B47  6F0C 4CBD 826C 3907 4E38
+	//  uid   Golang Gopher <no-reply@golang.com>
+	//  sub   rsa1024/0x945DB1AF61D85727 2018-06-14 [S] [revoked: 2018-06-14]
+
+	keys, err := ReadArmoredKeyRing(bytes.NewBufferString(keyWithSubKey))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(keys) != 1 {
+		t.Fatal("Failed to read key with a sub key")
+	}
+
+	identity := keys[0].Identities["Golang Gopher <no-reply@golang.com>"]
+
+	// Test for an issue where Subkey Binding Signatures (RFC 4880 5.2.1) were added to the identity
+	// preceding the Subkey Packet if the Subkey Packet was followed by more than one signature.
+	// For example, the current key has the following layout:
+	//    PUBKEY UID SELFSIG SUBKEY REV SELFSIG
+	// The last SELFSIG would be added to the UID's signatures. This is wrong.
+	if numIdentitySigs, numExpected := len(identity.Signatures), 0; numIdentitySigs != numExpected {
+		t.Fatalf("got %d identity signatures, expected %d", numIdentitySigs, numExpected)
+	}
+
+	if numSubKeys, numExpected := len(keys[0].Subkeys), 1; numSubKeys != numExpected {
+		t.Fatalf("got %d subkeys, expected %d", numSubKeys, numExpected)
+	}
+
+	subKey := keys[0].Subkeys[0]
+	if subKey.Sig == nil {
+		t.Fatalf("subkey signature is nil")
+	}
+
+}
+
 func TestSubkeyRevocation(t *testing.T) {
 	kring, err := ReadKeyRing(readerFromHex(revokedSubkeyHex))
 	if err != nil {
@@ -547,3 +585,31 @@ heiQvzkApQup5c+BhH5zFDFdKJ2CBByxw9+7QjMFI/wgLixKuE0Ob2kAokXf7RlB
 7qTZOahrETw=
 =IKnw
 -----END PGP PUBLIC KEY BLOCK-----`
+
+const keyWithSubKey = `-----BEGIN PGP PUBLIC KEY BLOCK-----
+
+mI0EWyKwKQEEALwXhKBnyaaNFeK3ljfc/qn9X/QFw+28EUfgZPHjRmHubuXLE2uR
+s3ZoSXY2z7Dkv+NyHYMt8p+X8q5fR7JvUjK2XbPyKoiJVnHINll83yl67DaWfKNL
+EjNoO0kIfbXfCkZ7EG6DL+iKtuxniGTcnGT47e+HJSqb/STpLMnWwXjBABEBAAG0
+I0dvbGFuZyBHb3BoZXIgPG5vLXJlcGx5QGdvbGFuZy5jb20+iM4EEwEKADgWIQQ/
+lRafP/p9PytHbwxMvYJsOQdOOAUCWyKwKQIbAwULCQgHAwUVCgkICwUWAgMBAAIe
+AQIXgAAKCRBMvYJsOQdOOOsFBAC62mXww8XuqvYLcVOvHkWLT6mhxrQOJXnlfpn7
+2uBV9CMhoG/Ycd43NONsJrB95Apr9TDIqWnVszNbqPCuBhZQSGLdbiDKjxnCWBk0
+69qv4RNtkpOhYB7jK4s8F5oQZqId6JasT/PmJTH92mhBYhhTQr0GYFuPX2UJdkw9
+Sn9C67iNBFsisDUBBAC3A+Yo9lgCnxi/pfskyLrweYif6kIXWLAtLTsM6g/6jt7b
+wTrknuCPyTv0QKGXsAEe/cK/Xq3HvX9WfXPGIHc/X56ZIsHQ+RLowbZV/Lhok1IW
+FAuQm8axr/by80cRwFnzhfPc/ukkAq2Qyj4hLsGblu6mxeAhzcp8aqmWOO2H9QAR
+AQABiLYEKAEKACAWIQQ/lRafP/p9PytHbwxMvYJsOQdOOAUCWyK16gIdAAAKCRBM
+vYJsOQdOOB1vA/4u4uLONsE+2GVOyBsHyy7uTdkuxaR9b54A/cz6jT/tzUbeIzgx
+22neWhgvIEghnUZd0vEyK9k1wy5vbDlEo6nKzHso32N1QExGr5upRERAxweDxGOj
+7luDwNypI7QcifE64lS/JmlnunwRCdRWMKc0Fp+7jtRc5mpwyHN/Suf5RokBagQY
+AQoAIBYhBD+VFp8/+n0/K0dvDEy9gmw5B044BQJbIrA1AhsCAL8JEEy9gmw5B044
+tCAEGQEKAB0WIQSNdnkaWY6t62iX336UXbGvYdhXJwUCWyKwNQAKCRCUXbGvYdhX
+JxJSA/9fCPHP6sUtGF1o3G1a3yvOUDGr1JWcct9U+QpbCt1mZoNopCNDDQAJvDWl
+mvDgHfuogmgNJRjOMznvahbF+wpTXmB7LS0SK412gJzl1fFIpK4bgnhu0TwxNsO1
+8UkCZWqxRMgcNUn9z6XWONK8dgt5JNvHSHrwF4CxxwjL23AAtK+FA/UUoi3U4kbC
+0XnSr1Sl+mrzQi1+H7xyMe7zjqe+gGANtskqexHzwWPUJCPZ5qpIa2l8ghiUim6b
+4ymJ+N8/T8Yva1FaPEqfMzzqJr8McYFm0URioXJPvOAlRxdHPteZ0qUopt/Jawxl
+Xt6B9h1YpeLoJwjwsvbi98UTRs0jXwoY
+=3fWu
+-----END PGP PUBLIC KEY BLOCK-----`