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

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

Open
tomholub opened this issue Jan 24, 2023 · 9 comments · May be fixed by #4971
Open

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

tomholub opened this issue Jan 24, 2023 · 9 comments · May be fixed by #4971
Assignees

Comments

@tomholub
Copy link
Collaborator

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 #4725 (comment)

@tomholub
Copy link
Collaborator Author

image

@tomholub tomholub added this to the 8.4.4 milestone Jan 24, 2023
@tomholub tomholub mentioned this issue Jan 24, 2023
5 tasks
@tomholub
Copy link
Collaborator Author

image

@rrrooommmaaa
Copy link
Contributor

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
Copy link
Collaborator Author

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

@sosnovsky
Copy link
Collaborator

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
Copy link
Collaborator Author

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

@rrrooommmaaa
Copy link
Contributor

rrrooommmaaa commented Feb 27, 2023

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
Copy link
Collaborator Author

tomholub commented Mar 6, 2023

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

@rrrooommmaaa
Copy link
Contributor

Reported openpgpjs/openpgpjs#1608

@sosnovsky sosnovsky modified the milestones: 8.5.2, 8.5.3 Oct 25, 2023
@sosnovsky sosnovsky modified the milestones: 8.5.4, 8.5.5 Mar 1, 2024
@sosnovsky sosnovsky modified the milestones: 8.5.5, 8.5.6 May 13, 2024
@sosnovsky sosnovsky modified the milestones: 8.5.6, 8.5.7, First priority May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants