[PATCH] verifiers: fix double close on pgp's sig file descriptor

[PATCH] verifiers: fix double close on pgp's sig file descriptor

[Michael Chang]

An error emerged as when I was tesing the verifiers branch, so instead
of putting it in pgp prefix, the verifiers is used to reflect what the
patch is based on.

While running verify_detached, grub aborts with error.

verify_detached /@/.snapshots/1/snapshot/boot/grub/grub.cfg
/@/.snapshots/1/snapshot/boot/grub/grub.cfg.sig

alloc magic is broken at 0x7beea660: 0
Aborted. Press any key to exit.

The error is caused by sig file desciptor been closed twice, first time
in grub_verify_signature() to which it is passed as parameter. Second in
grub_cmd_verify_signature() or in whichever opens the sig file
decriptor. The second close is not consider as bug to me either, as in
common rule of what opens a file has to close it to avoid file
descriptor leakage.

Afterall the design of grub_verify_signature() makes it diffcult to keep
a good trace on opened file descriptor from it's caller. Let's refine
the application interface to accept file path rather than descriptor, in
this way the caller doesn't have to care about closing the descriptor by
delegating it to grub_verify_signature() with full tracing to opened
file descriptor by itself.

Signed-off-by: Michael Chang <mchang@suse.com>
---
 grub-core/commands/pgp.c | 33 ++++++++++++++++-----------------
 include/grub/pubkey.h    |  2 +-
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/grub-core/commands/pgp.c b/grub-core/commands/pgp.c
index d5d7c0f0a..f294057b5 100644
--- a/grub-core/commands/pgp.c
+++ b/grub-core/commands/pgp.c
@@ -495,13 +495,12 @@ grub_verify_signature_init (struct grub_pubkey_context *ctxt, grub_file_t sig)
 
   grub_dprintf ("crypt", "alive\n");
 
-  ctxt->sig = sig;
-
   ctxt->hash_context = grub_zalloc (ctxt->hash->contextsize);
   if (!ctxt->hash_context)
     return grub_errno;
 
   ctxt->hash->init (ctxt->hash_context);
+  ctxt->sig = sig;
 
   return GRUB_ERR_NONE;
 }
@@ -684,15 +683,26 @@ grub_pubkey_close (void *ctxt)
 }
 
 grub_err_t
