Add password prompting support & EVP_read_pw_string #2419
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2419 +/- ##
==========================================
+ Coverage 78.81% 78.87% +0.05%
==========================================
Files 621 623 +2
Lines 108431 108874 +443
Branches 15398 15442 +44
==========================================
+ Hits 85462 85875 +413
- Misses 22299 22325 +26
- Partials 670 674 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // if there is not enough room. If either |buf| or |userdata| is NULL, 0 is | ||
| // returned. Note that this is different from OpenSSL, which prompts for a | ||
| // password. | ||
| // PEM_def_callback provides a password for PEM encryption/decryption operations. |
There was a problem hiding this comment.
We should mention that this function is used as the default callback to provide a password for PEM functions such as PEM_do_header and PEM_ASN1_write_bio. Currently, the documentation for PEM_ASN1_write_bio makes no mention of this, and PEM_do_header is undocumented.
There was a problem hiding this comment.
I see, there's more use cases for the PEM_def_callback. I'll update the comments here to talk about the other 2 funcs as well. Looks like both PEM_do_header and PEM_ASN1_write_bio are undocumented, wrote new stuff for them
| } | ||
|
|
||
| // Proactively zeroize |buf| and verify_buf | ||
| OPENSSL_cleanse(buf, sizeof(buf)); |
There was a problem hiding this comment.
| OPENSSL_cleanse(buf, sizeof(buf)); | |
| OPENSSL_cleanse(buf, length); |
This should be length I think? Otherwise you are just getting the size of the pointer no?
| #include <stdio.h> | ||
| #include <string.h> | ||
| #include <signal.h> | ||
| #include <string.h> |
There was a problem hiding this comment.
| #include <string.h> |
Already imported on line 7
| // Read password with echo disabled, returns 1 on success, 0 on error, -2 on interrupt | ||
| ret = openssl_console_read(buf, min_length, length, 0); | ||
| if (ret != 0) { | ||
| OPENSSL_cleanse(buf, sizeof(buf)); |
There was a problem hiding this comment.
| OPENSSL_cleanse(buf, sizeof(buf)); | |
| OPENSSL_cleanse(buf, length); |
Same question if this should be length
| if (strncmp(buf, verify_buf, length >= 1024 ? 1024 : length) != 0) { | ||
| openssl_console_write("Verify failure\n"); | ||
| ret = -1; | ||
| } |
There was a problem hiding this comment.
This seems like a weird behavior to me? We only verify the first 1024 characters if the input length is longer then 1024? Should we just set a hard limit that length can't exceed 1024?
There was a problem hiding this comment.
Also I guess is there any constant time concerns around comparing this value in memory?
Issues:
CryptoAlg-3060Description of changes:
EVP_read_pw_stringPEM_def_callback's behavior with OpenSSL. Previously, this function would fail if theuserdataparam was NULL. This is different from openSSL which would prompt a user for a password in the same scenario. NowPEM_def_callbackwill correctly prompt the user ifuserdatais NULL.PEM_def_callback. Add carriage return support so this tool works on windows.Call-outs:
Testing:
We only test the stdin stdout routes in GTest. Console functionality was manually tested.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.