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

soter: make sure we have the RSA private key before trying to decrypt… #334

Merged
merged 1 commit into from Nov 1, 2018

Conversation

ignatk
Copy link
Contributor

@ignatk ignatk commented Nov 1, 2018

… data

Some OpenSSL versions do not handle gracefully, when you try to do RSA
decryption with a public key (no private part in the RSA structure) and just
crash the whole process. So we add an explicit check we have the private part
before attempting the decryption.

@ignatk
Copy link
Contributor Author

ignatk commented Nov 1, 2018

Fixes #332

The crash.c program does not crash now, however, fails with 11 (which is just THEMIS_FAIL). So the proper SOTER_INVALID_PARAMETER error is not propagated properly.

@vixentael
Copy link
Contributor

@secumod thank you for this fix!

Could you please update code because tests are crashing at this point.

src/soter/openssl/soter_asym_cipher.c:249:2: error: implicit declaration of function 'RSA_get0_key' [-Werror=implicit-function-declaration]
  RSA_get0_key(rsa, NULL, NULL, &d);
  ^
cc1: all warnings being treated as errors
Makefile:294: recipe for target 'build/obj/soter/openssl/soter_asym_cipher.o' failed```

@ignatk
Copy link
Contributor Author

ignatk commented Nov 1, 2018

Yes, I saw that - older OpenSSL libs (< 1.1.0) do not have this accessor function.

… data

Some OpenSSL versions do not handle gracefully, when you try to do RSA
decryption with a public key (no private part in the RSA structure) and just
crash the whole process. So we add an explicit check we have the private part
before attempting the decryption.
Copy link
Collaborator

@ilammy ilammy left a comment

Choose a reason for hiding this comment

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

I've tested this change with OpenSSL 1.0.2, OpenSSL 1.1.0, and LibreSSL 2.8.2. The code from linked issue no longer crashes for me as well. Themis behavior is now identical across all supported crypto backends (BoringSSL already returns THEMIS_FAIL in this case).

LGTM 👍

I guess this resolves the issue. Thank you, @secumod!

@ignatk
Copy link
Contributor Author

ignatk commented Nov 1, 2018

Thanks for testing. I guess we should decide separately, if we should fix the return codes. Any opinions @vixentael ?

Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you @secumod and @ilammy for such quick testing and fixing

@vixentael
Copy link
Contributor

This fix will be part of 0.11.0 which will be released eventually (I think it makes sense to fix all issues found by @ilammy before release).

@vixentael vixentael merged commit 1a86f53 into cossacklabs:master Nov 1, 2018
@vixentael
Copy link
Contributor

I guess we should decide separately, if we should fix the return codes.

@secumod agree, I think matching Soter and Themis error codes requires more discussion and is definitely a different issue. Shall we open a new one?

@vixentael vixentael added the core Themis Core written in C, its packages label Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Themis Core written in C, its packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants