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 PKCS#11 C_Decrypt, C_Encrypt, C_Sign, C_SignFinal buffer output size. #1979

Merged

Conversation

dewyatt
Copy link
Contributor

@dewyatt dewyatt commented May 26, 2019

The spec does say that modules should set pulBufLen to the exact byte
count when pBuf is not NULL. However, some modules fail to do this and
it's easy to work around here.

EDIT: See below.

This is a contribution from Ribose Inc (@riboseinc).

@dewyatt
Copy link
Contributor Author

dewyatt commented May 26, 2019

Actually I'm a little tired and it looks like this is actually a bug in botan and not the PKCS#11 module.
The first size is an estimate, which is allowed to be larger than the actual output, according to the spec (section 5.2). The vector needs to be resized after that second call.

Will update title and such.

@codecov-io
Copy link

codecov-io commented May 26, 2019

Codecov Report

Merging #1979 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1979      +/-   ##
==========================================
+ Coverage   91.64%   91.64%   +<.01%     
==========================================
  Files         533      533              
  Lines       57324    57323       -1     
  Branches     6083     6083              
==========================================
  Hits        52533    52533              
+ Misses       4791     4790       -1
Impacted Files Coverage Δ
src/tests/test_pkcs11_high_level.cpp 99.27% <ø> (-0.01%) ⬇️
src/lib/misc/cryptobox/cryptobox.cpp 95.23% <0%> (+1.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dd03c9...4a759ef. Read the comment docs.

Section 5.2 of the spec states that there are two ways to call
functions that return a variable-length buffer:

1. When the output buffer is NULL, an estimated size is returned (which
may be larger than required).
2. When the output buffer is not NULL, the exact size must be returned.

So only after the second call to C_Decrypt has the final output size
been determined, and we must resize the output buffer.
@dewyatt dewyatt force-pushed the dewyatt-pkcs11-C_Decrypt_pulBufLen branch from 0bf1325 to a82fdf2 Compare May 26, 2019 23:34
@dewyatt dewyatt changed the title Workaround buggy PKCS#11 modules setting inexact pulBufLen in C_Decrypt. The vector needs to be resized after that second call. May 26, 2019
@dewyatt dewyatt changed the title The vector needs to be resized after that second call. Fix PKCS#11 C_Decrypt buffer output size. May 26, 2019
@dewyatt
Copy link
Contributor Author

dewyatt commented May 26, 2019

Copy & paste error on the title fixed :P, I should get some sleep before I hurt myself.

@randombit
Copy link
Owner

@neusdan can you review?

@dewyatt
Copy link
Contributor Author

dewyatt commented May 27, 2019

It looks like there are similar bugs in other places, so I'll add some commits here and update the title.

@dewyatt dewyatt changed the title Fix PKCS#11 C_Decrypt buffer output size. Fix PKCS#11 C_Decrypt, C_Encrypt, C_Sign, C_SignFinal buffer output size. May 27, 2019
Copy link
Collaborator

@neusdan neusdan left a comment

Choose a reason for hiding this comment

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

LGTM. I have checked the specification. This change definitely makes sense. Unit tests with SoftHSM and a CardOS card were successful.

@randombit randombit merged commit 4a759ef into randombit:master Jun 5, 2019
randombit added a commit that referenced this pull request Jun 5, 2019
@dewyatt dewyatt deleted the dewyatt-pkcs11-C_Decrypt_pulBufLen branch October 29, 2019 16:54
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.

4 participants