Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encrypted private key is authenticated using an insecure two-byte hash #176

Closed
DenBond7 opened this issue Sep 7, 2021 · 16 comments
Closed
Labels
bug Something isn't working module: core Issue affects the core module upstream Issue is caused by an upstream package

Comments

@DenBond7
Copy link
Contributor

DenBond7 commented Sep 7, 2021

@vanitasvitae Please look at the following issue FlowCrypt/flowcrypt-browser#3945.

After updating from 0.2.3 to 0.2.4 a user is not able to import a modified key(via PGPainless) to FlowCrypt for the browser. It seems something was changed in the encryption key process. I'm still doing the investigation. I will try to find a commit that causes the issue.

We use the following code

private fun changeKeyPassphrase(
    key: PGPSecretKeyRing,
    oldPassphrase: Passphrase,
    newPassphrase: Passphrase
  ): PGPSecretKeyRing {
    return encryptKey(decryptKey(key, oldPassphrase), newPassphrase)
  }
fun decryptKey(key: PGPSecretKeyRing, passphrase: Passphrase): PGPSecretKeyRing {
    return PGPainless.modifyKeyRing(key)
      .changePassphraseFromOldPassphrase(passphrase)
      .withSecureDefaultSettings()
      .toNoPassphrase()
      .done()
  }
private fun encryptKey(key: PGPSecretKeyRing, passphrase: Passphrase): PGPSecretKeyRing {
    return PGPainless.modifyKeyRing(key)
      .changePassphraseFromOldPassphrase(null)
      .withSecureDefaultSettings()
      .toNewPassphrase(passphrase)
      .done()
  }
@vanitasvitae
Copy link
Member

This could have to do with S2K parameters (KeyRingProtectionSettings), but this is just a wild guess.

@vanitasvitae vanitasvitae added bug Something isn't working module: core Issue affects the core module labels Sep 8, 2021
@DenBond7
Copy link
Contributor Author

DenBond7 commented Sep 8, 2021

I think I'm very close to the answer. Need to check a few commits.

@DenBond7
Copy link
Contributor Author

DenBond7 commented Sep 8, 2021

After this commit we have the current issue.

@vanitasvitae
Copy link
Member

I see.
This commit switches away from manually constructing the newly encrypted secret key via

        PGPDigestCalculator checksumCalculator = ImplementationFactory.getInstance()
                .getPGPDigestCalculator(defaultDigestHashAlgorithm);
        PBESecretKeyEncryptor encryptor = protector.getEncryptor(publicKey.getKeyID());
        // freshly locked key
        PGPSecretKey secretKey = new PGPSecretKey(privateKey, publicKey, checksumCalculator, publicKey.isMasterKey(), encryptor);

to using the supposedly better approach of relying on BC code:

        secretKey = PGPSecretKey.copyWithNewPassword(secretKey, decryptor, encryptor);

This method does a bunch of checks and additional logic depending on S2K usage and such. Perhaps there is an error in this method somewhere?

Btw: What exactly does the issue title mean? Is this an error message returned by FlowCrypt Browser? I can see that there is some checksum being calculated by BC.

@DenBond7
Copy link
Contributor Author

DenBond7 commented Sep 8, 2021

Btw: What exactly does the issue title mean? Is this an error message returned by FlowCrypt Browser? I can see that there is some checksum being calculated by BC.

@tomholub Could you provide more details here or point somebody from the team who can add more details from the code for FlowCrypt Browser?(maybe @rrrooommmaaa)

@tomholub
Copy link
Contributor

tomholub commented Sep 8, 2021

Encrypted private key is authenticated using an insecure two-byte hash is an error thrown by OpenPGP.js when trying to use that key. @DenBond7 could you share a test key that you created where you could reproduce this?

@DenBond7
Copy link
Contributor Author

DenBond7 commented Sep 8, 2021

@DenBond7 could you share a test key that you created where you could reproduce this?

