On Thu, Aug 19, 2021 at 09:12:25PM +0300, Vitaly Chikunov wrote:
> 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@altlinux.org>
> > > ---
> > > 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.
> 

Keeping the OPENSSL_* calls as a meaning of "secure calls" while keeping
the standard C library calls for "non-secure" seems indeed cleaner.

> Thanks,
> 
> > 
> > thanks,
> > 
> > Mimi
> > 
> > >  	if (!data) {
> > >  		log_err("Failed to malloc %zu bytes: %s\n", len, name);
> > >  		fclose(fp);
> 

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt