[BUG] Passwords created with the PGPainless backend are not encrypted

Question

[BUG] Passwords created with the PGPainless backend are not encrypted

kevinnls opened this issue 4 years ago · 28 comments

Describe the bug

Under the PGPainless (new PGP back-end), passwords created using the app are not encrypted with the keyring. They are being saved as PGP message Compressed Data (old).

Steps to reproduce

Enable new PGP backend and import a key (with passphrase)
Tap on the + button
Create a password [1]
Quit app/return to homescreen
Open the created password [1]
Ignore password prompt and tap OK

Expected behavior

The password should be encrypted with the added PGP key (or fail with error if it isn't able to)

Screenshots

CLI demo in GPG without any installed keyring

Device information

Device: Redmi 9 Power / MS2010J19SI
OS: MIUI Global 12.5.7 / Android11
App version: SNAPSHOT f5cf1ca

Additional context

No response

Answer 1 · 2022-04-28T18:51:46.000Z

Hmm this is definitely a severe bug, thanks for the report! Unfortunately I'll be travelling this weekend so I can realistically only work on this in 5 or so days.

Answer 2 · 2022-04-28T19:28:43.000Z

hey, no worries 😄

take your time, comes with the territory when one uses pre-release

but if you could tell me the path/file i should fish around in, i might be able to look into it a bit to create a possible fix.

i don't think i will be able to code the fix, it'll be theoretical. i'll be happy to do a little bit of the studying for the team? :)

Answer 3 · 2022-04-28T19:35:28.000Z

The encryption happens in this class, you can reference the PGPainless examples for the correct way to do this.

Answer 4 · 2022-04-28T20:23:47.000Z

i'll take a look over the weekend. safe travels! :)

Answer 5 · 2022-04-29T07:10:41.000Z

I took a stab at this here to not much success.

The current status now is:

Files encrypted by APS snapshots: PGP message Compressed Data (old)
Files encrypted by APS @ 8c4c17d: PGP message Public-Key Encrypted Session Key (old)
Files encrypted by pass CLI: PGP RSA encrypted session key - keyid: BA7B6D36 801E03D1 RSA (Encrypt or Sign) 4096b .
Files encrypted by OpenKeychain: PGP RSA encrypted session key - keyid: 17CE1531 71810787 RSA (Encrypt or Sign) 4096b

Answer 6 · 2022-04-29T07:17:25.000Z

FWIW I'm pretty confident the fix I tried made things worse because both the tests for PGPainlessCryptoHandler now fail :'D

@vanitasvitae can you review this method and let me know what I'm doing wrong? Thanks! 🙏🏼

Answer 7 · 2022-04-29T17:48:18.000Z

Ouch, this sounds like a bug indeed.

What I could imaging what happened is, that your set of OpenPGP keys was empty, so this line never actually added any recipients.

I'm wondering, why are you de-armoring the keys yourselves in this block? It should be sufficient to do

        val publicKeyRingCollection =
          pubKeysStream.use {
            PGPainless.readKeyRing().publicKeyRingCollection(pubKeysStream)
          }

If this does not fix your issue, please let me know.

Answer 8 · 2022-04-29T18:23:24.000Z

Oh, one thing I would definitely recommend is to add the equivalent of

encryptionStream.close();
EncryptionResult result = encryptionStream.getResult();
assert result.isEncrypted(); // throw if false

to your encryption method. That way you can guarantee that the password are infact getting encrypted.

Answer 9 · 2022-05-01T20:02:09.000Z

Ouch, this sounds like a bug indeed.

What I could imaging what happened is, that your set of OpenPGP keys was empty, so this line never actually added any recipients.

I'm wondering, why are you de-armoring the keys yourselves in this block? It should be sufficient to do
        val publicKeyRingCollection =
          pubKeysStream.use {
            PGPainless.readKeyRing().publicKeyRingCollection(pubKeysStream)
          }
If this does not fix your issue, please let me know.

Finally got home and had some time to try this out. I've made the relevant change here and have confirmed through additional logging that there definitely is at least one key being passed to the method.

Oh, one thing I would definitely recommend is to add the equivalent of
encryptionStream.close();
EncryptionResult result = encryptionStream.getResult();
assert result.isEncrypted(); // throw if false
to your encryption method. That way you can guarantee that the password are infact getting encrypted.

I tried adding this assertion but it seems the isEncrypted method is only available for OpenPgpMetadata which is returned after decryption, and EncryptionResult instead exposes isEncryptedFor(PGPPublicKeyRing) which I ended up using.

The files are still unencrypted, as confirmed by gpg --list-packets.

➜ gpg --list-packets broken.gpg 
# off=0 ctb=a3 tag=8 hlen=1 plen=0 indeterminate
:compressed packet: algo=1
# off=2 ctb=cb tag=11 hlen=2 plen=10 new-ctb
:literal data packet:
	mode b (62), created 0, name="",
	raw data: 4 bytes

For an actually encrypted file, the output looks like this:

➜ gpg --list-packets working.gpg 
gpg: encrypted with rsa4096 key, ID 0x366D7BBAD1031E80, created 2020-03-02
      "Harsh Shandilya (msfjarvis) <me@msfjarvis.dev>"
gpg: encrypted with rsa4096 key, ID 0x3115CE1787078171, created 2020-02-07
      "Harsh Shandilya (msfjarvis) <me@msfjarvis.dev>"
# off=0 ctb=85 tag=1 hlen=3 plen=524
:pubkey enc packet: version 3, algo 1, keyid 3115CE1787078171
	data: [4095 bits]
# off=527 ctb=85 tag=1 hlen=3 plen=524
:pubkey enc packet: version 3, algo 1, keyid 366D7BBAD1031E80
	data: [4095 bits]
# off=1054 ctb=d2 tag=18 hlen=2 plen=91 new-ctb
:encrypted data packet:
	length: 91
	mdc_method: 2
# off=1075 ctb=a3 tag=8 hlen=1 plen=0 indeterminate
:compressed packet: algo=1
# off=1077 ctb=cb tag=11 hlen=2 plen=47 new-ctb
:literal data packet:
	mode b (62), created 1619340100, name="",
	raw data: 41 bytes

Answer 10 · 2022-05-01T21:38:00.000Z

Could you provide me with a test key, so that I can try to reproduce this myself?

Answer 11 · 2022-05-01T21:48:45.000Z

Could you provide me with a test key, so that I can try to reproduce this myself?

This repo has a test key (john@doe.org.{pub,sec}) that I can confirm reproduces the issue.

Answer 12 · 2022-05-01T22:07:57.000Z

Hm, when I try to reproduce your encryption code in java, everything works as expected:

package investigations;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.nio.charset.StandardCharsets;

import org.bouncycastle.openpgp.PGPException;
import org.bouncycastle.openpgp.PGPPublicKeyRingCollection;
import org.bouncycastle.util.io.Streams;
import org.junit.jupiter.api.Test;
import org.pgpainless.PGPainless;
import org.pgpainless.algorithm.EncryptionPurpose;
import org.pgpainless.encryption_signing.EncryptionOptions;
import org.pgpainless.encryption_signing.EncryptionResult;
import org.pgpainless.encryption_signing.EncryptionStream;
import org.pgpainless.encryption_signing.ProducerOptions;

public class APSInvestigationTest {

    public static final String CERT = "-----BEGIN PGP PUBLIC KEY BLOCK-----\n" +
            "\n" +
            "mQGNBF8ZYcsBDADN7uFG6b/GZYK4zaBXEJ0ZTV1AmNCRDVHyp2GY/TSJYYLCpiyl\n" +
            "PhAlgems44L55XkDjFnAUkNqEmeB1j/nt277LLU6mr+OyT1ONvUCSonGCJpvLy04\n" +
            "PesX8TmPrzYHxIXeeeEAG5FzajHeR7IKczihBYJCIBw8k9jq2Xw6MgeYwNOkewcC\n" +
            "8sXp7DJm4lvlJTr7myZQZSzU1fQVj1cvtEFV6Ui2ga3zXqGvJpyvkltr0n7E0qhV\n" +
            "awQP8WJZR2+GvloIGocYSWgnHcV6hIOLyns4JrGOUbOXejiH7LxdeSFWCl6RBnGq\n" +
            "BfH8bFIuy9p2Js81kgyvO4iKBGWUNLLwBA++0h1RNVKupQOopgJfFvxT1brhEYpi\n" +
            "DCm2Nh4z210Xf+cvbbRS5r+8PVJJtTu9njFZOgkhAoDPyisSwGIkjowR34xZWaGk\n" +
            "0vUq6cgy++UzXagStdh+TBMHnUrofHzZi8rZ7neGdv5BEO05VH069ypCq//M6jFv\n" +
            "sCXPcfSGppSGIVcAEQEAAbQXSm9obiBEb2UgPGpvaG5AZG9lLm9yZz6JAcwEEwEK\n" +
            "ADYWIQQ2oHrzlxvNky+z1N+5UK4oE4QVhQUCXxlhywIbAwQLCQgHBBUKCQgFFgID\n" +
            "AQACHgECF4AACgkQuVCuKBOEFYV6GgwAiXxeRh+RUye14PYQhUl25FOk1kIH7Aes\n" +
            "y0O5NlkIDgPPErLhBPClSzmVekjQGPfByO2jnzwW764OBfbMmrCiykJScRTEnFa7\n" +
            "rt73UCCs7Ag0KsC8kjVxVaF0ywOEa2nq60ZC1NvGAAnZgOFj+pjJW5M7GyRikZ/G\n" +
            "iGx9pVyGOk3tMjZiJ4HZtAYEj8aOGB4BF2JAfUUeXR9lOBw8RQw6HmKPngH4271c\n" +
            "c7CNDZGUcFh3afS5Z7x9DKP4EezgzwlL2hdhRvQApe0N6yq64eGlluyqswkphZE+\n" +
            "6bOCXMQ3SHycE3vWeRnYZQSeSO6BDQ98PDPQVnQ0QUpacqEJQsJ0Ff3/l1DodbTj\n" +
            "8M0Ye2itrpKzDfNETHsNXC56m8JjxoGjQb2WefS7d8K2+KiTlgf9oRStkVo9HiDS\n" +
            "a3g0pAyQmjtAg6ulixxQovIrsBhnkA750PIeny+lWL6yp2kCfcd/szBaeqLy1Azl\n" +
            "/DW9gKMfaOkzkAX74YwI9DJmXG8vImSCuQGNBF8ZYcsBDAC25Xxq6FfnVbhEfu50\n" +
            "12NdkqQsbeL8rgqwWaEfELzrKGG5oa23iTiwYK+8al5RGIWuoLGwj5k37G5htf2V\n" +
            "nlmhRArBAHisEuXB51dw/HdqCF4lhzuXPHQMAR0KV85GjTQY/mxaNX3/xbJKk3DG\n" +
            "uCfuVvtneMaBmCe2pcGJ7B33FEKNNBQ7uZYxyFXs9mzc8VebMBsPaEMIActU0+rY\n" +
            "m3G0pf60k5oswYAEmQtQY5YkbeDfBIdBaaXwTQkh2t2kIMeVtHgsUdJH2OxIb9SA\n" +
            "Iugl6embpN1gHwVVAotL5pVkj7N5iPZwsjP8paazQHVyUTr+Rnyo+IyOXyF+K/xs\n" +
            "eVKtW4gDysaMGoDDOPQ/RzJArEfBFl1UNwc+y6/+BYZN5ML8UxrpVq8KzNcn2KsJ\n" +
            "7gS050eb9h+RufrYr/uVFDz81ZGc/K3+XF1ryvTtDbwyDUuKtDLyfuwuHxu6Q0kP\n" +
            "hjzpbvPddt0FR9Er7nv2SgFhOiodlpQV2KJj1DAmr0HdtUkAEQEAAYkBtgQYAQoA\n" +
            "IBYhBDagevOXG82TL7PU37lQrigThBWFBQJfGWHLAhsMAAoJELlQrigThBWFP2UL\n" +
            "/Agvjr2hJcaNM1mz2XhwMZkypfvkdIPvgdvEgMllO67G+GPPYoalLPGa7yPZrcJf\n" +
            "MN9Ochqzr0SAK/w8jWG1n2XYW/LKO/uLS9yIJWDHitNQDMV4c62x7Hvi7vQaCQch\n" +
            "yGSRGZjAsn+mYyAlqD4saj0rDjrLbIe5jTFOl3MaEc7dSWzd2hWLDy50lMUvQkWn\n" +
            "plaxOZJEnW7aeU0W19Do2sYqzHgFUDKAXU6PV4SAZ78JpoG90nMkpcs+cmJPtrBf\n" +
            "mb9NnSKTsVweJqydc8hQV2f/WNCbtCXmZlbvucvYAzZLy75edFMGOajmY8VzyseV\n" +
            "qC8tQmoFJ82cElB50/pHjzHtNQ34EQxg/dBjjZ3uqFdXI8s12jGPaeq4bz+AExyr\n" +
            "+X3fUDh8MO6gZuKiHriWGPMn2rjD71YfyOQfpMiyu7kkzwLP4tsiJYTo+8uDFK/r\n" +
            "UfMHRR0mMTC0f3YaHfhiLu2GGlGq0YZmgb2Siokev0ETMmTEN+HcfRqK7wZKl3CE\n" +
            "ew==\n" +
            "=6e/5\n" +
            "-----END PGP PUBLIC KEY BLOCK-----\n";

    @Test
    public void testEncryptTo() throws PGPException, IOException {
        ByteArrayInputStream pubKeysStream = new ByteArrayInputStream(CERT.getBytes(StandardCharsets.UTF_8));
        PGPPublicKeyRingCollection collection = PGPainless.readKeyRing().publicKeyRingCollection(pubKeysStream);
        assertEquals(1, collection.size()); // There is at one cert in this case

        EncryptionOptions encOpt = new EncryptionOptions(EncryptionPurpose.COMMUNICATIONS);
        encOpt.addRecipients(collection);

        ProducerOptions options = ProducerOptions.encrypt(encOpt);

        ByteArrayOutputStream out = new ByteArrayOutputStream();
        EncryptionStream encryptionStream = PGPainless.encryptAndOrSign()
                .onOutputStream(out)
                .withOptions(options);

        ByteArrayInputStream in = new ByteArrayInputStream("Hello, World!\n".getBytes(StandardCharsets.UTF_8));

        Streams.pipeAll(in, encryptionStream);
        encryptionStream.close();

        EncryptionResult result = encryptionStream.getResult();

        assertTrue(result.isEncryptedFor(collection.iterator().next()));

        System.out.println(out);
    }
}

The result looks like this.

This is a strange issue...

Answer 13 · 2022-05-01T22:31:54.000Z

That is quite curious, I wonder if some Kotlin ✨ magic ✨ is misbehaving in my code. I'll investigate this in the morning (4AM here 😵‍💫). Appreciate you taking a look!

Answer 14 · 2022-05-01T22:32:52.000Z

No problem. I'm setting up APS myselves right now to take a look :)

Answer 15 · 2022-05-01T22:50:32.000Z

No problem. I'm setting up APS myselves right now to take a look :)

Great! The first-time user experience sadly leaves a lot to be desired so I'll leave some pointers here in case some of the problems trip you up.

Make sure you use the snapshot build and enable the PGPainless backend via Settings > PGP Settings > Enable new PGP backend.
The local repo creation flow doesn't fully support PGPainless yet, so using the 'Clone remote repo' option with the pass-test repo is probably going to be the least painful way to get set up.
You will need to add the test key by placing it on your device storage and selecting it from the file picker that shows up when you use the import option in PGP Settings.

Answer 16 · 2022-05-01T22:52:52.000Z

FWIW I'm pretty confident the fix I tried made things worse because both the tests for PGPainlessCryptoHandler now fail :'D

@vanitasvitae can you review this method and let me know what I'm doing wrong? Thanks! 🙏🏼

Hm, for me those tests succeed without modification. I'm on hs/2022-04-29/pgpainless-encryption-fixup. Maybe your local setup is somehow different?

Answer 17 · 2022-05-01T22:57:09.000Z

FWIW I'm pretty confident the fix I tried made things worse because both the tests for PGPainlessCryptoHandler now fail :'D
@vanitasvitae can you review this method and let me know what I'm doing wrong? Thanks! 🙏🏼

Hm, for me those tests succeed without modification. I'm on hs/2022-04-29/pgpainless-encryption-fixup. Maybe your local setup is somehow different?

That's my bad, I should've used a new branch. I made that comment when the branch was at this commit, where the tests did fail. I pushed a new version of it today with this commit instead which doesn't break the tests but doesn't fix the issue either.

Answer 18 · 2022-05-01T22:58:26.000Z

I see. I will give it another try tomorrow :)
Good night!