Key before the passphrase change

user: default@flowcrypt.test
passphrase: My super strong password 2018

-----BEGIN PGP PRIVATE KEY BLOCK-----
Version: FlowCrypt 1.1.7_dev_117__2021_04_29 Gmail Encryption
Comment: Seamlessly send and receive encrypted email

lIYEYIq7phYJKwYBBAHaRw8BAQdAat45rrh+gvQwWwJw5eScq3Pdxt/8d+lWNVSm
kImXcRP+CQMCN1DDBAVFK5hgUQ9Gv5lUX5ckMkPbXGeJg4b646TssJOXLwhMAN0P
Qw1qLJopKgXmXhFgXK/LvohrHm7JyPCgTcnRyfbFnDhkpH8tBkOwmrQWZGVmYXVs
dEBmbG93Y3J5cHQudGVzdIh4BBMWCgAgBQJgirumAhsDBRYCAwEABAsJCAcFFQoJ
CAsCHgECGQEACgkQIl+AI8INCVcysgD/cu23M07rImuV5gIl98uOnSIR+QnHUD/M
I34b7iY/iTQBALMIsqO1PwYl2qKwmXb5lSoMj5SmnzRRE2RwAFW3AiMCnIsEYIq7
phIKKwYBBAGXVQEFAQEHQA8q7iPr+0OXqBGBSAL6WNDjzHuBsG7uiu5w8l/A6v8l
AwEIB/4JAwJOpYoeRSBH0mD00TwFiadSxNB/I30mP+V++wqILsLGOok8aLfR6tJR
bi0dc6zUHSiccfEu7iWDlQQzFHuZ8aWbtmxGJmyyOTDJWDSgji+tiHUEGBYKAB0F
AmCKu6YCGwwFFgIDAQAECwkIBwUVCgkICwIeAQAKCRAiX4Ajwg0JV+sbAQCv4LVM
0+AN54ivWa4vPRyYOfSQ1FqsipkYLJce+xwUeAD+LZpEVCypFtGWQVdeSJVxIHx3
k40IfHsK0fGgR+NrRAw=
=XEyh
-----END PGP PRIVATE KEY BLOCK-----

Compatibility details:

(1) Primary keys found: 1  
(2) ----- Testing key 0 -----  
(3) PK 0 > Key type: openpgp  
(4) PK 0 > Is Private?: [-] true  
(5) PK 0 > User id 0: default@flowcrypt.test  
(6) PK 0 > Primary User: default@flowcrypt.test  
(7) PK 0 > Fingerprint: 3DEB E9F6 77D5 B9BB 38E5 A244 225F 8023 C20D 0957  
(8) PK 0 > Subkeys: [-] 1  
(9) PK 0 > Primary key algo: [-] eddsa  
(10) PK 0 > Usage flags: [-] [sign_data, certify_keys]  
(11) PK 0 > key decrypt: [-] INCORRECT PASSPHRASE  
(12) PK 0 > isFullyDecrypted: [-] false  
(13) PK 0 > isFullyEncrypted: [-] true  
(14) PK 0 > Primary key verify: [-] valid  
(15) PK 0 > Primary key creation?: [-] 1619704742 or 2021-04-29T13:59:02.000Z  
(16) PK 0 > Primary key expiration?: [-] -  
(17) PK 0 > Encrypt/Decrypt test: Encryption with key was successful:   
(18) PK 0 > Encrypt/Decrypt test: Skipping decryption because isPrivate:true isFullyDecrypted:false:   
(19) PK 0 > Sign/Verify test: [-] skipped, not fully decrypted  
(20) PK 0 > SK 0 > LongId: [-] 4F1458BD22B7BB53  
(21) PK 0 > SK 0 > Created: [-] 1619704742 or 2021-04-29T13:59:02.000Z  
(22) PK 0 > SK 0 > Algo: [-] ecdh  
(23) PK 0 > SK 0 > Usage flags: [-] [encrypt_communication, encrypt_storage]  
(24) PK 0 > SK 0 > Verify: [-] OK  
(25) PK 0 > SK 0 > Subkey tag: [-] 7  
(26) PK 0 > SK 0 > Subkey getBitSize: [-] undefined  
(27) PK 0 > SK 0 > Subkey decrypted: [-] false  
(28) PK 0 > SK 0 > Binding signature length: [-] 1  
(29) PK 0 > SK 0 > SIG 0 > Key flags: [-] 12  
(30) PK 0 > SK 0 > SIG 0 > Tag: [-] 2  
(31) PK 0 > SK 0 > SIG 0 > Version: [-] 4  
(32) PK 0 > SK 0 > SIG 0 > Public key algorithm: [-] 22  
(33) PK 0 > SK 0 > SIG 0 > Sig creation time: [-] 1619704742 or 2021-04-29T13:59:02.000Z  
(34) PK 0 > SK 0 > SIG 0 > Sig expiration time: [-] -  
(35) PK 0 > SK 0 > SIG 0 > Verified: [-] true  
(36) PK 0 > expiration: [-] undefined  
(37) PK 0 > internal dateBeforeExpiration: [-] undefined  
(38) PK 0 > internal usableForEncryptionButExpired: [-] false  
(39) PK 0 > internal usableForSigningButExpired: [-] false  

