Skip to content

Commit

Permalink
Fix for issue neuhalje#50, where we cannot encrypt if a key doesn't h…
Browse files Browse the repository at this point in the history 
…ave a KeyFlags subpacket
  • Loading branch information
@mdesmons
mdesmons committed Apr 28, 2020
1 parent 607e866 commit b899853
Show file tree
Hide file tree
Showing 5 changed files with 165 additions and 35 deletions.
88 changes: 67 additions & 21 deletions ...lfen/projects/crypto/bouncycastle/openpgp/keys/callbacks/Rfc4880KeySelectionStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,14 @@

import java.io.IOException;
import java.time.Instant;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Set;
import java.util.*;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;
import javax.annotation.Nullable;
import name.neuhalfen.projects.crypto.bouncycastle.openpgp.keys.generation.KeyFlag;
import name.neuhalfen.projects.crypto.bouncycastle.openpgp.keys.keyrings.KeyringConfig;
import org.bouncycastle.bcpg.PublicKeyAlgorithmTags;
import org.bouncycastle.openpgp.PGPException;
import org.bouncycastle.openpgp.PGPPublicKey;
import org.bouncycastle.openpgp.PGPPublicKeyRing;
Expand All @@ -33,6 +32,20 @@ public class Rfc4880KeySelectionStrategy implements KeySelectionStrategy {
private final boolean ignoreCase;
private final boolean matchPartial;

// list of algorithms that can be used for encryption
private final List<Integer> encryptionAlgorithms = Arrays.asList(PublicKeyAlgorithmTags.RSA_GENERAL,
PublicKeyAlgorithmTags.RSA_ENCRYPT,
PublicKeyAlgorithmTags.ECDH,
PublicKeyAlgorithmTags.ELGAMAL_ENCRYPT,
PublicKeyAlgorithmTags.ELGAMAL_GENERAL);

// list of algorithms that can be used for signing
private final List<Integer> signatureAlgorithms = Arrays.asList(PublicKeyAlgorithmTags.RSA_GENERAL,
PublicKeyAlgorithmTags.RSA_SIGN,
PublicKeyAlgorithmTags.DSA,
PublicKeyAlgorithmTags.ECDSA,
PublicKeyAlgorithmTags.EDDSA);

/**
* Construct an instance with matchPartial and ignoreCase set to true.
*
Expand Down Expand Up @@ -221,39 +234,72 @@ protected boolean isExpired(PGPPublicKey pubKey) {
return isExpired;
}


/**
* Checks if a public key may be used for encryption. This uses the key KeyFlags subpacket content by default,
* falling back to the key algorithm if there isn't any KeyFlags subpacket
* @param publicKey public key to examine
* @return true if the key can be used for encryption
*/
protected boolean isEncryptionKey(PGPPublicKey publicKey) {
requireNonNull(publicKey, "publicKey must not be null");
boolean isEncryptionKey = false;

final Optional<Set<KeyFlag>> optionalKeyFlags = extractPublicKeyFlags(publicKey);

/* If the key contains a KeyFlag subpacket, we extract its flags to determine if the
key can be used for encryption
*/
if (optionalKeyFlags.isPresent()) { // NOPMD:LawOfDemeter
final Set<KeyFlag> keyFlags = optionalKeyFlags.get();
final boolean canEncryptCommunication = keyFlags // NOPMD:LawOfDemeter
.contains(KeyFlag.ENCRYPT_COMMS);
final boolean canEncryptStorage = keyFlags // NOPMD:LawOfDemeter
.contains(KeyFlag.ENCRYPT_STORAGE);
isEncryptionKey = canEncryptCommunication || canEncryptStorage;
} else {
/* If the key doesn't contain any KeyFlag subpacket, check the key algorithm.
This is what GPG does (g10/misc.c) and lets us encrypt with keys that don't contain a KeyFlag subpacket
*/
isEncryptionKey = encryptionAlgorithms.contains(publicKey.getAlgorithm());
}

final Set<KeyFlag> keyFlags = extractPublicKeyFlags(publicKey);
return isEncryptionKey;
}

final boolean canEncryptCommunication = keyFlags // NOPMD:LawOfDemeter
.contains(KeyFlag.ENCRYPT_COMMS);
final boolean canEncryptStorage = keyFlags // NOPMD:LawOfDemeter
.contains(KeyFlag.ENCRYPT_STORAGE);
protected boolean isVerificationKey(PGPPublicKey publicKey) {
requireNonNull(publicKey, "publicKey must not be null");

return canEncryptCommunication || canEncryptStorage;
}
final Optional<Set<KeyFlag>> optionalKeyFlags = extractPublicKeyFlags(publicKey);
boolean isVerificationKey;

protected boolean isVerificationKey(PGPPublicKey pubKey) {
final boolean isVerficationKey =
extractPublicKeyFlags(pubKey).contains(KeyFlag.SIGN_DATA); // NOPMD:LawOfDemeter
/* If the key contains a KeyFlag subpacket, we extract its flags to determine if the
key can be used for signing
*/
if (optionalKeyFlags.isPresent()) { // NOPMD:LawOfDemeter
isVerificationKey = optionalKeyFlags.get().contains(KeyFlag.SIGN_DATA); // NOPMD:LawOfDemeter
} else {
/* If the key doesn't contain any KeyFlag subpacket, check the key algorithm.
This is what GPG does (g10/misc.c) and lets us signing with keys that don't contain a KeyFlag subpacket
*/
isVerificationKey =signatureAlgorithms.contains(publicKey.getAlgorithm());
}

if (!isVerficationKey) {
if (!isVerificationKey) {
LOGGER.trace("Skipping pubkey {} (no signing key)",
Long.toHexString(pubKey.getKeyID()));
Long.toHexString(publicKey.getKeyID()));
}
return isVerficationKey;

return isVerificationKey;
}


protected boolean isRevoked(PGPPublicKey pubKey) {
requireNonNull(pubKey, "pubKey must not be null");
protected boolean isRevoked(PGPPublicKey publicKey) {
requireNonNull(publicKey, "pubKey must not be null");

final boolean hasRevocation = pubKey.hasRevocation();
final boolean hasRevocation = publicKey.hasRevocation();
if (hasRevocation) {
LOGGER.trace("Skipping pubkey {} (revoked)",
Long.toHexString(pubKey.getKeyID()));
Long.toHexString(publicKey.getKeyID()));
}
return hasRevocation;
}
Expand Down
23 changes: 19 additions & 4 deletions ...ain/java/name/neuhalfen/projects/crypto/bouncycastle/openpgp/keys/generation/KeyFlag.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@

import java.util.EnumSet;
import java.util.Iterator;
import java.util.Optional;
import java.util.Set;

import org.bouncycastle.bcpg.SignatureSubpacketTags;
import org.bouncycastle.bcpg.sig.KeyFlags;
import org.bouncycastle.openpgp.PGPPublicKey;
import org.bouncycastle.openpgp.PGPSignature;
Expand Down Expand Up @@ -92,21 +95,33 @@ public static Set<KeyFlag> fromInteger(int bitmask) {
}

@SuppressWarnings({"PMD.LawOfDemeter"})
public static Set<KeyFlag> extractPublicKeyFlags(PGPPublicKey publicKey) {
public static Optional<Set<KeyFlag>> extractPublicKeyFlags(PGPPublicKey publicKey) {
requireNonNull(publicKey, "publicKey must not be null");

int aggregatedKeyFlags = 0;
boolean hasKeyFlags = false;
Optional<Set<KeyFlag>> publicKeyFlags;

final Iterator<PGPSignature> directKeySignatures = publicKey.getSignatures();

while (directKeySignatures.hasNext()) {
final PGPSignature signature = directKeySignatures.next();
final PGPSignatureSubpacketVector hashedSubPackets = signature.getHashedSubPackets();

final int keyFlags = hashedSubPackets.getKeyFlags();
aggregatedKeyFlags |= keyFlags;
if (hashedSubPackets.hasSubpacket(SignatureSubpacketTags.KEY_FLAGS)) {
hasKeyFlags = true;
final int keyFlags = hashedSubPackets.getKeyFlags();
aggregatedKeyFlags |= keyFlags;
}
}
return fromInteger(aggregatedKeyFlags);

if (hasKeyFlags) {
publicKeyFlags = Optional.of(fromInteger(aggregatedKeyFlags));
} else {
publicKeyFlags = Optional.empty();
}

return publicKeyFlags;
}

public int getFlag() {
Expand Down
47 changes: 44 additions & 3 deletions ...me/neuhalfen/projects/crypto/bouncycastle/openpgp/encrypting/EncryptionScenariosTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,8 @@

import name.neuhalfen.projects.crypto.bouncycastle.openpgp.BouncyGPG;
import name.neuhalfen.projects.crypto.bouncycastle.openpgp.algorithms.DefaultPGPAlgorithmSuites;
import name.neuhalfen.projects.crypto.bouncycastle.openpgp.keys.callbacks.KeyringConfigCallbacks;
import name.neuhalfen.projects.crypto.bouncycastle.openpgp.keys.keyrings.InMemoryKeyring;
import name.neuhalfen.projects.crypto.bouncycastle.openpgp.keys.keyrings.KeyringConfig;
import name.neuhalfen.projects.crypto.bouncycastle.openpgp.keys.keyrings.KeyringConfigs;
import name.neuhalfen.projects.crypto.bouncycastle.openpgp.testtooling.Configs;
import name.neuhalfen.projects.crypto.bouncycastle.openpgp.testtooling.ExampleMessages;
import org.bouncycastle.openpgp.PGPException;
import org.bouncycastle.util.io.Streams;
Expand All @@ -19,6 +16,7 @@
import java.time.Instant;

import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertNotNull;

/**
* Tests several encryption scenarios. As usual, most are integration style tests.
Expand Down Expand Up @@ -75,4 +73,47 @@ public void encrypt_masterKeyWithoutSubkeys_works()
assertArrayEquals(ExampleMessages.IMPORTANT_QUOTE_TEXT.getBytes(), plainBA.toByteArray());
}

@Test
public void encrypt_masterKeyWithoutSubkeysOrKeyFlags_works()
throws IOException, PGPException, NoSuchAlgorithmException, SignatureException, NoSuchProviderException {

// Reported in https://github.com/neuhalje/bouncy-gpg/issues/50

/* We test that a key that doesn't contain any KeyFlags subpacket can still be used for encrypting,
by checking the key algorithm, like GPG does
*/

// keyring
InMemoryKeyring sendersKeyring = KeyringConfigs.forGpgExportedKeys(keyId -> null);
sendersKeyring.addPublicKey(ExampleMessages.ONLY_MASTER_KEY_PUBKEY_NO_KEY_FLAGS.getBytes());

// encrypt
ByteArrayOutputStream result = new ByteArrayOutputStream();
BufferedOutputStream bufferedOutputStream = new BufferedOutputStream(result);

final OutputStream outputStream = BouncyGPG
.encryptToStream()
.withConfig(sendersKeyring)
.setReferenceDateForKeyValidityTo(Instant.MAX)
.withAlgorithms(DefaultPGPAlgorithmSuites.strongSuite())
.toRecipient(ExampleMessages.ONLY_MASTER_KEY_UID_NO_KEY_FLAGS)
.andDoNotSign()
.binaryOutput()
.andWriteTo(bufferedOutputStream);

final InputStream is = new ByteArrayInputStream(
ExampleMessages.IMPORTANT_QUOTE_TEXT.getBytes());
Streams.pipeAll(is, outputStream);
outputStream.close();
bufferedOutputStream.close();
is.close();

/* test that the data can be encrypted.
If the test fails, Bouncy-PGP would complain that it can't find a public key to encrypt with
*/

final byte[] ciphertext = result.toByteArray();
result.close();
assertNotNull(ciphertext);
}
}
29 changes: 29 additions & 0 deletions ...java/name/neuhalfen/projects/crypto/bouncycastle/openpgp/testtooling/ExampleMessages.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,34 @@ public class ExampleMessages {
"=8wl4\n" +
"-----END PGP PRIVATE KEY BLOCK-----\n";

public final static long ONLY_MASTER_KEY_ID_NO_KEY_FLAGS = 0x9E698FBA9F857349L;
public final static String ONLY_MASTER_KEY_UID_NO_KEY_FLAGS = "support@anzgcis.com";
public final static String ONLY_MASTER_KEY_PASSPHRASE_NO_KEY_FLAGS = "";
public final static String ONLY_MASTER_KEY_PUBKEY_NO_KEY_FLAGS= "-----BEGIN PGP PUBLIC KEY BLOCK-----\n" +
"Version: SecureBlackbox 15\n" +
"\n" +
"xsBNBFtrhAABCACbJgPWFmodLq0cqIwiZG8hnHusSBA1nan8OQk2Dr+l37smSjO7\n" +
"TT0N/G8er79lgVyynYXXHH7EfY7tvvWPS5N55NvYiLxRYRPgFgy+cD6TsSn8W0qD\n" +
"zwCf5Jw5ZeU4pdRo6hN1Qgl6IcETQWZOsob3BWmg2aLnl+1SyY3zh0jW6z6Wn4En\n" +
"tpPj4iELe6o1SNcmC2A2fIVUoMlBHa+Mcumw+kqv4vsA6xJmpDYniBaW/asVaIBl\n" +
"4bA0Iux5Osr83pNJOBHpLk9ozyMF0rtqyyvA8l3SiLA2jyeXoBEQ9mEJi+G0/PBI\n" +
"mAoMs3FXlu4Dqdh6jk0vHDk261tMq8wqas7nABEBAAHCwGIEHwEIABYFCRLMAwAF\n" +
"AltreqYJEJ5pj7qfhXNJAACwjQgAktbLedTzTJmRgdgQsH3uakoFmi7Pc2Q/N9J7\n" +
"q8wJhBcxUUILQQck9fmzM4XOgAJmX0Sn+3xnkUvdnSP8EGd5SPezoDo29Udrm99z\n" +
"LV//9N5p4febzQwAhQJg3rGz5oDt5VMwTdSxhxPAKvkIV1QAQFqeZn6wO26Voz3h\n" +
"wfLt3xX904pomtCeakdRu+akqTBKS28Q6atHauHAAzlr1cfjLcLHdqI1y1zUbAyE\n" +
"dmYhpiIM3sr1ZvjaSRQv+vF/l5z88EVE5Ag4U5Mdw8X7R9WCnEk5ojOOveNYIsq2\n" +
"rjh9rAvD6aNBK4v+1wx9P1zSv227SojxX+AB103GSYitBSLTUs0oRmlsZWFjdGl2\n" +
"ZSBTdXBwb3J0IDxzdXBwb3J0QGFuemdjaXMuY29tPsLAXwQQAQgAEwUCW2wHRgIZ\n" +
"AQkQnmmPup+Fc0kAAGN8CACR82NV7nMGOm/crnu7k2dCHCoiTsucerG1tDFyCNYx\n" +
"3w0WATvSBS6XRwLfl6IaVbjpauZk+bH6NVgo9tpwKwGn7Wh9takHMsuRPoNiudqf\n" +
"S/duYt/ugOhlajfe3EFuGlt62j8pVT5o9bgCZQNEUgkGzwwz+j/saWHsI9fW26Ri\n" +
"W/tyJk0IICLf2aEJOi63PsllRQ+CtimvZkRalvcCuS19JNcYZlz3GyM/+z3LiYMi\n" +
"b1s+vDO4ZJgka+nTWHL1VrbctH79n23hmV1pK7LXAl8mZlo0wUU+R+01WumSVSQZ\n" +
"XGs5vAe6nG216ghBcNNd0UbQMZXwuIQlOtN5CSwoHdp6\n" +
"=qWOE\n" +
"-----END PGP PUBLIC KEY BLOCK-----\n";

public final static Map<Long, char[]> ALL_KEYRINGS_PASSWORDS = BUILD_ALL_KEYRINGS_PASSWORDS();
/**
* Encrypted-To: recipient@example.com Signed-By: sender@example.com Compressed: false
Expand Down Expand Up @@ -738,6 +766,7 @@ private static Map<Long, char[]> BUILD_ALL_KEYRINGS_PASSWORDS() {
m.put(ExampleMessages.PUBKEY_ID_RECIPIENT, "recipient".toCharArray());

m.put(ExampleMessages.ONLY_MASTER_KEY_ID, null);
m.put(ExampleMessages.ONLY_MASTER_KEY_ID_NO_KEY_FLAGS, null);
return Collections.unmodifiableMap(m);
}

Expand Down
13 changes: 6 additions & 7 deletions ...projects/crypto/bouncycastle/openpgp/testtooling/matcher/SecretKeyringKeyRoleMatcher.java
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
package name.neuhalfen.projects.crypto.bouncycastle.openpgp.testtooling.matcher;

import java.util.Collections;
import java.util.EnumSet;
import java.util.Iterator;
import java.util.Set;
import java.util.*;

import name.neuhalfen.projects.crypto.bouncycastle.openpgp.keys.generation.KeyFlag;
import org.bouncycastle.openpgp.PGPPublicKey;
import org.bouncycastle.openpgp.PGPSecretKey;
Expand Down Expand Up @@ -54,21 +52,22 @@ EnumSet<KeyRole> parseKeyRing(final PGPSecretKeyRing ring) {
EnumSet<KeyRole> parseKey(final PGPSecretKey item) {
final EnumSet<KeyRole> found = EnumSet.noneOf(KeyRole.class);
final PGPPublicKey publicKey = item.getPublicKey();
final Set<KeyFlag> keyFlags = KeyFlag.extractPublicKeyFlags(publicKey);
final Optional<Set<KeyFlag>> keyFlags = KeyFlag.extractPublicKeyFlags(publicKey);

if (item.isMasterKey()) {
found.add(KeyRole.MASTER);
}

if (item.isSigningKey()) {
if (keyFlags.contains(KeyFlag.SIGN_DATA)) {
if (keyFlags.isPresent() && keyFlags.get().contains(KeyFlag.SIGN_DATA)) {
found.add(KeyRole.SIGNING);
}
}

if (item.getPublicKey().isEncryptionKey()) {
final boolean hasEncryptionFlags =
keyFlags.contains(KeyFlag.ENCRYPT_STORAGE) || keyFlags.contains(KeyFlag.ENCRYPT_COMMS);
keyFlags.isPresent() && keyFlags.get().contains(KeyFlag.ENCRYPT_STORAGE) ||
keyFlags.isPresent() && keyFlags.get().contains(KeyFlag.ENCRYPT_COMMS);
if (hasEncryptionFlags) {
found.add(KeyRole.ENCRYPTION);
}
Expand Down

0 comments on commit b899853

Please sign in to comment.