Re: [PATCH] Fix security issue when reading username and password

[iripoll]

Hi Daniel,

  We wrote a blog entry explaining the problem and how it can be exploited:

http://hmarco.org/bugs/CVE-2015-8370-Grub2-authentication-bypass.html

   The underflow (although it is unsigned) takes effect in the function 
grub_memset.
The exploitation is extremely hard to do but possible, as shown in the blog.

Regards,
Ismael.

On 24/10/19 17:04, Daniel Kiper wrote:
> Hi Hector,
>
> On Thu, Oct 24, 2019 at 03:22:43PM +0100, Hector Marco wrote:
> > Hello Daniel,
> >
> > Something went wrong in my last email, what I wanted to say is:
> >
> > The patch prevents that "cur_len" underflows. No negative values for
> > "cur_len" so no way to underflow the "cur_len" variable and therefore no
> > vulnerability.
>
> First of all cur_len is unsigned. So, it does not get negative values at
> all. Though even it was signed I cannot see where in the code it can get
> negative value. Am I missing something?
>
> Daniel
>
> > Hector.
> >
> >
> > On 24/10/2019 15:13, Hector Marco wrote:
> > > Hello Daniel,
> > >
> > > The patch prevents that "cur_len" underflows. No negative values for
> > > "cur_len" so way to underflow the "cur_len" variable and therefore
> > >
> > > I hope this helps,
> > >
> > > Hector.
> > >
> > >
> > >
> > > On 23/10/2019 11:14, Daniel Kiper wrote:
> > > > On Fri, Oct 18, 2019 at 02:39:01PM +0200, Javier Martinez Canillas wrote:
> > > > > From: Hector Marco-Gisbert <address@hidden>
> > > > >
> > > > >    This patch fixes two integer underflows at:
> > > > >      * grub-core/lib/crypto.c
> > > > >      * grub-core/normal/auth.c
> > > > >
> > > > > Resolves: CVE-2015-8370
> > > > >
> > > > > Signed-off-by: Hector Marco-Gisbert <address@hidden>
> > > > > Signed-off-by: Ismael Ripoll-Ripoll <address@hidden>
> > > > > Signed-off-by: Javier Martinez Canillas <address@hidden>
> > > > > ---
> > > > >
> > > > >   grub-core/lib/crypto.c  | 2 +-
> > > > >   grub-core/normal/auth.c | 2 +-
> > > > >   2 files changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/grub-core/lib/crypto.c b/grub-core/lib/crypto.c
> > > > > index ca334d5a40e..e6c78d16d39 100644
> > > > > --- a/grub-core/lib/crypto.c
> > > > > +++ b/grub-core/lib/crypto.c
> > > > > @@ -468,7 +468,7 @@ grub_password_get (char buf[], unsigned buf_size)
> > > > >           break;
> > > > >         }
> > > > >
> > > > > -      if (key == '\b')
> > > > > +      if (key == '\b' && cur_len)
> > > > >         {
> > > > >           if (cur_len)
> > > > >             cur_len--;
> > > > > diff --git a/grub-core/normal/auth.c b/grub-core/normal/auth.c
> > > > > index 6be678c0de1..c35ce972473 100644
> > > > > --- a/grub-core/normal/auth.c
> > > > > +++ b/grub-core/normal/auth.c
> > > > > @@ -172,7 +172,7 @@ grub_username_get (char buf[], unsigned buf_size)
> > > > >           break;
> > > > >         }
> > > > >
> > > > > -      if (key == GRUB_TERM_BACKSPACE)
> > > > > +      if (key == GRUB_TERM_BACKSPACE && cur_len)
> > > > >         {
> > > > >           if (cur_len)
> > > > >             {
> > > >
> > > > TBH, I do not understand how this patch helps. It only delays continue
> > > > execution to the next "if (!grub_isprint (key))" if cur_len == 0.
> > > >
> > > > Daniel