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

pkcs11-tool: broken encryption/decryption #1786

Closed
frankmorgner opened this issue Sep 11, 2019 · 8 comments
Closed

pkcs11-tool: broken encryption/decryption #1786

frankmorgner opened this issue Sep 11, 2019 · 8 comments

Comments

@frankmorgner
Copy link
Member

Problem Description

9aa413b breaks PKCS#1 encryption/decryption test. The card (Gemalteo MultiApp IAS/ECC v1.0.1) responds with "Invalid Input Data". Since the commit only changes pkcs11-tool handling the encrypted (input) data, I think the problem lies in a bad formatting (usage) while creating the encrypted blob via OpenSSL.

Proposed Resolution

Unknown.

Logs

pkcs11-spy output looks identical in both cases.
fail.txt
spy-fail.txt
ok.txt
spy-ok.txt

@frankmorgner
Copy link
Member Author

@AlexandreGonzalo could you please review your commit, again?

By the way, I'm using OpenSSL 1.1.0k.

@AlexandreGonzalo
Copy link
Contributor

AlexandreGonzalo commented Sep 11, 2019

Hi Frank,
My own tests are OK with OpenSSL 1.1.1d, OpenSC-0.20.0-rc1 and the latest version of our PKCS11 implementation.
I will test with OpenSSL 1.1.0k and I will let you know the results.
Regards,
Alexandre.

@dengert
Copy link
Member

dengert commented Sep 12, 2019

original code has:
unsigned char orig_data[] = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', '\0'};

if (EVP_PKEY_encrypt(ctx, encrypted, &outlen, orig_data, sizeof(orig_data)) <= 0) {

i.e. size of orig_data is 10

new code:

unsigned char	orig_data[512] = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', '\0'};

in_len = mod_len-11;

if (EVP_PKEY_encrypt(ctx, encrypted, &out_len, orig_data, in_len) <= 0) {	

i.e. size of orig_data is mod_len-11= 256-11 = 245 so you are encrypting different data in the old and new case.

Why are you trying to enforce the PKCS#11 mod_len-11 restriction which the PKCS#11 module or card would enforce? The test case was originally to encrypt 10 bytes.

in_len does not look correct and the fact the new code sets the size of the the data to 512 does not look correct either.

@mouse07410
Copy link
Contributor

First, shouldn't

	/* Limit the input length to <= mod_len-11 */
	in_len = mod_len-11;

be

	/* Limit the input length to <= mod_len-11 */
	if (in_len > mod_len - 11)
		in_len = mod_len-11;

Your comment does say "limit"?

Second - like @dengert said, what's the point of changing orig_data[] = { ... }; to orig_data[512] = { ... };?

@AlexandreGonzalo
Copy link
Contributor

original code has:
unsigned char orig_data[] = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', '\0'};

if (EVP_PKEY_encrypt(ctx, encrypted, &outlen, orig_data, sizeof(orig_data)) <= 0) {

i.e. size of orig_data is 10

new code:

unsigned char	orig_data[512] = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', '\0'};

in_len = mod_len-11;

if (EVP_PKEY_encrypt(ctx, encrypted, &out_len, orig_data, in_len) <= 0) {	

i.e. size of orig_data is mod_len-11= 256-11 = 245 so you are encrypting different data in the old and new case.

Why are you trying to enforce the PKCS#11 mod_len-11 restriction which the PKCS#11 module or card would enforce? The test case was originally to encrypt 10 bytes.

in_len does not look correct and the fact the new code sets the size of the the data to 512 does not look correct either.

I used a buffer of 512 bytes for the input data because that was required by OpenSSL for the raw RSA encryption (exactly RSA_size(rsa) for RSA_NO_PADDING)
https://www.openssl.org/docs/manmaster/man3/RSA_public_encrypt.html
In fact, I tried to have common code for RAW, OAEP and PKCS1.5.

But it seems that this is too large for this smart card...

@AlexandreGonzalo
Copy link
Contributor

What about this patch?

encrypt_decrypt_limit_input_size.txt

@frankmorgner
Copy link
Member Author

Thanks, this works

@AlexandreGonzalo
Copy link
Contributor

Great!

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

No branches or pull requests

4 participants