-grub_verify_signature (grub_file_t f, grub_file_t sig,
+grub_verify_signature (grub_file_t f, const char *fsig,
 		       struct grub_public_key *pkey)
 {
+  grub_file_t sig;
   grub_err_t err;
   struct grub_pubkey_context ctxt;
   grub_uint8_t *readbuf = NULL;
+
+  sig = grub_file_open (fsig,
+			GRUB_FILE_TYPE_SIGNATURE
+			| GRUB_FILE_TYPE_NO_DECOMPRESS);
+  if (!sig)
+    return grub_errno;
+
   err = grub_verify_signature_init (&ctxt, sig);
   if (err)
-    return err;
+    {
+      grub_file_close (sig);
+      return err;
+    }
 
   readbuf = grub_zalloc (READBUF_SIZE);
   if (!readbuf)
@@ -806,7 +816,7 @@ static grub_err_t
 grub_cmd_verify_signature (grub_extcmd_context_t ctxt,
 			   int argc, char **args)
 {
-  grub_file_t f = NULL, sig = NULL;
+  grub_file_t f = NULL;
   grub_err_t err = GRUB_ERR_NONE;
   struct grub_public_key *pk = NULL;
 
@@ -844,19 +854,8 @@ grub_cmd_verify_signature (grub_extcmd_context_t ctxt,
       goto fail;
     }
 
-  sig = grub_file_open (args[1],
-			GRUB_FILE_TYPE_SIGNATURE
-			| GRUB_FILE_TYPE_NO_DECOMPRESS);
-  if (!sig)
-    {
-      err = grub_errno;
-      goto fail;
-    }
-
-  err = grub_verify_signature (f, sig, pk);
+  err = grub_verify_signature (f, args[1], pk);
  fail:
-  if (sig)
-    grub_file_close (sig);
   if (f)
     grub_file_close (f);
   if (pk)
diff --git a/include/grub/pubkey.h b/include/grub/pubkey.h
index 4a9d04b43..fb8be9cbb 100644
--- a/include/grub/pubkey.h
+++ b/include/grub/pubkey.h
@@ -25,7 +25,7 @@ struct grub_public_key *
 grub_load_public_key (grub_file_t f);
 
 grub_err_t
-grub_verify_signature (grub_file_t f, grub_file_t sig,
+grub_verify_signature (grub_file_t f, const char *fsig,
 		       struct grub_public_key *pk);
 
 
-- 
2.19.0

Re: [PATCH] verifiers: fix double close on pgp's sig file descriptor

[Daniel Kiper]

On Tue, Nov 13, 2018 at 02:31:18PM +0800, Michael Chang wrote:
> An error emerged as when I was tesing the verifiers branch, so instead
> of putting it in pgp prefix, the verifiers is used to reflect what the
> patch is based on.
>
> While running verify_detached, grub aborts with error.
>
> verify_detached /@/.snapshots/1/snapshot/boot/grub/grub.cfg
> /@/.snapshots/1/snapshot/boot/grub/grub.cfg.sig
>
> alloc magic is broken at 0x7beea660: 0
> Aborted. Press any key to exit.
>
> The error is caused by sig file desciptor been closed twice, first time
> in grub_verify_signature() to which it is passed as parameter. Second in
> grub_cmd_verify_signature() or in whichever opens the sig file
> decriptor. The second close is not consider as bug to me either, as in
> common rule of what opens a file has to close it to avoid file
> descriptor leakage.
>
> Afterall the design of grub_verify_signature() makes it diffcult to keep
> a good trace on opened file descriptor from it's caller. Let's refine
> the application interface to accept file path rather than descriptor, in
> this way the caller doesn't have to care about closing the descriptor by
> delegating it to grub_verify_signature() with full tracing to opened
> file descriptor by itself.
>
> Signed-off-by: Michael Chang <mchang@suse.com>
> ---
>  grub-core/commands/pgp.c | 33 ++++++++++++++++-----------------
>  include/grub/pubkey.h    |  2 +-
>  2 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/grub-core/commands/pgp.c b/grub-core/commands/pgp.c
> index d5d7c0f0a..f294057b5 100644
> --- a/grub-core/commands/pgp.c
> +++ b/grub-core/commands/pgp.c
> @@ -495,13 +495,12 @@ grub_verify_signature_init (struct grub_pubkey_context *ctxt, grub_file_t sig)
>
>    grub_dprintf ("crypt", "alive\n");
>
> -  ctxt->sig = sig;
> -
>    ctxt->hash_context = grub_zalloc (ctxt->hash->contextsize);
>    if (!ctxt->hash_context)
>      return grub_errno;
>
>    ctxt->hash->init (ctxt->hash_context);
> +  ctxt->sig = sig;

This change does not seem to belong to this patch. Otherwise it LGTM.
You can split this patch into two patches or add a blurb about this change
into commit message or drop it at all. I would choose first option.

Daniel

Re: [PATCH] verifiers: fix double close on pgp's sig file descriptor

[Michael Chang]

On Wed, Nov 14, 2018 at 05:23:58PM +0100, Daniel Kiper wrote:
> On Tue, Nov 13, 2018 at 02:31:18PM +0800, Michael Chang wrote:
> > An error emerged as when I was tesing the verifiers branch, so instead
> > of putting it in pgp prefix, the verifiers is used to reflect what the
> > patch is based on.
> >
> > While running verify_detached, grub aborts with error.
> >
> > verify_detached /@/.snapshots/1/snapshot/boot/grub/grub.cfg
> > /@/.snapshots/1/snapshot/boot/grub/grub.cfg.sig
> >
> > alloc magic is broken at 0x7beea660: 0
> > Aborted. Press any key to exit.
> >
> > The error is caused by sig file desciptor been closed twice, first time
> > in grub_verify_signature() to which it is passed as parameter. Second in
> > grub_cmd_verify_signature() or in whichever opens the sig file
> > decriptor. The second close is not consider as bug to me either, as in
> > common rule of what opens a file has to close it to avoid file
> > descriptor leakage.
> >
> > Afterall the design of grub_verify_signature() makes it diffcult to keep
> > a good trace on opened file descriptor from it's caller. Let's refine
> > the application interface to accept file path rather than descriptor, in
> > this way the caller doesn't have to care about closing the descriptor by
> > delegating it to grub_verify_signature() with full tracing to opened
> > file descriptor by itself.
> >
> > Signed-off-by: Michael Chang <mchang@suse.com>
> > ---
> >  grub-core/commands/pgp.c | 33 ++++++++++++++++-----------------
> >  include/grub/pubkey.h    |  2 +-
> >  2 files changed, 17 insertions(+), 18 deletions(-)
> >
> > diff --git a/grub-core/commands/pgp.c b/grub-core/commands/pgp.c
> > index d5d7c0f0a..f294057b5 100644
> > --- a/grub-core/commands/pgp.c
> > +++ b/grub-core/commands/pgp.c
> > @@ -495,13 +495,12 @@ grub_verify_signature_init (struct grub_pubkey_context *ctxt, grub_file_t sig)
> >
> >    grub_dprintf ("crypt", "alive\n");
> >
> > -  ctxt->sig = sig;
> > -
> >    ctxt->hash_context = grub_zalloc (ctxt->hash->contextsize);
> >    if (!ctxt->hash_context)
> >      return grub_errno;
> >
> >    ctxt->hash->init (ctxt->hash_context);
> > +  ctxt->sig = sig;
> 
> This change does not seem to belong to this patch. Otherwise it LGTM.
> You can split this patch into two patches or add a blurb about this change
> into commit message or drop it at all. I would choose first option.

We can drop this. I just wanted to make it clear the sig descriptor is
not referenced in error returning path of grub_verify_signature_init()
so that we feel more comfortable to close it directly.

Thanks,
Michael

> 
> Daniel