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

Fix CFB decryption performance in JS fallback for ciphers other than AES #1679

Merged
merged 1 commit into from Sep 18, 2023

Conversation

larabr
Copy link
Collaborator

@larabr larabr commented Sep 12, 2023

By calling the underlying cipher decryption function for one block at a time.
Before, the entire ciphertext was passed for decryption, but only a subset of it (equal to the block size) was actually processed in each iteration.
This resulted in a lot of extra work for e.g. Cast5 decryption, whose decryption function supported decrypting more than a block at once.

This issue affected non-AES ciphers (legacy), such as Cast5, in Node 18+ or in browser.

Fix #1668 .

By calling the underlying cipher decryption function for one block at a time.
Before, the entire ciphertext was passed for decryption, but only a subset of it (equal to the block size)
was actually processed in each iteration.
This resulted in a lot of extra work for e.g. Cast5 decryption, whose decryption function supported decrypting more than a block at once.

This issue affected non-AES ciphers (legacy), such as Cast5, in Node 18+ or in browser.
@larabr larabr requested review from twiss and removed request for twiss September 12, 2023 10:06
Copy link
Member

@twiss twiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think part of the issue here is that some of the block ciphers encrypt only one block (which this code seems to expect), while some ciphers encrypts everything. It would be good to make that uniform, and if all block ciphers encrypt only one block, then this code doesn't need to be changed.

If we do want to change it here, we should also change the encrypt function (even though it's less likely to be used).

@larabr
Copy link
Collaborator Author

larabr commented Sep 17, 2023

@twiss even if the block cipher were to decrypt only one block, passing the full ciphertext in input to them seems confusing. I'd leave more extensive changes to the underlying ciphers be done as part as #1654 .

The encryption function is fine as-is since blockc is always of IV length

@larabr larabr merged commit 2ba8229 into openpgpjs:main Sep 18, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CAST5 file decryption hangs indefinitely
2 participants