Improve robustness agains invalid input

.github/workflows/ios.yml

@@ -14,10 +14,10 @@ jobs:
     runs-on: macos-latest
 
     steps:
-      - name: Set up xcode 16.0.0
+      - name: Set up xcode
         uses: maxim-lobanov/setup-xcode@v1
         with:
-          xcode-version: 16.0.0
+          xcode-version: 26.0.1
         id: xcode
 
       - name: Set up Go 1.x

crypto/encrypt_decrypt_err_test.go

@@ -235,3 +235,16 @@ func testVerificationSuccess(
 		t.Fatal("Expected no signature failure")
 	}
 }
+
+func TestEncryptPlainDetachedSignatureNoSigningKey(t *testing.T) {
+	for _, material := range testMaterialForProfiles {
+		t.Run(material.profileName, func(t *testing.T) {
+			if _, err := material.pgp.Encryption().
+				Recipients(material.keyRingTestPublic).
+				PlainDetachedSignature().
+				New(); err == nil {
+				t.Fatal("should not create a plain detached signature handle without signing key")
+			}
+		})
+	}
+}

crypto/encryption_handle.go

@@ -110,7 +110,9 @@ func (eh *encryptionHandle) Encrypt(message []byte) (*PGPMessage, error) {
 		return nil, err
 	}
 	checksum := eh.armorChecksumRequired()
-	return pgpMessageBuffer.PGPMessageWithOptions(eh.PlainDetachedSignature, !checksum), nil
+	// PlainDetachedSignature is ignored if DetachedSignature is set.
+	isPlainDetachedSignature := eh.PlainDetachedSignature && !eh.DetachedSignature
+	return pgpMessageBuffer.PGPMessageWithOptions(isPlainDetachedSignature, !checksum), nil
 }
 
 // EncryptSessionKey encrypts a session key with the encryption handle.
@@ -153,7 +155,7 @@ func (eh *encryptionHandle) validate() error {
 		return errors.New("gopenpgp: no signing key but signing context provided")
 	}
 
