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

Add ECB/NoPadding and CBC/NoPadding support to AES encryption #2013

Merged
merged 4 commits into from
Apr 5, 2025

Conversation

plvie
Copy link
Contributor

@plvie plvie commented Apr 4, 2025

Summary

This pull request adds support for two new AES encryption modes:

  • ECB/NoPadding
  • CBC/NoPadding

Details

  • Updated the AESEncrypt operation to include new mode options in the UI.
  • Modified the logic to handle NoPadding modes, including input validation (input length must be a multiple of 16 bytes).
  • Implemented the same logic at decryption to disable padding when NoPadding is selected.

Code Changes

  • File: src/core/operations/AESEncrypt.mjs
  • +19/-1 lines

Notes

  • If NoPadding is selected and the input is not a multiple of 16 bytes, the operation now throws an error.
  • forge padding is bypassed accordingly.

Closes #1988

@CLAassistant
Copy link

CLAassistant commented Apr 4, 2025

CLA assistant check
All committers have signed the CLA.

@a3957273
Copy link
Member

a3957273 commented Apr 5, 2025

One tiny change, otherwise seems good! Is there a reason why all AES encryption modes can't have a 'no padding' option, whereupon a checkbox might be reasonable? It feels strained to put everything in a single dropdown box, especially if the number of options increase.

@plvie
Copy link
Contributor Author

plvie commented Apr 5, 2025

Yes, you're right that a split might be clearer. I followed the implementation already in place for decryption, but splitting the options could indeed improve clarity.

As for not using a checkbox: there's actually a reason for that. Only ECB and CBC modes support padding. Modes like CFB, OFB, CTR (and GCM, which is based on CTR) don’t require padding, as they handle plaintexts of arbitrary lengths by XORing the plaintext with the output of the block cipher. The last partial block of plaintext is XORed with the beginning of the final key stream block, resulting in a ciphertext block of the same size as the final partial plaintext block.

Since we're using node-forge, which only supports these six modes (https://github.com/andersk/node-forge/blob/b08787905f26239f7c560f47f4487362a7071342/lib/aes.js#L254), we've already reached the maximum number of available modes. So I don't think adding a checkbox would be the most suitable solution here.

@a3957273
Copy link
Member

a3957273 commented Apr 5, 2025

A very clear explanation, and your logic seems sound! Will get this merged in, thank you for the contribution! :)

@a3957273 a3957273 merged commit e849569 into gchq:master Apr 5, 2025
4 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.

Feature request: AES ECB/NoPadding option for encryption
3 participants