FlowCrypt / flowcrypt-browser

FlowCrypt Browser extension for Chrome and Firefox
https://flowcrypt.com
Other
374 stars 46 forks source link

Use OpenPGP.js v5 native ways to reject algorithms #4905

Open tomholub opened 1 year ago

tomholub commented 1 year ago

I suppose there was new functionality for this in OpenPGP.js v5, where we can simply configure which algorithms to use or forbid? Either in global config or when doing particular actions (forgot about their implementation). In which case we could stop using removeWeakKeyPackets / remove it. Can be another issue.

_Originally posted by @tomholub in https://github.com/FlowCrypt/flowcrypt-browser/pull/4725#discussion_r1085576518_

tomholub commented 1 year ago

image

tomholub commented 1 year ago

image

rrrooommmaaa commented 1 year ago

Yes, there is minRSABits setting with the default value = 2047, as well as rejectPublicKeyAlgorithms and rejectCurves already filled with some insecure algos. However, I was surprised to see how OpenPGP.js behaves in this setup: primary key = RSA1024 (insecure) subkey = RSA4096 (secure)

It does allow to use this subkey (signed by the weak primary key) for encryption. Should we file this as a bug to OpenPGP.js or follow this logic, @tomholub ?

tomholub commented 1 year ago

i'd file it as a bug. cc @sosnovsky

sosnovsky commented 1 year ago

I found possible explanation for such logic in openpgp.js:

There is no way to "upgrade" an OpenPGP key. You will have to create a new one, and you will lose your reputation in the web of trust. Some people I met decided to stick with a RSA 1024 primary key, but use stronger subkeys instead (which is easily possible without losing your reputation in the web of trust), which comes with secure day-to-day use (for encryption/signing documents with your subkeys), but might enable attackers to add and revoke certifications, subkeys and UIDs.

(from https://superuser.com/a/613926)

So probably they doesn't see it as a bug, but expected functionality and they check minRsaBytes only for subkey.

Then we'll still need to check primary key algorithm on our side or propose openpgp.js to add new config property which will apply minRsaBytes to primary keys too.

tomholub commented 1 year ago

I think it should still be reported as a bug. It's called minRSABits and not minRSABitsExceptPrimaryKey

rrrooommmaaa commented 1 year ago

So probably they doesn't see it as a bug, but expected functionality and they check minRsaBytes only for subkey.

That's not entirely accurate. They check minRSABits for certain usages, that is, in this setup: PK RSA1024 [certifyKeys, signData], SK 4096 [encryptCommunication], it's not possible to sign data (the PK is invalid for signing), but possible to encrypt a message (with the SK certified by the weak PK). So, it's like minRSABitsExceptKeyCertification restriction

tomholub commented 1 year ago

Sounds buggy for sure. Once you report it, please link that issue here. Let's see if they intended this behavior.

rrrooommmaaa commented 1 year ago

Reported https://github.com/openpgpjs/openpgpjs/issues/1608