Key after the passphrase change(causes the issue)

user: default@flowcrypt.test
passphrase: My super strong password 2021

-----BEGIN PGP PRIVATE KEY BLOCK-----
Version: FlowCrypt 1.2.0_dev_120__2021_09_08 Gmail Encryption
Comment: Seamlessly send and receive encrypted email

lHQEYIq7phYJKwYBBAHaRw8BAQdAat45rrh+gvQwWwJw5eScq3Pdxt/8d+lWNVSm
kImXcRP/CQMCfHrzw7xFgfJg/eEDFiraG7TXBWHyTNgGoJqw+oFzSOxjoKgPJk4Z
srqmcCnr2oqQB+E4RNYXXHxNxPSJiLQWZGVmYXVsdEBmbG93Y3J5cHQudGVzdIh4
BBMWCgAgBQJgirumAhsDBRYCAwEABAsJCAcFFQoJCAsCHgECGQEACgkQIl+AI8IN
CVcysgD/cu23M07rImuV5gIl98uOnSIR+QnHUD/MI34b7iY/iTQBALMIsqO1PwYl
2qKwmXb5lSoMj5SmnzRRE2RwAFW3AiMCnHkEYIq7phIKKwYBBAGXVQEFAQEHQA8q
7iPr+0OXqBGBSAL6WNDjzHuBsG7uiu5w8l/A6v8lAwEIB/8JAwLzb9X3CHTwOGC7
ZQHScSMdv/jo/ZDKhy0d/n5Vb/IlTWbXLTaknj7NxhKI3mKD7od+r0Yt1UdffU0t
kbTgiHUEGBYKAB0FAmCKu6YCGwwFFgIDAQAECwkIBwUVCgkICwIeAQAKCRAiX4Aj
wg0JV+sbAQCv4LVM0+AN54ivWa4vPRyYOfSQ1FqsipkYLJce+xwUeAD+LZpEVCyp
FtGWQVdeSJVxIHx3k40IfHsK0fGgR+NrRAw=
=Tbrl
-----END PGP PRIVATE KEY BLOCK-----

Compatibility details:

