Mimi,
On Thu, Aug 19, 2021 at 02:06:03PM -0400, Mimi Zohar wrote:
> On Thu, 2021-08-19 at 05:11 +0300, Vitaly Chikunov wrote:
> > After CRYPTO_secure_malloc_init OpenSSL will store private keys in
> > secure heap. This facility is only available since OpenSSL_1_1_0-pre1.
> >
> > Signed-off-by: Vitaly Chikunov <vt@xxxxxxxxxxxx>
> > ---
> > Change from v1:
> > - Do not use setfbuf to disable buffering as this is not proven to be
> > meaningful.
> > - Use secure heap for passwords too as suggested by Mimi Zohar.
> > - Fallback to OPENSSL_malloc for old OpenSSL as suggested by Mimi Zohar.
> > - Simplify logic of calling CRYPTO_secure_malloc_init (call it always on
> > OpenSSL init.)
> > - Should be applied after Bruno Meneguele's "evmctl: fix memory leak in
> > get_password" patch v2.
>
> Not sure why it isn't applying with/without Bruno's v2 patch.
It should be over next-testing + (manually git am'ed) Bruno's patch:
db25fcd 2021-08-19 03:20:48 +0300 Use secure heap for private keys and passwords (Vitaly Chikunov)
d37ea6d 2021-08-16 12:15:59 -0300 evmctl: fix memory leak in get_password (Bruno Meneguele)
b1818c1 2021-08-03 16:40:08 -0400 Create alternative tpm2_pcr_read() that uses IBM TSS (Ken Goldman) (origin/next-testing)
>
> >
> > src/evmctl.c | 143 ++++++++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 118 insertions(+), 25 deletions(-)
> >
> > diff --git a/src/evmctl.c b/src/evmctl.c
>
> > @@ -188,7 +207,9 @@ static int bin2file(const char *file, const char *ext, const unsigned char *data
> > return err;
> > }
> >
> > -static unsigned char *file2bin(const char *file, const char *ext, int *size)
> > +/* Return data in OpenSSL secure heap if 'secure' is true. */
> > +static unsigned char *file2bin(const char *file, const char *ext, int *size,
> > + int secure)
> > {
> > FILE *fp;
> > size_t len;
> > @@ -215,7 +236,10 @@ static unsigned char *file2bin(const char *file, const char *ext, int *size)
> > }
> > len = stats.st_size;
> >
> > - data = malloc(len);
> > + if (secure)
> > + data = OPENSSL_secure_malloc(len);
> > + else
> > + data = malloc(len);
>
> Without being able to apply the patch, it's hard to tell if there
> should be a preparatory patch that first replaces malloc() with
> OPENSSL_malloc(), and other similar changes.
There is no OPENSSL_malloc use and I don't see why it should be.
Thanks,
>
> thanks,
>
> Mimi
>
> > if (!data) {
> > log_err("Failed to malloc %zu bytes: %s\n", len, name);
> > fclose(fp);
