From 6cfeba064e8eeb525918acb169282f3eceacb663 Mon Sep 17 00:00:00 2001 From: Paul Schaub Date: Thu, 22 Feb 2024 13:58:47 +0100 Subject: [PATCH] Sanitize key flags --- .../key/generation/OpenPgpKeyGenerator.kt | 120 ++++++++++++------ .../key/generation/OpenPgpKeyGeneratorTest.kt | 53 +++++++- 2 files changed, 131 insertions(+), 42 deletions(-) diff --git a/pgpainless-core/src/main/kotlin/org/pgpainless/key/generation/OpenPgpKeyGenerator.kt b/pgpainless-core/src/main/kotlin/org/pgpainless/key/generation/OpenPgpKeyGenerator.kt index efc01bb3..d93e79c0 100644 --- a/pgpainless-core/src/main/kotlin/org/pgpainless/key/generation/OpenPgpKeyGenerator.kt +++ b/pgpainless-core/src/main/kotlin/org/pgpainless/key/generation/OpenPgpKeyGenerator.kt @@ -20,6 +20,7 @@ import org.pgpainless.algorithm.AlgorithmSuite import org.pgpainless.algorithm.CertificationType import org.pgpainless.algorithm.HashAlgorithm import org.pgpainless.algorithm.KeyFlag +import org.pgpainless.algorithm.PublicKeyAlgorithm import org.pgpainless.implementation.ImplementationFactory import org.pgpainless.key.generation.DefinePrimaryKey.PrimaryKeyBuilder import org.pgpainless.key.generation.DefineSubkeys.SubkeyBuilder @@ -169,7 +170,13 @@ internal constructor(val policy: Policy, val creationTime: Date, val preferences keyFlags: List? = listOf(KeyFlag.CERTIFY_OTHER), creationTime: Date = this.creationTime, block: PrimaryKeyBlock? = null - ): O = doSetPrimaryKey(type, keyFlags, creationTime, block) + ): O { + require(type.canCertify) { + "Primary key cannot use algorithm ${type.algorithm} because it needs to be " + + "signing capable." + } + return doSetPrimaryKey(type, keyFlags, creationTime, block) + } fun setPrimaryKey(type: KeyType, block: PrimaryKeyBlock?): O = setPrimaryKey(type, listOf(KeyFlag.CERTIFY_OTHER), this.creationTime, block) @@ -200,6 +207,10 @@ internal constructor(val policy: Policy, val creationTime: Date, val preferences // Do nothing } + protected open fun sanitizeKeyFlags(algorithm: PublicKeyAlgorithm, keyFlags: List?) { + // Do nothing + } + /** * Function that can be applied to the primary key. * @@ -407,14 +418,23 @@ internal constructor( @JvmOverloads fun addSubkey( type: KeyType, + flags: List? = null, creationTime: Date = this.creationTime, block: SubkeyBlock? = null ): B = apply { + sanitizeKeyFlags(type.algorithm, flags) sanitizeSubkeyCreationTime(creationTime, primaryKey) var subkey = generateSubkey(type, creationTime) - subkey = invokeOnSubkey(subkey, block) + val subkeyBlock = + block + ?: { + addBindingSignature( + SelfSignatureSubpackets.applyHashed { flags?.let { setKeyFlags(it) } }, + bindingTime = creationTime) + } + subkey = invokeOnSubkey(subkey, subkeyBlock) subkeys.add(subkey) } as B @@ -490,6 +510,10 @@ internal constructor( // Do nothing } + protected open fun sanitizeKeyFlags(algorithm: PublicKeyAlgorithm, keyFlags: List?) { + // Do nothing + } + /** * Function that can be applied to subkeys. * @@ -723,8 +747,6 @@ internal constructor(policy: Policy, creationTime: Date, preferences: AlgorithmS creationTime: Date, block: PrimaryKeyBlock? ): OpinionatedDefineSubkeysV4 { - // Check algorithm is signing capable - require(type.algorithm.isSigningCapable()) { "Primary Key MUST be capable of signing." } // Check key strength require(policy.publicKeyAlgorithmPolicy.isAcceptable(type.algorithm, type.bitStrength)) { @@ -732,6 +754,9 @@ internal constructor(policy: Policy, creationTime: Date, preferences: AlgorithmS " for the current public key algorithm policy." } + // Sanitize key flags + sanitizeKeyFlags(type.algorithm, keyFlags) + // Remember flags for DK and UID signatures this.keyFlags = keyFlags @@ -762,6 +787,27 @@ internal constructor(policy: Policy, creationTime: Date, preferences: AlgorithmS "key was created at ${primaryKey.publicKey.creationTime.formatUTC()}." } } + + override fun sanitizeKeyFlags(algorithm: PublicKeyAlgorithm, keyFlags: List?) { + keyFlags?.forEach { flag -> + when (flag) { + KeyFlag.CERTIFY_OTHER, + KeyFlag.SIGN_DATA, + KeyFlag.AUTHENTICATION -> + require(algorithm.isSigningCapable()) { + "Primary key cannot carry key flag $flag because the " + + "algorithm $algorithm is not signing capable." + } + KeyFlag.ENCRYPT_COMMS, + KeyFlag.ENCRYPT_STORAGE -> + require(algorithm.isEncryptionCapable()) { + "Primary key cannot carry key flag $flag because the " + + "algorithm $algorithm is not encryption capable." + } + else -> {} // no special requirements for SPLIT and SHARED + } + } + } } /** @@ -842,11 +888,10 @@ internal constructor(primaryKey: PGPKeyPair, policy: Policy, creationTime: Date) fun addSigningSubkey( type: KeyType, creationTime: Date = this.creationTime, - block: SubkeyBlock? = { - addBindingSignature( - SelfSignatureSubpackets.applyHashed { setKeyFlags(KeyFlag.SIGN_DATA) }) - } - ) = addSubkey(type, creationTime, block) + block: SubkeyBlock? = null + ): OpinionatedDefineSubkeysV4 { + return addSubkey(type, listOf(KeyFlag.SIGN_DATA), creationTime, block) + } /** * Add a subkey for signing messages to the OpenPGP key. @@ -868,13 +913,11 @@ internal constructor(primaryKey: PGPKeyPair, policy: Policy, creationTime: Date) fun addEncryptionSubkey( type: KeyType, creationTime: Date = this.creationTime, - block: SubkeyBlock? = { - addBindingSignature( - SelfSignatureSubpackets.applyHashed { - setKeyFlags(KeyFlag.ENCRYPT_COMMS, KeyFlag.ENCRYPT_STORAGE) - }) - } - ) = addSubkey(type, creationTime, block) + block: SubkeyBlock? = null, + ): OpinionatedDefineSubkeysV4 { + return addSubkey( + type, listOf(KeyFlag.ENCRYPT_COMMS, KeyFlag.ENCRYPT_STORAGE), creationTime, block) + } /** * Add a subkey for message encryption to the OpenPGP key. @@ -907,6 +950,27 @@ internal constructor(primaryKey: PGPKeyPair, policy: Policy, creationTime: Date) "Primary key was created at ${primaryKey.publicKey.creationTime.formatUTC()}." } } + + override fun sanitizeKeyFlags(algorithm: PublicKeyAlgorithm, keyFlags: List?) { + keyFlags?.forEach { flag -> + when (flag) { + KeyFlag.CERTIFY_OTHER, + KeyFlag.SIGN_DATA, + KeyFlag.AUTHENTICATION -> + require(algorithm.isSigningCapable()) { + "Subkey cannot carry key flag $flag because the " + + "algorithm $algorithm is not signing capable." + } + KeyFlag.ENCRYPT_COMMS, + KeyFlag.ENCRYPT_STORAGE -> + require(algorithm.isEncryptionCapable()) { + "Subkey cannot carry key flag $flag because the " + + "algorithm $algorithm is not encryption capable." + } + else -> {} // no special requirements for SPLIT and SHARED + } + } + } } /** @@ -1078,17 +1142,9 @@ class OpenPgpKeyTemplates private constructor() { SelfSignatureSubpackets.applyHashed { setKeyFlags(KeyFlag.CERTIFY_OTHER) }) } // singing key - .addSubkey(KeyType.EDDSA(EdDSACurve._Ed25519)) { - addBindingSignature( - SelfSignatureSubpackets.applyHashed { setKeyFlags(KeyFlag.SIGN_DATA) }) - } + .addSigningSubkey(KeyType.EDDSA(EdDSACurve._Ed25519)) // encryption key - .addSubkey(KeyType.XDH(XDHSpec._X25519)) { - addBindingSignature( - SelfSignatureSubpackets.applyHashed { - setKeyFlags(KeyFlag.ENCRYPT_COMMS, KeyFlag.ENCRYPT_STORAGE) - }) - } + .addEncryptionSubkey(KeyType.XDH(XDHSpec._X25519)) .build() /** @@ -1122,17 +1178,9 @@ class OpenPgpKeyTemplates private constructor() { SelfSignatureSubpackets.applyHashed { setKeyFlags(KeyFlag.CERTIFY_OTHER) }) } // signing key - .addSubkey(KeyType.RSA(length)) { - addBindingSignature( - SelfSignatureSubpackets.applyHashed { setKeyFlags(KeyFlag.SIGN_DATA) }) - } + .addSigningSubkey(KeyType.RSA(length)) // encryption key - .addSubkey(KeyType.RSA(length)) { - addBindingSignature( - SelfSignatureSubpackets.applyHashed { - setKeyFlags(KeyFlag.ENCRYPT_COMMS, KeyFlag.ENCRYPT_STORAGE) - }) - } + .addEncryptionSubkey(KeyType.RSA(length)) .build() /** diff --git a/pgpainless-core/src/test/kotlin/org/pgpainless/key/generation/OpenPgpKeyGeneratorTest.kt b/pgpainless-core/src/test/kotlin/org/pgpainless/key/generation/OpenPgpKeyGeneratorTest.kt index db14e8ad..c716665f 100644 --- a/pgpainless-core/src/test/kotlin/org/pgpainless/key/generation/OpenPgpKeyGeneratorTest.kt +++ b/pgpainless-core/src/test/kotlin/org/pgpainless/key/generation/OpenPgpKeyGeneratorTest.kt @@ -109,10 +109,7 @@ class OpenPgpKeyGeneratorTest { val key = OpenPgpKeyGenerator.buildV4Key() .setPrimaryKey(KeyType.EDDSA(EdDSACurve._Ed25519)) - .addSubkey(KeyType.EDDSA(EdDSACurve._Ed25519)) { - addBindingSignature( - SelfSignatureSubpackets.applyHashed { setKeyFlags(KeyFlag.SIGN_DATA) }) - } + .addSubkey(KeyType.EDDSA(EdDSACurve._Ed25519), listOf(KeyFlag.SIGN_DATA)) .build() assertFalse( @@ -376,7 +373,7 @@ class OpenPgpKeyGeneratorTest { assertThrows { OpenPgpKeyGenerator.buildV4Key(policy, t1) .setPrimaryKey(KeyType.EDDSA(EdDSACurve._Ed25519)) - .addSubkey(KeyType.XDH(XDHSpec._X25519), t0) + .addSubkey(KeyType.XDH(XDHSpec._X25519), null, t0) } } @@ -389,7 +386,7 @@ class OpenPgpKeyGeneratorTest { OpenPgpKeyGenerator.buildV4Key(policy, t1) .unopinionated() .setPrimaryKey(KeyType.EDDSA(EdDSACurve._Ed25519)) - .addSubkey(KeyType.XDH(XDHSpec._X25519), t0) + .addSubkey(KeyType.XDH(XDHSpec._X25519), null, t0) } @Test @@ -442,6 +439,50 @@ class OpenPgpKeyGeneratorTest { } } + @Test + fun `opinionated set primary key to encryption key fails`() { + val policy = Policy() + + assertThrows { + OpenPgpKeyGenerator.buildV4Key(policy).setPrimaryKey(KeyType.XDH(XDHSpec._X25519)) + } + } + + @Test + fun `opinionated set primary key to sign-only algorithm but with encryption flag fails`() { + val policy = Policy() + + assertThrows { + OpenPgpKeyGenerator.buildV4Key(policy) + .setPrimaryKey( + KeyType.EDDSA(EdDSACurve._Ed25519), + listOf(KeyFlag.CERTIFY_OTHER, KeyFlag.ENCRYPT_STORAGE)) + } + } + + @Test + fun `unopinionated set primary key to sign-only algorithm but with encryption flag is okay`() { + val policy = Policy() + + OpenPgpKeyGenerator.buildV4Key(policy) + .unopinionated() + .setPrimaryKey( + KeyType.EDDSA(EdDSACurve._Ed25519), + listOf(KeyFlag.CERTIFY_OTHER, KeyFlag.ENCRYPT_STORAGE)) + } + + @Test + fun `opinionated add encryption-only subkey with additional sign flag fails`() { + val policy = Policy() + + assertThrows { + OpenPgpKeyGenerator.buildV4Key(policy) + .setPrimaryKey(KeyType.EDDSA(EdDSACurve._Ed25519)) + .addSubkey( + KeyType.XDH(XDHSpec._X25519), listOf(KeyFlag.ENCRYPT_COMMS, KeyFlag.SIGN_DATA)) + } + } + @Test fun `add image attribute to key`() { // smallest JPEG according to https://stackoverflow.com/a/2349470/11150851