On 9/5/21 4:11 AM, Vitaly Chikunov wrote:
Stefan,
On Sat, Sep 04, 2021 at 09:10:50PM -0400, Stefan Berger wrote:
On 9/4/21 6:50 AM, Vitaly Chikunov wrote:
After CRYPTO_secure_malloc_init OpenSSL will automatically store private
keys in secure heap. OPENSSL_secure_malloc(3):
If a secure heap is used, then private key BIGNUM values are stored
there. This protects long-term storage of private keys, but will not
necessarily put all intermediate values and computations there.
Additionally, we try to keep user passwords in secure heap too.
This facility is only available since OpenSSL_1_1_0-pre1.
Signed-off-by: Vitaly Chikunov <vt@xxxxxxxxxxxx>
Reviewed-by: Bruno Meneguele <bmeneg@xxxxxxxxxx>
Cc: Stefan Berger <stefanb@xxxxxxxxxxxxx>
---
Change since v3:
- Undo secure heap handling from (file2bin and) calc_evm_hmac since this
is debugging tool only. Add comment about this.
- Since there is only code removals and new comments I keep Reviewed-by
tag.
src/evmctl.c | 97 +++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 85 insertions(+), 12 deletions(-)
@@ -2596,15 +2616,41 @@ static struct option opts[] = {
};
+/*
+ * Copy password from optarg into secure heap, so it could be
+ * freed in the same way as a result of get_password().
+ */
+static char *optarg_password(char *optarg)
+{
Mimi applied my patch that takes the password from an environment variable
to the next-testing branch. The code there (imaevm_params.keypass =
getenv("..."))would have to change as well calling this function here. Even
though the man page of getenv says that 'The caller must take care not to
modify this string, since that would change the environment of the process'
we should be able to overwrite the env variable's password value just like
here. Maybe for this purpose rename this function to dup_password() ?
`/proc/<pid>/environ' is never world readable (unlike `cmdline'), so we
don't even need to overwrite it there. But, we can.
Or you could add an option for whether to overwrite the source...
(Btw, I found the 'EVMCTL_KEY_PASSWORD' patch.)
@@ -2643,6 +2691,7 @@ int main(int argc, char *argv[])
ENGINE *eng = NULL;
unsigned long keyid;
char *eptr;
+ char *keypass = NULL; /* @secure heap */
#if !(OPENSSL_VERSION_NUMBER < 0x10100000)
OPENSSL_init_crypto(
@@ -2651,6 +2700,17 @@ int main(int argc, char *argv[])
#endif
OPENSSL_INIT_ENGINE_ALL_BUILTIN, NULL);
#endif
+#if OPENSSL_VERSION_NUMBER > 0x10100000
+ /*
+ * This facility is available since OpenSSL_1_1_0-pre1.
Shouldn't the comparison then not include 0x10100000?
#if OPENSSL_VERSION_NUMBER >= 0x10100000
The number is from `include/openssl/opensslv.h' from the time when
relevant commit is already applied. So, I use `>'. It seems good match
for OpenSSL_1_1_0-pre1 too:
$ git grep OPENSSL_VERSION_NUMBER OpenSSL_1_1_0-pre1:include/openssl/opensslv.h
OpenSSL_1_1_0-pre1:include/openssl/opensslv.h:# define OPENSSL_VERSION_NUMBER 0x10100001L
You are right. I had checkout out this commit here:
commit abd30777cc72029e8a44e4b67201cae8ed3d19c1 (HEAD -> OpenSSL_1_1_0,
tag: OpenSSL_1_1_0)
Author: Matt Caswell <matt@xxxxxxxxxxx>
Date: Thu Aug 25 16:29:18 2016 +0100
Prepare for 1.1.0 release
Reviewed-by: Richard Levitte <levitte@xxxxxxxxxxx
It shows this here:
$ grep -r OPENSSL_VERSION_NUM ./include/
./include/openssl/crypto.h:# define SSLEAY_VERSION_NUMBER
OPENSSL_VERSION_NUMBER
./include/openssl/opensslv.h:# define OPENSSL_VERSION_NUMBER 0x1010000fL
There's an 'f' at the 'end'.
Stefan
Thanks,
