openpgp: Handle signatures that have multiple signature packets

Fixes #12242

Change-Id: Ie40e84bc9e7db390219d85a7eddea99accb19cb8
Reviewed-on: https://go-review.googlesource.com/13781
Reviewed-by: Adam Langley <agl@golang.org>

openpgp/read.go

@@ -8,12 +8,13 @@ package openpgp // import "golang.org/x/crypto/openpgp"
 import (
 	"crypto"
 	_ "crypto/sha256"
-	"golang.org/x/crypto/openpgp/armor"
-	"golang.org/x/crypto/openpgp/errors"
-	"golang.org/x/crypto/openpgp/packet"
 	"hash"
 	"io"
 	"strconv"
+
+	"golang.org/x/crypto/openpgp/armor"
+	"golang.org/x/crypto/openpgp/errors"
+	"golang.org/x/crypto/openpgp/packet"
 )
 
 // SignatureType is the armor type for a PGP signature.
@@ -357,44 +358,55 @@ func (scr *signatureCheckReader) Read(buf []byte) (n int, err error) {
 // returns the signer if the signature is valid. If the signer isn't known,
 // ErrUnknownIssuer is returned.
 func CheckDetachedSignature(keyring KeyRing, signed, signature io.Reader) (signer *Entity, err error) {
-	p, err := packet.Read(signature)
-	if err != nil {
-		return
-	}
-
 	var issuerKeyId uint64
 	var hashFunc crypto.Hash
 	var sigType packet.SignatureType
+	var keys []Key
+	var p packet.Packet
 
-	switch sig := p.(type) {
-	case *packet.Signature:
-		if sig.IssuerKeyId == nil {
-			return nil, errors.StructuralError("signature doesn't have an issuer")
+	packets := packet.NewReader(signature)
+	for {
+		p, err = packets.Next()
+		if err == io.EOF {
+			return nil, errors.ErrUnknownIssuer
 		}
-		issuerKeyId = *sig.IssuerKeyId
-		hashFunc = sig.Hash
-		sigType = sig.SigType
-	case *packet.SignatureV3:
-		issuerKeyId = sig.IssuerKeyId
-		hashFunc = sig.Hash
-		sigType = sig.SigType
-	default:
-		return nil, errors.StructuralError("non signature packet found")
+		if err != nil {
+			return nil, err
+		}
+
+		switch sig := p.(type) {
+		case *packet.Signature:
+			if sig.IssuerKeyId == nil {
+				return nil, errors.StructuralError("signature doesn't have an issuer")
+			}
+			issuerKeyId = *sig.IssuerKeyId
+			hashFunc = sig.Hash
+			sigType = sig.SigType
+		case *packet.SignatureV3:
+			issuerKeyId = sig.IssuerKeyId
+			hashFunc = sig.Hash
+			sigType = sig.SigType
+		default:
+			return nil, errors.StructuralError("non signature packet found")
+		}
+
+		keys = keyring.KeysByIdUsage(issuerKeyId, packet.KeyFlagSign)
+		if len(keys) > 0 {
+			break
+		}
+	}
+
+	if len(keys) == 0 {
+		panic("unreachable")
 	}
 
 	h, wrappedHash, err := hashForSignature(hashFunc, sigType)
 	if err != nil {
-		return
+		return nil, err
 	}
 
-	_, err = io.Copy(wrappedHash, signed)
-	if err != nil && err != io.EOF {
-		return
-	}
-
-	keys := keyring.KeysByIdUsage(issuerKeyId, packet.KeyFlagSign)
-	if len(keys) == 0 {
-		return nil, errors.ErrUnknownIssuer
+	if _, err := io.Copy(wrappedHash, signed); err != nil && err != io.EOF {
+		return nil, err
 	}
 
 	for _, key := range keys {
@@ -403,15 +415,15 @@ func CheckDetachedSignature(keyring KeyRing, signed, signature io.Reader) (signe
 			err = key.PublicKey.VerifySignature(h, sig)
 		case *packet.SignatureV3:
 			err = key.PublicKey.VerifySignatureV3(h, sig)
+		default:
+			panic("unreachable")
 		}
+
 		if err == nil {
 			return key.Entity, nil
 		}
 	}
 
-	if err == nil {
-		err = errors.ErrUnknownIssuer
-	}
 	return nil, err
 }
 

openpgp/read_test.go

@@ -315,6 +315,11 @@ func TestDetachedSignatureDSA(t *testing.T) {
 	testDetachedSignature(t, kring, readerFromHex(detachedSignatureDSAHex), signedInput, "binary", testKey3KeyId)
 }
 
+func TestMultipleSignaturePacketsDSA(t *testing.T) {
+	kring, _ := ReadKeyRing(readerFromHex(dsaTestKeyHex))
+	testDetachedSignature(t, kring, readerFromHex(missingHashFunctionHex+detachedSignatureDSAHex), signedInput, "binary", testKey3KeyId)
+}
+
 func testHashFunctionError(t *testing.T, signatureHex string) {
 	kring, _ := ReadKeyRing(readerFromHex(testKeys1And2Hex))
 	_, err := CheckDetachedSignature(kring, nil, readerFromHex(signatureHex))
@@ -338,8 +343,16 @@ func TestUnknownHashFunction(t *testing.T) {
 
 func TestMissingHashFunction(t *testing.T) {
 	// missingHashFunctionHex contains a signature packet that uses
-	// RIPEMD160, which isn't compiled in.
-	testHashFunctionError(t, missingHashFunctionHex)
+	// RIPEMD160, which isn't compiled in.  Since that's the only signature
+	// packet we don't find any suitable packets and end up with ErrUnknownIssuer
+	kring, _ := ReadKeyRing(readerFromHex(testKeys1And2Hex))
+	_, err := CheckDetachedSignature(kring, nil, readerFromHex(missingHashFunctionHex))
+	if err == nil {
+		t.Fatal("Packet with missing hash type was correctly parsed")
+	}
+	if err != errors.ErrUnknownIssuer {
+		t.Fatalf("Unexpected class of error: %s", err)
+	}
 }
 
 func TestReadingArmoredPrivateKey(t *testing.T) {
@@ -488,6 +501,6 @@ const dsaKeyWithSHA512 = `9901a2044f04b07f110400db244efecc7316553ee08d179972aab8
 
 const unknownHashFunctionHex = `8a00000040040001990006050253863c24000a09103b4fe6acc0b21f32ffff01010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101`
 
-const missingHashFunctionHex = `8a00000040040001030006050253863c24000a09103b4fe6acc0b21f32ffff01010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101`
+const missingHashFunctionHex = `8a00000040040001030006050253863c24000a09103b4fe6acc0b21f32ffff0101010101010101010101010101010101010101010101010101010101010101010101010101`
 
 const campbellQuine = `a0b001000300fcffa0b001000d00f2ff000300fcffa0b001000d00f2ff8270a01c00000500faff8270a01c00000500faff000500faff001400ebff8270a01c00000500faff000500faff001400ebff428821c400001400ebff428821c400001400ebff428821c400001400ebff428821c400001400ebff428821c400000000ffff000000ffff000b00f4ff428821c400000000ffff000000ffff000b00f4ff0233214c40000100feff000233214c40000100feff0000`