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@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.
>
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
Attachment:
signature.asc
Description: PGP signature