(1) Primary keys found: 1  
(2) ----- Testing key 0 -----  
(3) PK 0 > Key type: openpgp  
(4) PK 0 > Is Private?: [-] true  
(5) PK 0 > User id 0: default@flowcrypt.test  
(6) PK 0 > Primary User: default@flowcrypt.test  
(7) PK 0 > Fingerprint: 3DEB E9F6 77D5 B9BB 38E5 A244 225F 8023 C20D 0957  
(8) PK 0 > Subkeys: [-] 1  
(9) PK 0 > Primary key algo: [-] eddsa  
(10) PK 0 > Usage flags: [-] [sign_data, certify_keys]  
(11) PK 0 > key decrypt: [Error: Encrypted private key is authenticated using an insecure two-byte hash]  
(12) PK 0 > isFullyDecrypted: [-] false  
(13) PK 0 > isFullyEncrypted: [-] true  
(14) PK 0 > Primary key verify: [-] valid  
(15) PK 0 > Primary key creation?: [-] 1619704742 or 2021-04-29T13:59:02.000Z  
(16) PK 0 > Primary key expiration?: [-] -  
(17) PK 0 > Encrypt/Decrypt test: Encryption with key was successful:   
(18) PK 0 > Encrypt/Decrypt test: Skipping decryption because isPrivate:true isFullyDecrypted:false:   
(19) PK 0 > Sign/Verify test: [-] skipped, not fully decrypted  
(20) PK 0 > SK 0 > LongId: [-] 4F1458BD22B7BB53  
(21) PK 0 > SK 0 > Created: [-] 1619704742 or 2021-04-29T13:59:02.000Z  
(22) PK 0 > SK 0 > Algo: [-] ecdh  
(23) PK 0 > SK 0 > Usage flags: [-] [encrypt_communication, encrypt_storage]  
(24) PK 0 > SK 0 > Verify: [-] OK  
(25) PK 0 > SK 0 > Subkey tag: [-] 7  
(26) PK 0 > SK 0 > Subkey getBitSize: [-] undefined  
(27) PK 0 > SK 0 > Subkey decrypted: [-] false  
(28) PK 0 > SK 0 > Binding signature length: [-] 1  
(29) PK 0 > SK 0 > SIG 0 > Key flags: [-] 12  
(30) PK 0 > SK 0 > SIG 0 > Tag: [-] 2  
(31) PK 0 > SK 0 > SIG 0 > Version: [-] 4  
(32) PK 0 > SK 0 > SIG 0 > Public key algorithm: [-] 22  
(33) PK 0 > SK 0 > SIG 0 > Sig creation time: [-] 1619704742 or 2021-04-29T13:59:02.000Z  
(34) PK 0 > SK 0 > SIG 0 > Sig expiration time: [-] -  
(35) PK 0 > SK 0 > SIG 0 > Verified: [-] true  
(36) PK 0 > expiration: [-] undefined  
(37) PK 0 > internal dateBeforeExpiration: [-] undefined  
(38) PK 0 > internal usableForEncryptionButExpired: [-] false  
(39) PK 0 > internal usableForSigningButExpired: [-] false  

Selection_800

@vanitasvitae
Copy link
Member

vanitasvitae commented Sep 8, 2021

I did some debugging:

Before:
2476839211144907095
S2K Usage: 254
HashAlgo: SHA1
Iter count: 65536
IV: [55, 80, -61, 4, 5, 69, 43, -104]
Type: 3
Protection Mode: -1
5698276997885049683
S2K Usage: 254
HashAlgo: SHA1
Iter count: 65536
IV: [78, -91, -118, 30, 69, 32, 71, -46]
Type: 3
Protection Mode: -1
After:
2476839211144907095
S2K Usage: 255
HashAlgo: SHA1
Iter count: 65536
IV: [124, 122, -13, -61, -68, 69, -127, -14]
Type: 3
Protection Mode: -1
5698276997885049683
S2K Usage: 255
HashAlgo: SHA1
Iter count: 65536
IV: [-13, 111, -43, -9, 8, 116, -16, 56]
Type: 3
Protection Mode: -1

Note that the "after" key has a different S2K usage value.
Previously it was CHECKSUM (0xff=255), afterwards it is SHA1 (0xfe=254).
This appears to be the cause of the issue.