-	if eh.SignKeyRing == nil && eh.DetachedSignature {
+	if eh.SignKeyRing == nil && (eh.DetachedSignature || eh.PlainDetachedSignature) {
 		return errors.New("gopenpgp: no signing key provided for detached signature")
 	}
 	return nil

crypto/encryption_session.go

@@ -38,7 +38,7 @@ Loop:
 				keys := unverifiedEntity.DecryptionKeys(p.KeyId, time.Time{}, &packet.Config{})
 				for _, key := range keys {
 					priv := key.PrivateKey
-					if priv.Encrypted {
+					if priv == nil || priv.Encrypted {
 						continue
 					}
 

crypto/key.go

@@ -238,7 +238,11 @@ func (key *Key) ArmorWithCustomHeaders(comment, version string) (string, error)
 		return "", err
 	}
 
-	return armor.ArmorWithTypeAndCustomHeadersChecksum(serialized, constants.PrivateKeyHeader, version, comment, !key.isV6())
+	header := constants.PublicKeyHeader
+	if key.IsPrivate() {
+		header = constants.PrivateKeyHeader
+	}
+	return armor.ArmorWithTypeAndCustomHeadersChecksum(serialized, header, version, comment, !key.isV6())
 }
 
 // GetArmoredPublicKey returns the armored public keys from this keyring.

crypto/key_test.go

@@ -106,6 +106,18 @@ func TestArmorKeysWithCustomHeader(t *testing.T) {
 
 	assert.Contains(t, armored, "Comment: "+comment)
 	assert.Contains(t, armored, "Version: "+version)
+	assert.True(t, strings.HasPrefix(armored, "-----BEGIN "+constants.PrivateKeyHeader+"-----"))
+
+	publicKey, err := keyTestRSA.ToPublic()
+	if err != nil {
+		t.Fatal("Could not extract the public key:", err)
+	}
+	armored, err = publicKey.ArmorWithCustomHeaders(comment, version)
+	if err != nil {
+		t.Fatal("Could not armor the public key:", err)
+	}
+	assert.True(t, strings.HasPrefix(armored, "-----BEGIN "+constants.PublicKeyHeader+"-----"))
+	assert.Contains(t, armored, "Comment: "+comment)
 }
 
 func TestLockUnlockKeys(t *testing.T) {

crypto/keyring.go

@@ -88,7 +88,7 @@ func (keyRing *KeyRing) GetKeys() []*Key {
 
 // GetKey returns the n-th openpgp key contained in this KeyRing.
 func (keyRing *KeyRing) GetKey(n int) (*Key, error) {
-	if n >= keyRing.CountEntities() {
+	if n < 0 || n >= keyRing.CountEntities() {
 		return nil, errors.New("gopenpgp: out of bound when fetching key")
 	}
 	return &Key{keyRing.entities[n]}, nil
@@ -253,6 +253,7 @@ func FilterExpiredKeys(contactKeys []*KeyRing) (filteredKeys []*KeyRing, err err
 				latestValid, err := subkey.LatestValidBindingSignature(now, &packet.Config{})
 				if err != nil {
 					hasExpired = true
+					continue
 				}
 				if subkey.PublicKey.KeyExpired(latestValid, now) {
 					hasExpired = true

crypto/keyring_test.go

@@ -1,14 +1,18 @@
 package crypto
 
 import (
+	"bytes"
 	"crypto/rsa"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/ProtonMail/go-crypto/openpgp/ecdh"
 	"github.com/ProtonMail/go-crypto/openpgp/eddsa"
+	"github.com/ProtonMail/go-crypto/openpgp/packet"
+	openpgp "github.com/ProtonMail/go-crypto/openpgp/v2"
 )
 
 var testSymmetricKey []byte
@@ -115,6 +119,22 @@ func TestFilterExpiredKeys(t *testing.T) {
 	assert.Exactly(t, unexpired[0].GetKeyIDs(), keyRingTestPrivate.GetKeyIDs())
 }
 
+func TestFilterExpiredKeysNoValidSubkeyBinding(t *testing.T) {
+	// Generate a key whose self-signatures (including the subkey binding) expired.
+	past := time.Now().Add(-48 * time.Hour)
+	config := &packet.Config{Time: func() time.Time { return past }, SigLifetimeSecs: 60}
+	entity, err := openpgp.NewEntity("expired", "", "expired@example.com", config)
+	require.NoError(t, err)
+	var serialized bytes.Buffer
+	require.NoError(t, entity.Serialize(&serialized))
+	keyRing, err := NewKeyRingFromBinary(serialized.Bytes())
+	require.NoError(t, err)
+
+	unexpired, err := FilterExpiredKeys([]*KeyRing{keyRing})
+	require.Error(t, err)
+	assert.Empty(t, unexpired)
+}
+
 func TestKeyIds(t *testing.T) {
 	keyIDs := keyRingTestPrivate.GetKeyIDs()
 	var assertKeyIDs = []uint64{4518840640391470884}
@@ -137,6 +157,9 @@ func TestMultipleKeyRing(t *testing.T) {
 	_, err = keyRingTestMultiple.GetKey(3)
 	require.Error(t, err)
 
+	_, err = keyRingTestMultiple.GetKey(-1)
+	require.Error(t, err)
+
 	singleKeyRing, err := keyRingTestMultiple.FirstKey()
 	if err != nil {
 		t.Fatal("Expected no error while filtering the first key, got:", err)

crypto/sessionkey_test.go

@@ -378,6 +378,32 @@ func TestAsymmetricKeyPacketDecryptionFailure(t *testing.T) {
 	assert.Error(t, err, "gopenpgp: unable to decrypt session key")
 }
 
+func TestAsymmetricKeyPacketDecryptionMissingPrivateKey(t *testing.T) {
+	privateKey, err := testPGP.KeyGeneration().AddUserId("test", "test@example.com").New().GenerateKey()
+	require.NoError(t, err)
+	publicKey, err := privateKey.ToPublic()
+	require.NoError(t, err)
+	encryptor, err := testPGP.Encryption().Recipient(publicKey).New()
+	require.NoError(t, err)
+	pgpMessage, err := encryptor.Encrypt([]byte("message"))
+	require.NoError(t, err)
+
+	// Public key used as a decryption key.
+	decryptor, err := testPGP.Decryption().DecryptionKey(publicKey).New()
+	require.NoError(t, err)
+	_, err = decryptor.DecryptSessionKey(pgpMessage.BinaryKeyPacket())
+	require.Error(t, err)
+
+	// Private key whose encryption subkey has no secret key material.
+	partialKey, err := privateKey.Copy()
+	require.NoError(t, err)
+	partialKey.entity.Subkeys[0].PrivateKey = nil
+	decryptor, err = testPGP.Decryption().DecryptionKey(partialKey).New()
+	require.NoError(t, err)
+	_, err = decryptor.DecryptSessionKey(pgpMessage.BinaryKeyPacket())
+	require.Error(t, err)
+}
+
 func TestSessionKeyAeadHandling(t *testing.T) {
 	pgp := PGPWithProfile(profile.Default())
 	profileAead := profile.Default()

mobile/mobile_stream.go

@@ -194,8 +194,9 @@ type KeyPacketSplitWriter struct {
 
 func NewKeyPacketSplitWriter(dataWriter crypto.Writer) *KeyPacketSplitWriter {
 	return &KeyPacketSplitWriter{
-		dataWriter: dataWriter,
-		keyPacket:  bytes.NewBuffer(nil),
+		dataWriter:           dataWriter,
+		keyPacket:            bytes.NewBuffer(nil),
+		encDetachedSignature: bytes.NewBuffer(nil),
 	}
 }
 

mobile/mobile_stream_test.go

@@ -237,9 +237,6 @@ func TestExplicitVerifyAllGoesWell(t *testing.T) {
 	if err = sigErr.SignatureError(); err != nil {
 		t.Fatalf("Got a signature error while verifying embedded sig: %v", err)
 	}
-	if err != nil {
-		t.Fatalf("Got an error while verifying embedded sig: %v", err)
-	}
 }
 
 func TestExplicitVerifyTooEarly(t *testing.T) {
@@ -343,3 +340,52 @@ func TestExplicitVerifyWrongVerifier(t *testing.T) {
 		t.Fatal("Signature error status was not SIGNATURE_NO_VERIFIER")
 	}
 }
+
+func TestKeyPacketSplitWriterDetachedSignature(t *testing.T) {
+	data := []byte("hello")
+	pgpHandle, pubKR, privKR, err := setUpTestKeyRing()
+	if err != nil {
+		t.Fatalf("Got an error while loading test key: %v", err)
+	}
+	defer privKR.ClearPrivateParams()
+	encHandle, err := pgpHandle.Encryption().Recipients(pubKR).SigningKeys(privKR).DetachedSignature().New()
+	if err != nil {
+		t.Fatalf("Got an error while creating encryption handle: %v", err)
+	}
+	var dataPackets bytes.Buffer
+	splitWriter := NewKeyPacketSplitWriter(&dataPackets)
+	ptWriter, err := encHandle.EncryptingWriter(splitWriter, crypto.Bytes)
+	if err != nil {
+		t.Fatalf("Got an error while creating encrypting writer: %v", err)
+	}
+	if _, err = ptWriter.Write(data); err != nil {
+		t.Fatalf("Got an error while writing data: %v", err)
+	}
+	if err = ptWriter.Close(); err != nil {
+		t.Fatalf("Got an error while closing encrypting writer: %v", err)
+	}
+	decHandle, _ := pgpHandle.Decryption().DecryptionKeys(privKR).VerificationKeys(pubKR).New()
+	splitReader := NewDetachedSignaturePGPSplitReader(
+		splitWriter.KeyPackets(),
+		bytes.NewReader(dataPackets.Bytes()),
+		splitWriter.EncryptedDetachedSignature(),
+	)
+	reader, err := decHandle.DecryptingReader(splitReader, crypto.Bytes)
+	if err != nil {
+		t.Fatalf("Got an error while decrypting stream data: %v", err)
+	}
+	decrypted, err := io.ReadAll(reader)
+	if err != nil {
+		t.Fatalf("Got an error while reading decrypted data: %v", err)
+	}
+	if !bytes.Equal(decrypted, data) {
+		t.Fatalf("Decrypted data does not match: %q", decrypted)
+	}
+	verifyResult, err := reader.VerifySignature()
+	if err != nil {
+		t.Fatalf("Got an error while verifying: %v", err)
+	}
+	if err = verifyResult.SignatureError(); err != nil {
+		t.Fatalf("Got a signature error while verifying detached sig: %v", err)
+	}
+}