Hi Daniel, Sorry I didn't pay to much attention to this. Yes, it is unsigned but the underflow is still there.
Maybe what you are missing is that this underflowed value (cur_len) is later used in grub_memset( buf + cur_len, 0, buf_size - cur_len); which results in a buffer overflow (not underflow this time). As Ismael say, please read the entry we wrote in 2015 http://hmarco.org/bugs/CVE-2015-8370-Grub2-authentication-bypass.html Best regards, Hector. On 24/10/2019 16: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 <hecma...@upv.es> >>>>> >>>>> 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 <hecma...@upv.es> >>>>> Signed-off-by: Ismael Ripoll-Ripoll <irip...@disca.upv.es> >>>>> Signed-off-by: Javier Martinez Canillas <javi...@redhat.com> >>>>> --- >>>>> >>>>> 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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