Now to find out how this comes to be :D

@vanitasvitae
Copy link
Member

I suppose that this line is the cause for the issue:
https://github.com/bcgit/bc-java/blob/bc3b92f1f0e78b82e2584c5fb4b226a13e7f8b3b/pg/src/main/java/org/bouncycastle/openpgp/PGPSecretKey.java#L815-L818

            if (s2kUsage == SecretKeyPacket.USAGE_NONE)
            {
                s2kUsage = SecretKeyPacket.USAGE_CHECKSUM;
            }

Since in your example code, you are doing a two step passphrase change (old -> none -> new), the S2K usage is set to none and then to CHECKSUM (which is insecure apparently).

I suppose, BC should set the S2K usage to SHA1 instead in that line.
You can however work around this in your code as well by calling PGPainless like this:

private fun changeKeyPassphrase(
    key: PGPSecretKeyRing,
    oldPassphrase: Passphrase,
    newPassphrase: Passphrase
  ): PGPSecretKeyRing {
    return PGPainless.modifyKeyRing(key)
      .changePassphraseFromOldPassphrase(oldPassphrase)
      .withSecureDefaultSettings()
      .toNewPassphrase(newPassphrase)
      .done()
  }

saving the intermediate step.

@DenBond7
Copy link
Contributor Author

DenBond7 commented Sep 8, 2021

You can however work around this in your code as well by calling PGPainless like this

Thank you! I will check

@vanitasvitae
Copy link
Member

This is what I get when I change the passphrase directly:

Before:
2476839211144907095
S2K Usage: USAGE_SHA1
HashAlgo: SHA1
Iter count: 65536
IV: [55, 80, -61, 4, 5, 69, 43, -104]
Type: 3
Protection Mode: -1
5698276997885049683
S2K Usage: USAGE_SHA1
HashAlgo: SHA1
Iter count: 65536
IV: [78, -91, -118, 30, 69, 32, 71, -46]
Type: 3
Protection Mode: -1
After:
2476839211144907095
S2K Usage: USAGE_SHA1
HashAlgo: SHA1
Iter count: 65536
IV: [36, -96, -119, 107, 91, 50, -31, 95]
Type: 3
Protection Mode: -1
5698276997885049683
S2K Usage: USAGE_SHA1
HashAlgo: SHA1
Iter count: 65536
IV: [-3, 125, -48, 87, 115, 81, -72, 47]
Type: 3
Protection Mode: -1

@vanitasvitae
Copy link
Member

I will ask upstream what the reason for USAGE_CHECKSUM is and if we can change to USAGE_SHA1 by default instead for when changing from unprotected to passphrase protected keys.

@vanitasvitae
Copy link
Member

I created bcgit/bc-java#1020 upstream.

@vanitasvitae vanitasvitae added the upstream Issue is caused by an upstream package label Sep 8, 2021
@DenBond7
Copy link
Contributor Author

DenBond7 commented Sep 8, 2021

You can however work around this in your code as well by calling PGPainless like this:

I confirm that it fixes the issue. @vanitasvitae Please close it if needed. Thank you!

@tomholub
Copy link
Contributor

tomholub commented Sep 8, 2021

By the way, is there some better option than USAGE_SHA1? Using a more secure hash? Or is that already the most secure option available?

@vanitasvitae
Copy link
Member

vanitasvitae commented Sep 8, 2021

I think for version 4 keys (which is what BC currently supports), USAGE_SHA1 is the best option.
For the upcoming version 5 keys, the current draft of the spec briefly mentions a new usage type (253) which supposedly makes use of AEAD. This would be the favourable option, but it is not yet available (and I'm not sure if the spec is stable enough already for implementations to emerge).

Edit: RFC4880bis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working module: core Issue affects the core module upstream Issue is caused by an upstream package
Projects
None yet
Development

No branches or pull requests

3 participants