[v1] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read

[edk2] [PATCH] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read

Posted by Jian J Wang 6 years, 9 months ago

In the for-loop condition of original code, the expression

  *CurrentCommand != CHAR_NULL 

is put before expression

  CurrentCommand < SortedCommandList + SortedCommandListSize/sizeof(CHAR16)

When CurrentCommand walks to the end of string buffer, one more character
over the end of string buffer will be read and then stop.

To fix this issue, just move the last expression to the first one. Because
of short-circuit evaludation of and-expression, the following one

  *CurrentCommand != CHAR_NULL

will not be evaluated if the expression before it is evaludated as FALSE.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c b/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
index a71ade3a20..75e3d74200 100644
--- a/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
+++ b/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
@@ -397,7 +397,7 @@ ShellCommandRunHelp (
         CopyListOfCommandNamesWithDynamic(&SortedCommandList, &SortedCommandListSize);
 
         for (CurrentCommand = SortedCommandList 
-          ; CurrentCommand != NULL && *CurrentCommand != CHAR_NULL && CurrentCommand < SortedCommandList + SortedCommandListSize/sizeof(CHAR16)
+          ; CurrentCommand < SortedCommandList + SortedCommandListSize/sizeof(CHAR16) && CurrentCommand != NULL && *CurrentCommand != CHAR_NULL
           ; CurrentCommand += StrLen(CurrentCommand) + 1
           ) {
           //
-- 
2.15.1.windows.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read

Posted by Ni, Ruiyu 6 years, 9 months ago

On 1/23/2018 10:14 AM, Jian J Wang wrote:
> In the for-loop condition of original code, the expression
> 
>    *CurrentCommand != CHAR_NULL
> 
> is put before expression
> 
>    CurrentCommand < SortedCommandList + SortedCommandListSize/sizeof(CHAR16)
> 
> When CurrentCommand walks to the end of string buffer, one more character
> over the end of string buffer will be read and then stop.
> 
> To fix this issue, just move the last expression to the first one. Because
> of short-circuit evaludation of and-expression, the following one
> 
>    *CurrentCommand != CHAR_NULL
> 
> will not be evaluated if the expression before it is evaludated as FALSE.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>   ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c b/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
> index a71ade3a20..75e3d74200 100644
> --- a/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
> +++ b/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
> @@ -397,7 +397,7 @@ ShellCommandRunHelp (
>           CopyListOfCommandNamesWithDynamic(&SortedCommandList, &SortedCommandListSize);
>   
>           for (CurrentCommand = SortedCommandList
> -          ; CurrentCommand != NULL && *CurrentCommand != CHAR_NULL && CurrentCommand < SortedCommandList + SortedCommandListSize/sizeof(CHAR16)
> +          ; CurrentCommand < SortedCommandList + SortedCommandListSize/sizeof(CHAR16) && CurrentCommand != NULL && *CurrentCommand != CHAR_NULL
>             ; CurrentCommand += StrLen(CurrentCommand) + 1
>             ) {
>             //
> 
How about keep "CurrentCommand != NULL" in the beginning? I agree to
swap the other two checks.

-- 
Thanks,
Ray
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read

Posted by Wang, Jian J 6 years, 9 months ago

Fair enough. It'll be updated in v2.

Regards,
Jian


> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Wednesday, January 24, 2018 11:36 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [PATCH] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read
> 
> On 1/23/2018 10:14 AM, Jian J Wang wrote:
> > In the for-loop condition of original code, the expression
> >
> >    *CurrentCommand != CHAR_NULL
> >
> > is put before expression
> >
> >    CurrentCommand < SortedCommandList +
> SortedCommandListSize/sizeof(CHAR16)
> >
> > When CurrentCommand walks to the end of string buffer, one more character
> > over the end of string buffer will be read and then stop.
> >
> > To fix this issue, just move the last expression to the first one. Because
> > of short-circuit evaludation of and-expression, the following one
> >
> >    *CurrentCommand != CHAR_NULL
> >
> > will not be evaluated if the expression before it is evaludated as FALSE.
> >
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >   ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
> b/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
> > index a71ade3a20..75e3d74200 100644
> > --- a/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
> > +++ b/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
> > @@ -397,7 +397,7 @@ ShellCommandRunHelp (
> >           CopyListOfCommandNamesWithDynamic(&SortedCommandList,
> &SortedCommandListSize);
> >
> >           for (CurrentCommand = SortedCommandList
> > -          ; CurrentCommand != NULL && *CurrentCommand != CHAR_NULL &&
> CurrentCommand < SortedCommandList +
> SortedCommandListSize/sizeof(CHAR16)
> > +          ; CurrentCommand < SortedCommandList +
> SortedCommandListSize/sizeof(CHAR16) && CurrentCommand != NULL &&
> *CurrentCommand != CHAR_NULL
> >             ; CurrentCommand += StrLen(CurrentCommand) + 1
> >             ) {
> >             //
> >
> How about keep "CurrentCommand != NULL" in the beginning? I agree to
> swap the other two checks.
> 
> --
> Thanks,
> Ray
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel