[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Fix security issue when reading username and password
From: |
Hector Marco |
Subject: |
Re: [PATCH] Fix security issue when reading username and password |
Date: |
Thu, 24 Oct 2019 17:34:54 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 |
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 <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
Re: [PATCH] Fix security issue when reading username and password, Vladimir 'phcoder' Serbinenko, 2019/10/24