More rigorously check key and binding sig expirations

- Check primary identity self-signature expiration
- When a signature is made by a subkey, also check primary key expiration
- When a signature is made by a subkey, also check backsig expiration

Also, if a binding signature is expired, we now return ErrSignatureExpired
instead of ErrKeyExpired.

Finally, more clearly document that CheckDetachedSignature can still
return a signer even if a signature verification error occurred.

openpgp/read.go

@@ -384,19 +384,7 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) {
 					key := scr.md.SignedBy
 					signatureError := key.PublicKey.VerifySignature(scr.h, sig)
 					if signatureError == nil {
-						now := scr.config.Now()
-						if key.Revoked(now) ||
-							key.Entity.Revoked(now) || // primary key is revoked (redundant if key is the primary key)
-							key.Entity.PrimaryIdentity().Revoked(now) {
-							signatureError = errors.ErrKeyRevoked
-						}
-						if sig.SigExpired(now) {
-							signatureError = errors.ErrSignatureExpired
-						}
-						if key.PublicKey.KeyExpired(key.SelfSignature, now) ||
-							key.SelfSignature.SigExpired(now) {
-							signatureError = errors.ErrKeyExpired
-						}
+						signatureError = checkSignatureDetails(key, sig, scr.config)
 					}
 					scr.md.Signature = sig
 					scr.md.SignatureError = signatureError
@@ -436,7 +424,8 @@ func (scr *signatureCheckReader) Read(buf []byte) (int, error) {
 }
 
 // CheckDetachedSignature takes a signed file and a detached signature and
-// returns the signer if the signature is valid. If the signer isn't known,
+// returns the entity the signature was signed by, if any, and a possible
+// signature verification error. If the signer isn't known,
 // ErrUnknownIssuer is returned.
 func CheckDetachedSignature(keyring KeyRing, signed, signature io.Reader, config *packet.Config) (signer *Entity, err error) {
 	var expectedHashes []crypto.Hash
@@ -507,20 +496,7 @@ func CheckDetachedSignatureAndHash(keyring KeyRing, signed, signature io.Reader,
 	for _, key := range keys {
 		err = key.PublicKey.VerifySignature(h, sig)
 		if err == nil {
-			now := config.Now()
-			if key.Revoked(now) ||
-				key.Entity.Revoked(now) || // primary key is revoked (redundant if key is the primary key)
-				key.Entity.PrimaryIdentity().Revoked(now) {
-				return key.Entity, errors.ErrKeyRevoked
-			}
-			if sig.SigExpired(now) {
-				return key.Entity, errors.ErrSignatureExpired
-			}
-			if key.PublicKey.KeyExpired(key.SelfSignature, now) ||
-				key.SelfSignature.SigExpired(now) {
-				return key.Entity, errors.ErrKeyExpired
-			}
-			return key.Entity, nil
+			return key.Entity, checkSignatureDetails(&key, sig, config)
 		}
 	}
 
@@ -537,3 +513,46 @@ func CheckArmoredDetachedSignature(keyring KeyRing, signed, signature io.Reader,
 
 	return CheckDetachedSignature(keyring, signed, body, config)
 }
+
+// checkSignatureDetails returns an error if:
+// - The primary key of the signing entity is revoked
+// The signature was signed by a subkey and:
+//   - The signing subkey is revoked
+// - The primary identity is revoked
+// - The signature is expired
+// - The primary key of the signing entity is expired according to the primary identity binding signature
+// The signature was signed by a subkey and:
+//   - The signing subkey is expired according to the subkey binding signature
+//   - The signing subkey binding signature is expired
+//   - The signing subkey cross-signature is expired
+// NOTE: The order of these checks is important, as the caller may choose to
+// ignore ErrSignatureExpired or ErrKeyExpired errors, but should never
+// ignore any other errors.
+// TODO: Also return an error if:
+// - The primary key is expired according to a direct-key signature
+// - (For V5 keys only:) The direct-key signature (exists and) is expired
+func checkSignatureDetails(key *Key, signature *packet.Signature, config *packet.Config) error {
+	now := config.Now()
+	primaryIdentity := key.Entity.PrimaryIdentity()
+	sigsToCheck := []*packet.Signature{ signature, primaryIdentity.SelfSignature }
+	if key.Entity.Revoked(now) || // primary key is revoked
+		(key.PublicKey != key.Entity.PrimaryKey && key.Revoked(now)) || // subkey is revoked
+		primaryIdentity.Revoked(now) { // primary identity is revoked
+		return errors.ErrKeyRevoked
+	}
+	if key.Entity.PrimaryKey.KeyExpired(primaryIdentity.SelfSignature, now) { // primary key is expired
+		return errors.ErrKeyExpired
+	}
+	if key.PublicKey != key.Entity.PrimaryKey {
+		if key.PublicKey.KeyExpired(key.SelfSignature, now) { // subkey is expired
+			return errors.ErrKeyExpired
+		}
+		sigsToCheck = append(sigsToCheck, key.SelfSignature, key.SelfSignature.EmbeddedSignature)
+	}
+	for _, sig := range sigsToCheck {
+		if sig.SigExpired(now) { // any of the relevant signatures are expired
+			return errors.ErrSignatureExpired
+		}
+	}
+	return nil
+}

openpgp/read_test.go

@@ -423,6 +423,18 @@ func TestRSASignatureBadMPILength(t *testing.T) {
 	}
 }
 
+func TestDetachedSignatureExpiredCrossSig(t *testing.T) {
+	kring, _ := ReadArmoredKeyRing(bytes.NewBufferString(keyWithExpiredCrossSig))
+	config := &packet.Config{}
+	_, err := CheckArmoredDetachedSignature(kring, bytes.NewBufferString("Hello World :)"), bytes.NewBufferString(sigFromKeyWithExpiredCrossSig), config)
+	if err == nil {
+		t.Fatal("Signature from key with expired subkey binding embedded signature was accepted")
+	}
+	if err != errors.ErrSignatureExpired {
+		t.Fatalf("Unexpected class of error: %s", err)
+	}
+}
+
 func TestReadingArmoredPrivateKey(t *testing.T) {
 	el, err := ReadArmoredKeyRing(bytes.NewBufferString(armoredPrivateKeyBlock))
 	if err != nil {

openpgp/read_write_test_data.go

@@ -171,3 +171,71 @@ y29VPonFXqi2zKkpZrvyvZxg+n5e8Nt9wNbuxeCd3QD/TtO2s+JvjrE4Siwv
 UQdl5MlBka1QSNbMq2Bz7XwNPg4=
 =6lbM
 -----END PGP MESSAGE-----`
+
+const keyWithExpiredCrossSig = `-----BEGIN PGP PUBLIC KEY BLOCK-----
+
+xsDNBF2lnPIBDAC5cL9PQoQLTMuhjbYvb4Ncuuo0bfmgPRFywX53jPhoFf4Zg6mv
+/seOXpgecTdOcVttfzC8ycIKrt3aQTiwOG/ctaR4Bk/t6ayNFfdUNxHWk4WCKzdz
+/56fW2O0F23qIRd8UUJp5IIlN4RDdRCtdhVQIAuzvp2oVy/LaS2kxQoKvph/5pQ/
+5whqsyroEWDJoSV0yOb25B/iwk/pLUFoyhDG9bj0kIzDxrEqW+7Ba8nocQlecMF3
+X5KMN5kp2zraLv9dlBBpWW43XktjcCZgMy20SouraVma8Je/ECwUWYUiAZxLIlMv
+9CurEOtxUw6N3RdOtLmYZS9uEnn5y1UkF88o8Nku890uk6BrewFzJyLAx5wRZ4F0
+qV/yq36UWQ0JB/AUGhHVPdFf6pl6eaxBwT5GXvbBUibtf8YI2og5RsgTWtXfU7eb
+SGXrl5ZMpbA6mbfhd0R8aPxWfmDWiIOhBufhMCvUHh1sApMKVZnvIff9/0Dca3wb
+vLIwa3T4CyshfT0AEQEAAc0hQm9iIEJhYmJhZ2UgPGJvYkBvcGVucGdwLmV4YW1w
+bGU+wsEABBMBCgATBYJeO2eVAgsJAxUICgKbAQIeAQAhCRD7/MgqAV5zMBYhBNGm
+bhojsYLJmA94jPv8yCoBXnMwKWUMAJ3FKZfJ2mXvh+GFqgymvK4NoKkDRPB0CbUN
+aDdG7ZOizQrWXo7Da2MYIZ6eZUDqBKLdhZ5gZfVnisDfu/yeCgpENaKib1MPHpA8
+nZQjnPejbBDomNqY8HRzr5jvXNlwywBpjWGtegCKUY9xbSynjbfzIlMrWL4S+Rfl
++bOOQKRyYJWXmECmVyqY8cz2VUYmETjNcwC8VCDUxQnhtcCJ7Aej22hfYwVEPb/J
+BsJBPq8WECCiGfJ9Y2y6TF+62KzG9Kfs5hqUeHhQy8V4TSi479ewwL7DH86XmIIK
+chSANBS+7iyMtctjNZfmF9zYdGJFvjI/mbBR/lK66E515Inuf75XnL8hqlXuwqvG
+ni+i03Aet1DzULZEIio4uIU6ioc1lGO9h7K2Xn4S7QQH1QoISNMWqXibUR0RCGjw
+FsEDTt2QwJl8XXxoJCooM7BCcCQo+rMNVUHDjIwrdoQjPld3YZsUQQRcqH6bLuln
+cfn5ufl8zTGWKydoj/iTz8KcjZ7w187AzQRdpZzyAQwA1jC/XGxjK6ddgrRfW9j+
+s/U00++EvIsgTs2kr3Rg0GP7FLWV0YNtR1mpl55/bEl7yAxCDTkOgPUMXcaKlnQh
+6zrlt6H53mF6Bvs3inOHQvOsGtU0dqvb1vkTF0juLiJgPlM7pWv+pNQ6IA39vKoQ
+sTMBv4v5vYNXP9GgKbg8inUNT17BxzZYHfw5+q63ectgDm2on1e8CIRCZ76oBVwz
+dkVxoy3gjh1eENlk2D4P0uJNZzF1Q8GV67yLANGMCDICE/OkWn6daipYDzW4iJQt
+YPUWP4hWhjdm+CK+hg6IQUEn2Vtvi16D2blRP8BpUNNa4fNuylWVuJV76rIHvsLZ
+1pbM3LHpRgE8s6jivS3Rz3WRs0TmWCNnvHPqWizQ3VTy+r3UQVJ5AmhJDrZdZq9i
+aUIuZ01PoE1+CHiJwuxPtWvVAxf2POcm1M/F1fK1J0e+lKlQuyonTXqXR22Y41wr
+fP2aPk3nPSTW2DUAf3vRMZg57ZpRxLEhEMxcM4/LMR+PABEBAAHCwrIEGAEKAAkF
+gl8sAVYCmwIB3QkQ+/zIKgFeczDA+qAEGQEKAAwFgl47Z5UFgwB4TOAAIQkQfC+q
+Tfk8N7IWIQQd3OFfCSF87i87N2B8L6pN+Tw3st58C/0exp0X2U4LqicSHEOSqHZj
+jiysdqIELHGyo5DSPv92UFPp36aqjF9OFgtNNwSa56fmAVCD4+hor/fKARRIeIjF
+qdIC5Y/9a4B10NQFJa5lsvB38x/d39LI2kEoglZnqWgdJskROo3vNQF4KlIcm6FH
+dn4WI8UkC5oUUcrpZVMSKoacIaxLwqnXT42nIVgYYuqrd/ZagZZjG5WlrTOd5+NI
+zi/l0fWProcPHGLjmAh4Thu8i7omtVw1nQaMnq9I77ffg3cPDgXknYrLL+q8xXh/
+0mEJyIhnmPwllWCSZuLv9DrD5pOexFfdlwXhf6cLzNpW6QhXD/Tf5KrqIPr9aOv8
+9xaEEXWh0vEby2kIsI2++ft+vfdIyxYw/wKqx0awTSnuBV1rG3z1dswX4BfoY66x
+Bz3KOVqlz9+mG/FTRQwrgPvR+qgLCHbuotxoGN7fzW+PI75hQG5JQAqhsC9sHjQH
+UrI21/VUNwzfw3v5pYsWuFb5bdQ3ASJetICQiMy7IW8WIQTRpm4aI7GCyZgPeIz7
+/MgqAV5zMG6/C/wLpPl/9e6Hf5wmXIUwpZNQbNZvpiCcyx9sXsHXaycOQVxn3McZ
+nYOUP9/mobl1tIeDQyTNbkxWjU0zzJl8XQsDZerb5098pg+x7oGIL7M1vn5s5JMl
+owROourqF88JEtOBxLMxlAM7X4hB48xKQ3Hu9hS1GdnqLKki4MqRGl4l5FUwyGOM
+GjyS3TzkfiDJNwQxybQiC9n57ij20ieNyLfuWCMLcNNnZUgZtnF6wCctoq/0ZIWu
+a7nvuA/XC2WW9YjEJJiWdy5109pqac+qWiY11HWy/nms4gpMdxVpT0RhrKGWq4o0
+M5q3ZElOoeN70UO3OSbU5EVrG7gB1GuwF9mTHUVlV0veSTw0axkta3FGT//XfSpD
+lRrCkyLzwq0M+UUHQAuYpAfobDlDdnxxOD2jm5GyTzak3GSVFfjW09QFVO6HlGp5
+01/jtzkUiS6nwoHHkfnyn0beZuR8X6KlcrzLB0VFgQFLmkSM9cSOgYhD0PTu9aHb
+hW1Hj9AO8lzggBQ=
+=Nt+N
+-----END PGP PUBLIC KEY BLOCK-----
+`
+
+const sigFromKeyWithExpiredCrossSig = `-----BEGIN PGP SIGNATURE-----
+
+wsDzBAABCgAGBYJfLAFsACEJEHwvqk35PDeyFiEEHdzhXwkhfO4vOzdgfC+qTfk8
+N7KiqwwAts4QGB7v9bABCC2qkTxJhmStC0wQMcHRcjL/qAiVnmasQWmvE9KVsdm3
+AaXd8mIx4a37/RRvr9dYrY2eE4uw72cMqPxNja2tvVXkHQvk1oEUqfkvbXs4ypKI
+NyeTWjXNOTZEbg0hbm3nMy+Wv7zgB1CEvAsEboLDJlhGqPcD+X8a6CJGrBGUBUrv
+KVmZr3U6vEzClz3DBLpoddCQseJRhT4YM1nKmBlZ5quh2LFgTSpajv5OsZheqt9y
+EZAPbqmLhDmWRQwGzkWHKceKS7nZ/ox2WK6OS7Ob8ZGZkM64iPo6/EGj5Yc19vQN
+AGiIaPEGszBBWlOpHTPhNm0LB0nMWqqaT87oNYwP8CQuuxDb6rKJ2lffCmZH27Lb
+UbQZcH8J+0UhpeaiadPZxH5ATJAcenmVtVVMLVOFnm+eIlxzov9ntpgGYt8hLdXB
+ITEG9mMgp3TGS9ZzSifMZ8UGtHdp9QdBg8NEVPFzDOMGxpc/Bftav7RRRuPiAER+
+7A5CBid5
+=aQkm
+-----END PGP SIGNATURE-----
+`