Skip to content

Conversation

@simo5
Copy link
Member

@simo5 simo5 commented Mar 14, 2025

Description

Fixes various issues with requesting the necessary buffer lengths via behavior 5.2 of the PKCS#11 spec.

Fixes #179

Checklist

  • Test suite updated with functionality tests
  • Test suite updated with negative tests
  • Documentation was updated
  • This is not a code change

Reviewer's checklist:

  • Any issues marked for closing are fully addressed
  • There is a test suite reasonably covering new functionality or modifications
  • This feature/change has adequate documentation added
  • A changelog entry is added if the change is significant
  • Code conform to coding style that today cannot yet be enforced via the check style test
  • Commits have short titles and sensible text

@simo5 simo5 requested a review from Jakuje March 14, 2025 19:46
@simo5 simo5 force-pushed the update_len branch 2 times, most recently from 92e166e to 2853fe9 Compare March 14, 2025 20:42
@simo5
Copy link
Member Author

simo5 commented Mar 14, 2025

I also added a quick commit as a stop -gap mesure for #184 but a proper fix will have to wait a little longer

@Jakuje
Copy link
Contributor

Jakuje commented Mar 15, 2025

Its much more changes than I expected. But code wise look good. Added one inline nit regarding magic numbers.

There are some typos in the commit messages. We should try to run the spellcheck on that too:
s/accoutn/account/
s/inpout/input/
s/ast/as/
s/crypton_len/cryption_len/

simo5 added 5 commits March 18, 2025 10:34
Change the [en/de]cryption_len() functions semantics.
a) allow calling these functions regardless of internal status, as now
they are not tied to it.

b) When called with the fin value set to true, the functions now return
the total needed output buffer without assuming the caller is at the
final step. The function does include internal state in the computation
but can be called even immediately after initialization.

This means that fn_encrypt()/fn_decrypt() functions can easily query
for the total required length, as well as _final() functions with the
same code.

c) When called without the 'fin' boolean turned on, assume that the
request is being made in the context of calling just the next _update()
function. This means calculations only enforce the minimum buffer output
needed for the input data being injested and does not need to account
for additional data that some modes my require on final like AES_CBC_PAD,
AES_GCM, and others that append padding or tags.

d) For block ciphers allow providing partial inputs, as long as, before
calling final(), the correct size has been fed into the operation we
are now able to take partial input buffers even for modes that require
the total input to be block sized.

e) An internal buffer is always used (for block modes) to store partial
blocks, and OpenSSL is only ever asked to encrypt block-sized quantities.
Modes that allow non-block sized inputs but require block sized outputs
complete the last short write in the _final() operation.

f) The above does require the caller to always provide enough output
buffer size to take entire output blocks taking in account possible
partial blocks previously fed to the function. The new
[en/de]cryption_len() semantics always correctly report how much output
is required in these cases.

Signed-off-by: Simo Sorce <[email protected]>
There are now more cases where this buffer contains a partial buffer in
the middle of operations than just a final tag or similar ciphertext,
therefore change the name to be more neutral.

Signed-off-by: Simo Sorce <[email protected]>
If we do not pass in a valid buffer we may get spurious failures.

Signed-off-by: Simo Sorce <[email protected]>
This is not a fix for latchset#184, but at least it is a stop-gap measure to
return an error instead of just crashing the process.

Signed-off-by: Simo Sorce <[email protected]>
Copy link
Contributor

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

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

lgtm

@xhanulik
Copy link

For #179 the patch fixes the problem with pkcs11-tool.

@simo5 simo5 merged commit 5e5983e into latchset:main Mar 24, 2025
24 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.

C_EncryptUpdate fails with CKR_BUFFER_TOO_SMALL

3 participants