Answer 19 · 2022-07-13T06:28:06.000Z

@vanitasvitae any other tips for how I can further debug this? I simplified the encryption logic to better match your suggestions but the issue still persists. I'm planning to completely remove OpenKeychain in favor of PGPainless so it'd be great to resolve this first.

Answer 20 · 2022-07-13T13:14:39.000Z

Hey,
unfortunately I did not manage to setup APS when I tried looking into it and then I kind of forgot about it :D

Does the issue still persist with release 1.3.1? I added some checks in the meantime to catch those cases.

Answer 21 · 2022-07-13T14:19:27.000Z

I found the issue! \o/

Here you are trying to read a PGPPublicKeyRingCollection from ASCII armored secret keys.

This operation will return an empty PGPPublicKeyRingCollection, since the underlying data is not public keys, but secret keys.

I got it to work, by converting the PGPSecretKeyRing objects into PGPPublicKeyRing objects using PGPainless.extractCert(secretKeyRing).

Unfortunately my Kotlin foo is not good enough to come up with a clean solution, but in Java I would do:

...
secKeysStream = ...;
PGPSecretKeyRingCollection secretKeyRingCollection = PGPainless.readKeyRing().secretKeyRingCollection(secKeysStream);
List<PGPPublicKeyRing> publicKeyRings = new ArrayList();
for (PGPSecretKeyRing secretKeyRing : secretKeyRingCollection) {
    publicKeyRings.add(PGPainless.extractCert(secretKeyRing);
}

and then pass secretKeyRings where you previously passed the PublicKeyRingCollection.

Answer 22 · 2022-07-13T14:22:30.000Z

For some reason this line did not catch the error, as it was an IllegalArgumentException.

Maybe it is also worth thinking about changing PGPainless' parser logic to extract public keys from secret keys the way you apparently expected it to do.

Answer 23 · 2022-07-13T14:27:49.000Z

Alternatively I could also add encryptionOptions.addRecipient() implementations that take PGPSecretKeyRings / PGPSecretKeyRingCollections as arguments, sparing you the need to convert between the types.

Answer 24 · 2022-07-13T15:23:28.000Z

Thank you so much for investigating this! It looks like this was indeed the bug. Newly encrypted files correctly contain encrypted packets.

➜ gpg --list-packets test.gpg 
gpg: encrypted with rsa4096 key, ID 0x366D7BBAD1031E80, created 2020-03-02
      "Harsh Shandilya (msfjarvis) <me@msfjarvis.dev>"
gpg: encrypted with rsa4096 key, ID 0x3115CE1787078171, created 2020-02-07
      "Harsh Shandilya (msfjarvis) <me@msfjarvis.dev>"
# off=0 ctb=85 tag=1 hlen=3 plen=524
:pubkey enc packet: version 3, algo 1, keyid 3115CE1787078171
	data: [4094 bits]
# off=527 ctb=85 tag=1 hlen=3 plen=524
:pubkey enc packet: version 3, algo 1, keyid 366D7BBAD1031E80
	data: [4089 bits]
# off=1054 ctb=d2 tag=18 hlen=2 plen=86 new-ctb
:encrypted data packet:
	length: 86
	mdc_method: 2
# off=1075 ctb=a3 tag=8 hlen=1 plen=0 indeterminate
:compressed packet: algo=1
# off=1077 ctb=cb tag=11 hlen=2 plen=43 new-ctb
:literal data packet:
	mode b (62), created 0, name="",
	raw data: 37 bytes

For some reason this line did not catch the error, as it was an IllegalArgumentException.

Maybe it is also worth thinking about changing PGPainless' parser logic to extract public keys from secret keys the way you apparently expected it to do.

I'm not fully sure where my expectation originates from, to be entirely honest. I think it's because of the user-facing behaviour of both OpenKeychain and the gpg2 CLI, where providing just a secret key allows all the public key operations to work just fine since they clearly extract the certificate internally to do the operation which remained opaque to me. I'm completely fine with PGPainless not making any changes on this front, but it'd certainly be nice to have this either documented or a slightly more idiot-proof API exposed for people like myself :)

Answer 25 · 2022-07-13T15:25:55.000Z

Thank YOU for using PGPainless and uncovering these shortcomings of the API :)

Answer 26 · 2022-07-13T15:34:50.000Z

Thank YOU for using PGPainless and uncovering these shortcomings of the API :)

With my luck there's a strong chance I will find more ways to shoot myself in the foot :D

Do you have any recommendations on how I would go about writing a regression test for this? I think I've peppered in enough assertions within the encryption path to catch these things (0b4ba25cf14a) but I can't find a way to do a blackbox test to verify from outside that method that the ciphertext being written to the stream is actually encrypted.

Answer 27 · 2022-07-13T15:37:58.000Z

That's a good question.

I'll think about it.

Answer 28 · 2022-07-13T19:12:17.000Z

Raised #2000 (cool number alert!) fixing this issue, and parameterized the existing tests to try encrypting with both the public and secret key to ensure the code path continues working.